-
Notifications
You must be signed in to change notification settings - Fork 50
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 to wasmi v0.32 and add lazy validation #1488
Conversation
It is a bit weird that Wasmi Good though that lazy compilation actually improves the situation. |
@tomaka FYI: I just uploaded Any explanation on your side for the severe slowdowns? Proper optimizations and optimized Wasm used? |
What I measured is wasmi that has itself been compiled to .wasm and being executed by NodeJS. |
I see that Wasmi |
Ok I wasn't actually optimizing the build (I'm a bit confused because the compilation options go through a couple of layers). |
No problem! Looking forward to the new numbers. :) |
Okay here we go. I didn't actually wait an hour: wasmi 0.31: 293.095ms wasmi 0.32: 334.245ms wasmi 0.32 with lazy compilation: 63.022ms I've also uploaded the runtime (decompressed) in hexadecimal (because of GitHub uploads limitations), if you want to try it yourself: |
Thanks for the quick update! The new numbers (especially eager) look way more realistic now. |
This is off-topic, but I don't think so. Each section in a wasm file starts with its length, so we can easily iterate between the (not so many) sections until we find the ones we want. And then the content of the sections (what I'm parsing) are somewhere around 100 to 200 bytes. |
Ah okay, 100-200 bytes indeed shouldn't make a huge difference. |
@Robbepop Interestingly, one specific test seems to fail with I have many other tests in the repo that compile and run Wasm blobs, and these pass successfully. That specific failing test uses an old Polkadot runtime, so it's not impossible that the runtime was miscompiled or something. You can find the failing Wasm here: https://github.com/smol-dot/smoldot/blob/main/lib/src/executor/vm/test-polkadot-runtime-v9160.wasm |
Ah that's very interesting. I know what is happening there. Due to some optimizations the new Wasmi uses 16-bit encoded branching offsets whereas old Wasmi used 32-bit branching offsets. I would have never assumed that this was going to be problematic because it means that there are jumps over more than 32k Wasmi instructions. Note that a single Wasmi can make up to 4 Wasm instructions. Probably a result of very aggressive inlining. This is a current limitation of the new Wasmi bytecode, however, if necessary I know how to deal with it. |
Since a valid wasm bytecode fails to compile, it's kind of necessary, no? |
Every Wasm runtime has certain limitations and thus every wasm runtime may encounter valid Wasm files that fail to compile. So it is more of a question where to draw the line. |
That's very surprising to me, especially after the discussion about lazy validation. I understand that there are issues such as the maximum size of the stack, but I was convinced that validating WebAssembly would deterministically either pass or fail. It's like the WebAssembly committee/people care about problematic situations that are easy to solve but not the ones that are hard to solve. The Wasm runtime used in the failing test is one that was used on the Polkadot chain. It's not, say, a weird experiment. It's legitimate Rust code that was compiled with the Rust compiler. While the newer Polkadot runtime seems to work, the chances that a future runtime no longer compiles with wasmi seem relatively high to me. |
It is all about runtime limitations. For practical reasons those limitations should be chosen to allow for all practical use cases. Wasmtime for example has a limitation on function parameters, so even though a Wasm function with 2k parameters is valid it would fail to compile on Wasmtime. However, practical Wasm blobs dont make use of so many parameters anyways. I was under the impression that 16-bit branch offsets should be enough for all practical use case but it seems I was wrong so I agree that this needs to be fixed. |
All tests are passing, apart from the ones that fail because git dependencies are forbidden. |
That's amazing! Thanks a lot for the testing and updates. |
@tomaka Just release |
@tomaka I see that you just merged the PR to use the new beta Wasmi. Is that a good idea? It isn't officially production ready, yet. If this is not a big deal for Smoldot then it is obviously great for the new Wasmi version to get battle tested. |
All the runtimes that I've tried work, and given that changes in the runtime are usually pretty small, the chances that it breaks in the future are not very high. |
@tomaka Okay great, thanks for the elaboration. I am happy to see that Smoldot is a perfect testing ground for the new Wasmi version. :) |
This reverts commit 3ff5453.
This reverts commit 3ff5453.
…ot#1488)" (smol-dot#1527)" This reverts commit 961c920.
cc #864
This PR shouldn't be merged as-is. The compilation mode should not always be
Lazy
. Instead, we should add a newExecHint
.However I've done this change in order to measure how much faster it is.
To do so, I've measured three times how long it takes to compile the Westend runtime.
"Compile the Westend runtime" includes decoding the zstd-compressed runtime, parsing some custom Wasm sections, creating a
Module
(with wasmi), and creating an instance (with wasmi) but not executing it. So the times below are not just wasmi, but should still mainly be wasmi.With wasmi v0.31:
264.161ms
195.032ms
210.432ms
With wasmi master branch (commit 86d097623592dc499852ea4bb01cace0097d70a4):
702.528ms
661.07ms
684.553ms
With wasmi master branch (commit 86d097623592dc499852ea4bb01cace0097d70a4) with compilation mode set to
Lazy
(this PR)83.617ms
84.843ms
85.62ms
cc @Robbepop