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

Tweak enum serialization to generate better LLVM IR and more compact code #77

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

alessandrod
Copy link
Contributor

Hello!

I'm looking at reducing the size of some programs that use borsh, and I've noticed that deriving BorshSerialize on enums can sometimes trigger some pretty bad inlining from rustc/LLVM.

This PR changes enum serialization from:

  match self {
    Enum::Variant1(field1, field2, ...) => {
        // serialize variant idx
        let variant_idx: u8 = 0;
        writer.write_all(&variant_idx.to_le_bytes())?;

        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...
    }
    Enum::Variant2(field1, field2, ...) => {
        // serialize variant idx
        let variant_idx: u8 = 1;
        writer.write_all(&variant_idx.to_le_bytes())?;

        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...

    }
    ...
  }

To:

  let variant_idx: u8 = match self {
    Enum::Variant1(..) => 0,
    Enum::Variant2(..) => 1,
    ...
  };
  // serialize variant_idx
  writer.write_all(&variant_idx.to_le_bytes())?;

  match self {
    Enum::Variant1(field1, field2, ...) => {
        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...
    }
    Enum::Variant2(field1, field2, ...) => {
        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...

    }
    ...
  }

The latter generates better LLVM IR, and avoids writer.write_all(&variant_idx.to_le_bytes())? from being inlined into each match branch.

Here's a test case that shows the issue: https://gist.github.com/alessandrod/f192c59f6b75159733d2cb6c60a78f1b

Cargo bloat before:

 File  .text    Size Crate Name
13.8%  36.5%  6.1KiB bopt? <bopt::LargeEnum as borsh::ser::BorshSerialize>::serialize
 4.6%  12.0%  2.0KiB   std core::fmt::Formatter::pad_integral
 3.2%   8.5%  1.4KiB   std <&T as core::fmt::Display>::fmt
 3.1%   8.2%  1.4KiB   std <core::fmt::Arguments as core::fmt::Display>::fmt
 2.7%   7.0%  1.2KiB   std core::fmt::num::imp::<impl core::fmt::Display for u64>::fmt
10.5%  27.8%  4.7KiB       And 33 smaller methods. Use -n N to show more.
37.9% 100.0% 16.9KiB       .text section size, the file size is 44.4KiB

Cargo bloat after:

 File  .text    Size Crate Name
 6.8%  19.0%  2.0KiB   std core::fmt::Formatter::pad_integral
 4.8%  13.3%  1.4KiB   std <&T as core::fmt::Display>::fmt
 4.7%  12.9%  1.4KiB   std <core::fmt::Arguments as core::fmt::Display>::fmt
 4.0%  11.0%  1.2KiB   std core::fmt::num::imp::<impl core::fmt::Display for u64>::fmt
 2.9%   8.1%    888B   std <core::str::iter::CharIndices as core::iter::traits::iterator::Iterator>::next
12.9%  35.7%  3.8KiB       And 31 smaller methods. Use -n N to show more.
36.1% 100.0% 10.7KiB       .text section size, the file size is 29.7KiB

…code

Change enum serialization form

  match self {
    Enum::Variant1(field1, field2, ...) => {
        // serialize variant idx
        let variant_idx: u8 = 0;
        writer.write_all(&variant_idx.to_le_bytes())?;

        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...
    }
    Enum::Variant2(field1, field2, ...) => {
        let variant_idx: u8 = 1;
        writer.write_all(&variant_idx.to_le_bytes())?;

        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...

    }
    ...
  }

To:

  let variant_idx: u8 = match self {
    Enum::Variant1(..) => 0,
    Enum::Variant2(..) => 1,
    ...
  };
  // serialize variant_idx
  writer.write_all(&variant_idx.to_le_bytes())?;

  match self {
    Enum::Variant1(field1, field2, ...) => {
        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...
    }
    Enum::Variant2(field1, field2, ...) => {
        // serialize variant fields
        BorshSerialize::serialize(field1, writer)?;
        BorshSerialize::serialize(field2, writer)?;
        ...

    }
    ...
  }

The latter generates better LLVM IR, and avoids
writer.write_all(&variant_idx.to_le_bytes())?; from being inlined into each
match branch.
@matklad
Copy link
Contributor

matklad commented Feb 3, 2022

excellent find, thanks!

@matklad matklad merged commit f341871 into near:master Feb 3, 2022
matklad added a commit to matklad/borsh-rs that referenced this pull request Feb 3, 2022
matklad added a commit that referenced this pull request Feb 4, 2022
* CI: drop coverage reports from CI

Coverage doesn't work today:

https://github.com/near/borsh-rs/runs/4955323970?check_suite_focus=true#step:3:1141

We might think of reviving it in the future, but first, let's makes sure
that our CI actually reflects de-facto practice

* CI: test all feature combinations

We didn't check that `--no-default-features` works, and it did broke.

Note that we `pushd borsh` before testing -- this is to make sure that
other ws crates (like fuzzing) don't enable extra features.

* ci: drop needless rustfmt args

* fix: fix no_std build

* chore: changelog for #77

* publish 0.9.3
@alessandrod alessandrod deleted the enum_ser_smol branch May 12, 2022 22:53
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.

3 participants