Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Enable wasm-opt optimizations in wasm-builder #13408

Closed
wants to merge 11 commits into from
Closed

Enable wasm-opt optimizations in wasm-builder #13408

wants to merge 11 commits into from

Conversation

athei
Copy link
Member

@athei athei commented Feb 17, 2023

A while back we made the switch from the abandoned wasm-gc to wasm-opt. However, we didn't enable any optimizations in order to make it a in-place replacement at first.

The plan was always to use those optimizations as we have seen big gains especially when running wasmi under wasmtime. The reason why this is the case is that LLVM as a register machine has a hard time optimizing wasm code. Hence this postprocessing step can be quite powerful.

A burn in with those optimization enabled has already been performed by @koute. What is left is running all benchmarks to make sure we are not producing a performance regression.

For release builds we are using O1 which just adds 3 seconds to the build. For production builds we use the slowest profile O4 which adds 6 minutes on my machine. The production profile is already pretty slow and only used for benchmarking and releases. No developer should be using this profile locally in their workflow.

@alvicsam How can a produce a new baseline in this PR using the old bare metal machines?

cc @Robbepop

Weight Compare

@athei athei added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes labels Feb 17, 2023
@athei athei requested a review from koute as a code owner February 17, 2023 13:31
@athei athei added T9-release This PR/Issue is related to topics touching the release notes. T0-node This PR/Issue is related to the topic “node”. and removed T9-release This PR/Issue is related to topics touching the release notes. labels Feb 17, 2023
@paritytech paritytech deleted a comment from command-bot bot Mar 1, 2023
@paritytech paritytech deleted a comment from command-bot bot Mar 1, 2023
@paritytech paritytech deleted a comment from oleg-plakida Mar 1, 2023
@paritytech paritytech deleted a comment from oleg-plakida Mar 1, 2023
@paritytech paritytech deleted a comment from oleg-plakida Mar 1, 2023
@athei
Copy link
Member Author

athei commented Mar 16, 2023

Don't merge we need to do another bench all after #13595 is merged.

@athei
Copy link
Member Author

athei commented Mar 16, 2023

bot bench $ all

@command-bot
Copy link

command-bot bot commented Mar 16, 2023

@athei https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2539552 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" all. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 74-ea91ff7a-22a8-4392-be82-96127994991f to cancel this command or bot cancel to cancel all commands in this pull request.

@paritytech-ci paritytech-ci requested a review from a team March 17, 2023 03:34
@command-bot
Copy link

command-bot bot commented Mar 17, 2023

@athei Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" all has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2539552 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2539552/artifacts/download.

@athei
Copy link
Member Author

athei commented Mar 17, 2023

bot bench $ all

@command-bot
Copy link

command-bot bot commented Mar 17, 2023

@athei https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2541022 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" all. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 76-f4c06778-b492-4a93-b24e-023a6262be0f to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 17, 2023

@athei Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" all has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2541022 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2541022/artifacts/download.

@ggwpez
Copy link
Member

ggwpez commented Mar 17, 2023

The last change does not seem to make a difference. Asymptotic is stable.

@athei
Copy link
Member Author

athei commented Mar 17, 2023

Yeah if O4 and O2 are similar we can just go with O2 which is more conservative and faster. But the optimizations don't seems to help overall. They are supposed to be more important for newer wasmi versions. I will check the effect of them on the new wasmi version here: #13312 before merging this.

@athei
Copy link
Member Author

athei commented Mar 19, 2023

Neither this PR nor under the new wasmi 0.28 in #13312 we saw any performance improvements with wasm-opt. On the contrary: The performance regressed. This is why we do not enable the optimization. It should only be done if clear improvements can be demonstrated.

Can be revisited later but for now there does not seem to be a reason to enable it. Maybe we can enable some selective passes that deal with code size if we want to. Or we may look into it again once we are ready to use more wasm features (post MVP).

@athei athei closed this Mar 19, 2023
@athei athei deleted the at/wasm-opt branch May 1, 2023 07:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants