-
Notifications
You must be signed in to change notification settings - Fork 801
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
Update Wasmi to v0.32.0-beta.5
#2941
Update Wasmi to v0.32.0-beta.5
#2941
Conversation
- cargo check -p pallet_contracts passes - cargo test -p pallet_contracts passes
v0.32.0 beta.5
v0.32.0-beta.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Can you run the benchmarks in here?
bot bench substrate-pallet --pallet=pallet_contracts |
@Robbepop https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4929394 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
@Robbepop Command |
@athei Shall we also enable lazy compilation and validation as it is now supported by Wasmi? |
Yes. But please first update the instruktion benchmark with something that cannot be optimized away by wasmi so we can get a clearer picture: polkadot-sdk/substrate/frame/contracts/src/benchmarking/mod.rs Lines 2585 to 2602 in 05cfb02
Can you also post the subweight table with the weight here? |
The rendered benchmarks from the last commit (without lazy compilation enabled):
As image with colors: Note to others: The -95% in the |
@athei This needs to be done in another PR to take effect for this PR I assume? |
It is interesting that everything regarding compilation (e.g |
I also am a bit confused about this. The register-machine is the default in this version and is used in the benchmarks since the stack-machine got removed entirely. I refactored the Wasm translation part quite significantly, so maybe that is an explanation, although I would have not even remotely hoped for such significant improvements as a result to be honest. |
Maybe also a case of our benchmark being too simplistic? We try to nest blocks as this is assumed to be the hardest to compile. This how how we generate a big contract and then measure the time to compile it: polkadot-sdk/substrate/frame/contracts/src/benchmarking/code.rs Lines 279 to 300 in 05cfb02
|
Seeing the code snippet that is generated: It uses |
Yes you seeing this correctly. The fix should be easy though: Replace the constant by a memory load. It will still immediately return at runtime but the wasmi can't optimize it away. |
Any of |
Can you update that benchmark, too? |
The question that still stands is if this adjustments of benchmarks is required in a separate PR as to properly run the new or adjusted test cases on both |
Yes I would suggest to do it in a separate PR that we merge before this one for better comparability. |
In #2941 we found out that the new Wasmi (register) is very effective at optimizing away certain benchmark bytecode constructs in a way that created an unfair advantage over Wasmi (stack) which yielded our former benchmarks to be ineffective at properly measuring the performance impact. This PR adjusts both affected benchmarks to fix the stated problems. Affected are - `instr_i64const` -> `instr_i64add`: Renamed since it now measures the performance impact of the Wasm `i64.add` instruction with locals as inputs and outputs. This makes it impossible for Wasmi (register) to aggressively optimize away the entire function body (as it previously did) but still provides a way for Wasmi (register) to shine with its register based execution model. - `call_with_code_per_byte`: Now uses `local.get` instead of `i32.const` for the `if` condition which prevents Wasmi (register) to aggressively optimizing away whole parts of the `if` creating an unfair advantage. cc @athei --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
bot bench substrate-pallet --pallet=pallet_contracts |
@Robbepop https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4977631 was started for your command Comment |
…=dev --target_dir=substrate --pallet=pallet_contracts
…=dev --target_dir=substrate --pallet=pallet_contracts
This comment was marked as outdated.
This comment was marked as outdated.
I think it is safe to say that the benchmarks for The The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense that the seal_*
host function benchmarks regress. Even when using lazy compilation it will still compile on execute. Reason is that we just call the high level call
function as benchmark. This will always load from storage, compile, execute. So it always benchmarks that compilation, too.
Lazy compilation even makes it worse. This is because all the code is actually executed and I assume doing it in one go is more efficient than using the lazy compilation. Even though it shouldn't make a big difference since all the code is in once function.
We need to change the benchmarks to compile the code in its setup so that when executing the contract it is only execution.
Always use CompilationMode::Eager allows us to remove all the parameter passing.
bot bench substrate-pallet --pallet=pallet_contracts |
@Robbepop https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4994107 was started for your command Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Only thing is the regression in weight for the host functions because we (wrongly) been including the compilation time into their benchmarks.
So I think we need two PRs merged to master before progressing with this one:
- Removal of the double compilation in the other comment.
- Rework of the host function benchmarks to only measure execution time.
…=dev --target_dir=substrate --pallet=pallet_contracts
This comment was marked as outdated.
This comment was marked as outdated.
As mandated by code review. However, this shouldn't change any benchmark results since it is just setup code.
The CI pipeline was cancelled due to failure one of the required jobs. |
The test probably failed because of the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find out why those changes broke the test.
@@ -3078,7 +3078,7 @@ fn gas_estimation_call_runtime() { | |||
// contract encodes the result of the dispatch runtime | |||
let outcome = u32::decode(&mut result.result.unwrap().data.as_ref()).unwrap(); | |||
assert_eq!(outcome, 0); | |||
assert!(result.gas_required.ref_time() > result.gas_consumed.ref_time()); | |||
assert!(result.gas_required.ref_time() >= result.gas_consumed.ref_time()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is not incorrect. The comment a bit further up explains this. It is the point of the test to make sure that the pre-charging works. In this case when calling into the runtime. We use a function that has a much bigger max weight than actual weight to exercise that.
Why this broke from your changes is indeed strange.
Is that an issue ? We compute the schedule by doing We should probably look at the Schedule result instead |
In paritytech#2941 we found out that the new Wasmi (register) is very effective at optimizing away certain benchmark bytecode constructs in a way that created an unfair advantage over Wasmi (stack) which yielded our former benchmarks to be ineffective at properly measuring the performance impact. This PR adjusts both affected benchmarks to fix the stated problems. Affected are - `instr_i64const` -> `instr_i64add`: Renamed since it now measures the performance impact of the Wasm `i64.add` instruction with locals as inputs and outputs. This makes it impossible for Wasmi (register) to aggressively optimize away the entire function body (as it previously did) but still provides a way for Wasmi (register) to shine with its register based execution model. - `call_with_code_per_byte`: Now uses `local.get` instead of `i32.const` for the `if` condition which prevents Wasmi (register) to aggressively optimizing away whole parts of the `if` creating an unfair advantage. cc @athei --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
take over #2941 [Weights compare](https://weights.tasty.limo/compare?unit=weight&ignore_errors=true&threshold=10&method=asymptotic&repo=polkadot-sdk&old=master&new=pg%2Fwasmi-to-v0.32.0-beta.7&path_pattern=substrate%2Fframe%2F**%2Fsrc%2Fweights.rs%2Cpolkadot%2Fruntime%2F*%2Fsrc%2Fweights%2F**%2F*.rs%2Cpolkadot%2Fbridges%2Fmodules%2F*%2Fsrc%2Fweights.rs%2Ccumulus%2F**%2Fweights%2F*.rs%2Ccumulus%2F**%2Fweights%2Fxcm%2F*.rs%2Ccumulus%2F**%2Fsrc%2Fweights.rs) --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
take over paritytech#2941 [Weights compare](https://weights.tasty.limo/compare?unit=weight&ignore_errors=true&threshold=10&method=asymptotic&repo=polkadot-sdk&old=master&new=pg%2Fwasmi-to-v0.32.0-beta.7&path_pattern=substrate%2Fframe%2F**%2Fsrc%2Fweights.rs%2Cpolkadot%2Fruntime%2F*%2Fsrc%2Fweights%2F**%2F*.rs%2Cpolkadot%2Fbridges%2Fmodules%2F*%2Fsrc%2Fweights.rs%2Ccumulus%2F**%2Fweights%2F*.rs%2Ccumulus%2F**%2Fweights%2Fxcm%2F*.rs%2Ccumulus%2F**%2Fsrc%2Fweights.rs) --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
take over paritytech#2941 [Weights compare](https://weights.tasty.limo/compare?unit=weight&ignore_errors=true&threshold=10&method=asymptotic&repo=polkadot-sdk&old=master&new=pg%2Fwasmi-to-v0.32.0-beta.7&path_pattern=substrate%2Fframe%2F**%2Fsrc%2Fweights.rs%2Cpolkadot%2Fruntime%2F*%2Fsrc%2Fweights%2F**%2F*.rs%2Cpolkadot%2Fbridges%2Fmodules%2F*%2Fsrc%2Fweights.rs%2Ccumulus%2F**%2Fweights%2F*.rs%2Ccumulus%2F**%2Fweights%2Fxcm%2F*.rs%2Ccumulus%2F**%2Fsrc%2Fweights.rs) --------- Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
This updates
pallet-contracts
to use the new experimental Wasmiv0.32.0-beta.5
with the register-machine execution model.Subsequently this also removes usage of the deprecated
FuelConsumptionMode
inpallet-contracts
.Please note that Wasmi
v0.32.0-beta.5
is not yet production ready!The most significant change was the handling of
wasmi::Error
since Wasmi error types have been refactored to no longer usewasmi::core::Trap
and instead use a model more similar to Wasmtime with a unifiedError
type that entails everything.cc @athei
Checked the following:
cargo check -p pallet-contracts
workscargo test -p pallet-contracts
works