contracts: Upgrade to wasmi 0.28 #13312
Conversation
athei
added
A0-please_review
|
Since this is enabling new optimizations in wasm-opt ideally I'd like to run a full burn-in before we consider merging this. Basically hack the WASM executor so that it dynamically runs wasm-opt on every runtime upgrade (so essentially apply it retroactively to every existing on-chain runtime) and see if everything successfully syncs. I'll set it up on my GCP node. |
|
Burn-in is running; will probably take a few days to finish: |
@koute Can you elaborate on why an update to |
I'm not entirely sure I understand the question. (: Here's the patch from this PR which changes |
|
I didn't notice that this PR was also updating |
Well, the last time we tried there was visible impact on performance when using |
|
I just added it to this PR because I suspected the degradation was from not using wasm-opt (wasmi is sensitive to that). But it was about the benchmarking machine I guess. Thanks for doing the burn in. If everything goes well I propose that we will remove the change from this PR and put it into a standalone PR. |
|
I removed the |
Sounds good to me. Current progress: |
|
@athei The burn-in finished successfully. |
|
In the meantime I released version |
|
Yeah will do that. There is a queue of PRs that need to go in before as soon as metering is fixed, though. |
athei
changed the title
|
Performance uplift without |
| if matches!(determinism, Determinism::Deterministic) { | ||
| contract_module.ensure_no_floating_types()?; | ||
| } |
There was a problem hiding this comment.
We no longer need this because the newest wasmparser rejects floating types for us if floats: false. Before it only rejected float instructions.
There was a problem hiding this comment.
wasmparser rejects floating types for us if floats: false
just curious, where do you define this setting?
There was a problem hiding this comment.
Just a bit above. Argument to Validator::new_with_features.
|
|
| if matches!(determinism, Determinism::Deterministic) { | ||
| contract_module.ensure_no_floating_types()?; | ||
| } |
There was a problem hiding this comment.
wasmparser rejects floating types for us if floats: false
just curious, where do you define this setting?
| @@ -1214,7 +1165,7 @@ mod tests { | |||
| (func (export "deploy")) | |||
| ) | |||
| "#, | |||
| Err("use of floating point type in globals is forbidden") | |||
There was a problem hiding this comment.
don't we want to keep the more specific error message?
There was a problem hiding this comment.
We can't. The function returns a &'static str. We opted for logging the error message to the console instead of returning it. Not optimal. The error handling there is a mess. But the whole thing will become much simpler once we have #13639. Then we can have a hard look on the error handling there.
|
I am doing the update to wasmi 0.29 in a follow up PR. I don't want to re-run benchmarks. |
|
Okay the rename needed a cumulus companion. Please also approve the trivial companion: paritytech/cumulus#2350 |
|
bot merge |
* Upgrade to wasmi 0.28 * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts * Update stale comment * Renamed variants of `Determinism` * Compile fix --------- Co-authored-by: command-bot <>
* Upgrade to wasmi 0.28 * ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts * Update stale comment * Renamed variants of `Determinism` * Compile fix --------- Co-authored-by: command-bot <>
Also upgrading to newest
wasmparserto keep up withwasmi.Performance uplift for instructions execution looks pretty good from wasmi 0.20. As we expected.
Please note that this is without wasm-opt as applying this to the runtime didn't improve performance as a whole (#13408). It also did remove all the benefits of this new wasmi-version. Hence we just merge without wasm-opt.
This is how this PR performed with wasmt-opt vs. no wasm-opt. this is with
-O2but-O4performed the same:Cumulus companion: paritytech/cumulus#2350