Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: optimize BorshSerialize derive for enums with unit variants #262

Merged
merged 5 commits into from
Dec 6, 2023

Conversation

Fuuzetsu
Copy link
Contributor

@Fuuzetsu Fuuzetsu commented Dec 6, 2023

Given enums like these:

enum Foo {
    A(u16),
    B,
    C(i32, i32),
}

enum Bar {
    A,
    B,
    C,
}

serialise derive produces something like these:

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Foo {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Foo::A(..) => 0u8,
            Foo::B => 1u8,
            Foo::C(..) => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Foo::A(id0) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
            }
            Foo::B => {}

            Foo::C(id0, id1) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
                borsh::BorshSerialize::serialize(id1, writer)?;
            }
        }
        Ok(())
    }
}

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Bar {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Bar::A => 0u8,
            Bar::B => 1u8,
            Bar::C => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Bar::A => {}

            Bar::B => {}

            Bar::C => {}
        }
        Ok(())
    }
}

Notably in bar case, the whole match self is useless because there's nothing left to serialise.

With this patch, the derives now look like this:

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Foo {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Foo::A(..) => 0u8,
            Foo::B => 1u8,
            Foo::C(..) => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Foo::A(id0) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
            }
            Foo::C(id0, id1) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
                borsh::BorshSerialize::serialize(id1, writer)?;
            }
            _ => {}
        }
        Ok(())
    }
}

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Bar {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Bar::A => 0u8,
            Bar::B => 1u8,
            Bar::C => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        Ok(())
    }
}

Notably, the whole match self is gone for Bar. For Foo, any unit field cases are now inside a _ => {} catch-all.

What's the point? Well, it's just nice to produce less for compiler to deal with, reducing upstream build times. Further, it makes it much nicer for anyone reading/copying the expanded macros.

However patch this does add some amount of code complexity so it's up to the maintainers to decide if it's worth taking.

dj8yf0μl and others added 3 commits December 6, 2023 19:47
Given enums like these:

```rust
enum Foo {
    A(u16),
    B,
    C(i32, i32),
}

enum Bar {
    A,
    B,
    C,
}
```

serialise derive produces something like these:

```rust
// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Foo {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Foo::A(..) => 0u8,
            Foo::B => 1u8,
            Foo::C(..) => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Foo::A(id0) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
            }
            Foo::B => {}

            Foo::C(id0, id1) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
                borsh::BorshSerialize::serialize(id1, writer)?;
            }
        }
        Ok(())
    }
}

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Bar {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Bar::A => 0u8,
            Bar::B => 1u8,
            Bar::C => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Bar::A => {}

            Bar::B => {}

            Bar::C => {}
        }
        Ok(())
    }
}
```

Notably in `Bar` case, the whole `match self` is useless because there's
nothing left to serialise.

With this patch, the derives now look like this:

```rust
// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Foo {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Foo::A(..) => 0u8,
            Foo::B => 1u8,
            Foo::C(..) => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        match self {
            Foo::A(id0) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
            }
            Foo::C(id0, id1) => {
                borsh::BorshSerialize::serialize(id0, writer)?;
                borsh::BorshSerialize::serialize(id1, writer)?;
            }
            _ => {}
        }
        Ok(())
    }
}

// Recursive expansion of borsh::BorshSerialize macro
// ===================================================

impl borsh::ser::BorshSerialize for Bar {
    fn serialize<W: borsh::io::Write>(
        &self,
        writer: &mut W,
    ) -> ::core::result::Result<(), borsh::io::Error> {
        let variant_idx: u8 = match self {
            Bar::A => 0u8,
            Bar::B => 1u8,
            Bar::C => 2u8,
        };
        writer.write_all(&variant_idx.to_le_bytes())?;
        Ok(())
    }
}
```

Notably, the whole `match self` is gone for `Bar`. For `Foo`, any unit
field cases are now inside a `_ => {}` catch-all.

What's the point? Well, it's just nice to produce less for compiler to
deal with, reducing upstream build times. Further, it makes it much
nicer for anyone reading/copying the expanded macros.

However patch this does add some amount of code complexity so it's up to
the maintainers to decide if it's worth taking.
@dj8yfo dj8yfo changed the title Do not produce useless match cases unit enums chore: optimize BorshSerialize derive for enums with unit variants Dec 6, 2023
@dj8yfo dj8yfo merged commit 73db73e into near:master Dec 6, 2023
7 checks passed
@frol frol mentioned this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants