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

feat: enable wasm optimizations using binaryen #433

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jun 2, 2022

This enables both the speed (3) and size (z) optimizations, bringing the miner actor down to 1MiB.

Draft because, while this PR is correct, there's a bug in the FVM filecoin-project/ref-fvm#602.


This PR has two main parts:

  1. It strips the binary, bringing, e.g., the miner actor down to 1.4MiB (ish).
  2. It then uses binaryen to both optimize and further trim down the binary to 1.0MiB.

Motivation:

  1. Reduce the block sizes (try to get them down below a megabyte).
  2. Reduce the CAR size. With these changes (even without the binaryen optimization step), we can get the compressed CAR size down to about half a megabyte.
  3. Optimize the WASM itself for better performance. Parity is doing this with substrate as well.

In terms of how much this helps.... we'll need more thorough testing, but it looks like it helps a bit...

The main downside of this change is that it would:

  1. Add an additional compile-time dep on a non-rust project (binaryen).
  2. Add a compile-time dependency on cmake (to build binaryen).

I'm happy to drop the binaryen part if we feel that it's too risky, too late. But I'd like to drop the debug info from the release builds so we can get the bundle size down. This would let us check-in the main bundles into the lotus source, significantly simplifying the build process.

@Stebalien Stebalien marked this pull request as ready for review June 2, 2022 19:10
@Stebalien
Copy link
Member Author

Let's start with #434. That only reduces the bundle size and doesn't optimize the actors, but it's a good start.

@ZenGround0
Copy link
Contributor

I think running the optimized code with dual execution in a debug bundle (assuming we can get gas/time reports from both) to check on performance improvements and get more confidence for correctness would be an excellent thing to do post M1 launch.

This enables both the speed (3) and size (z) optimizations, bringing the
miner actor down to 1MiB.
@anorth
Copy link
Member

anorth commented Aug 12, 2022

@Stebalien what's the status here? Do we still want to do this?

@Stebalien
Copy link
Member Author

Yes we probably still want it. At a minimum, it reduces binary size a bit.

It may also make things faster (actually, I'm pretty sure it does by optimizing some bulk memory operations), but we haven't merged it because we haven't benchmarked that yet.

Ideally we'd be able to use our conformance test benchmarks, but those don't give us accurate numbers at the moment.

@vyzo
Copy link
Contributor

vyzo commented Sep 14, 2022

we should run some gas benchmarks, this is very desirable.

@Stebalien
Copy link
Member Author

@Kubuxu did your benchmark show any benefit?

@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 22, 2022

In my benchmark it saves 0.8%.

@Stebalien
Copy link
Member Author

Stebalien commented Oct 11, 2022 via email

@Stebalien
Copy link
Member Author

Yeah, I've done some more benchmarking, and this mostly just makes the build really slow.

@Stebalien Stebalien closed this Dec 9, 2022
@Stebalien Stebalien deleted the feat/wasm-opt branch December 9, 2022 22:41
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.

None yet

5 participants