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

Add support for WASI based on Wasmtime's implementation #557

Merged
merged 8 commits into from
Nov 12, 2022

Conversation

OLUWAMUYIWA
Copy link
Contributor

@OLUWAMUYIWA OLUWAMUYIWA commented Nov 4, 2022

Closes #358

@OLUWAMUYIWA
Copy link
Contributor Author

Hi @Robbepop .This basically depends on wasmtime's implementation of the WASI, meaning I didn't have to do much. Much of what I did is to integrate it with wasmi's Linker, and your work has made that very easy. Please check and request changes. I'm happy to see it through.

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Nov 4, 2022

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.01ms 1.01ms 🟢 -0.03% 910.91µs 881.10µs 🟢 -3.24% 🟢 -13%
execute/
bare_call_0/typed
526.31µs 514.38µs 🟢 -2.21% 409.66µs 398.48µs 🟢 -2.65% 🟢 -23%
execute/
bare_call_1
1.06ms 1.05ms 🟢 -0.89% 1.08ms 1.08ms 🔴 0.01% 🟢 3%
execute/
bare_call_16
2.25ms 2.16ms 🟢 -4.53% 4.75ms 4.76ms 🟢 0.22% 🔴 120%
execute/
bare_call_16/typed
1.48ms 1.48ms ⚪ -0.71% 2.11ms 2.02ms 🟢 -4.51% 🟢 36%
execute/
bare_call_1/typed
609.11µs 611.42µs ⚪ 0.24% 686.88µs 726.24µs 🔴 5.54% 🟢 19%
execute/
bare_call_4
1.18ms 1.19ms ⚪ 0.28% 1.71ms 1.74ms ⚪ 1.21% 🟢 46%
execute/
bare_call_4/typed
626.42µs 626.41µs ⚪ 0.06% 814.07µs 815.16µs ⚪ 0.20% 🟢 30%
execute/
br_table
627.89µs 635.60µs ⚪ 1.48% 883.09µs 892.11µs ⚪ 1.10% 🟢 40%
execute/
count_until
697.15µs 651.91µs 🟢 -6.50% 2.00ms 2.06ms 🔴 2.60% 🔴 216%
execute/
factorial_iterative
307.83µs 304.73µs 🟢 -1.19% 844.39µs 845.50µs ⚪ -0.24% 🔴 177%
execute/
factorial_recursive
616.12µs 608.20µs 🟢 -1.33% 1.26ms 1.26ms ⚪ 0.24% 🔴 107%
execute/
fib_iterative
1.45ms 1.43ms 🟢 -1.19% 4.49ms 4.46ms ⚪ -0.56% 🔴 212%
execute/
fib_recursive
5.74ms 5.92ms 🔴 3.26% 11.39ms 11.30ms ⚪ -0.75% 🟡 91%
execute/
global_bump
950.25µs 950.23µs ⚪ -0.20% 3.11ms 3.16ms 🔴 1.59% 🔴 232%
execute/
global_const
717.68µs 716.46µs ⚪ -0.04% 2.32ms 2.32ms ⚪ -0.02% 🔴 224%
execute/
host_calls
30.13µs 28.97µs 🟢 -3.80% 37.46µs 38.98µs 🔴 3.98% 🟢 35%
execute/
memory_fill
1.28ms 1.28ms ⚪ -0.07% 3.90ms 3.98ms 🔴 1.87% 🔴 210%
execute/
memory_sum
1.81ms 1.25ms 🟢 -22.99% 3.93ms 3.95ms ⚪ 0.57% 🔴 216%
execute/
memory_vec_add
2.60ms 2.59ms ⚪ -0.15% 8.30ms 8.21ms ⚪ -1.14% 🔴 217%
execute/
recursive_is_even
1.13ms 1.11ms ⚪ -1.05% 2.10ms 2.10ms ⚪ -0.21% 🟡 89%
execute/
recursive_ok
148.57µs 146.23µs 🟢 -1.50% 302.09µs 293.55µs 🟢 -2.84% 🔴 101%
execute/
recursive_scan
182.07µs 179.50µs 🟢 -1.45% 371.56µs 375.10µs ⚪ 0.81% 🔴 109%
execute/
recursive_trap
14.76µs 14.44µs 🟢 -2.34% 29.60µs 28.61µs 🟢 -3.03% 🟡 98%
execute/
regex_redux
545.58µs 545.64µs ⚪ 0.05% 1.40ms 1.45ms 🔴 3.66% 🔴 166%
execute/
rev_complement
506.71µs 505.62µs ⚪ -0.26% 1.39ms 1.44ms 🔴 3.25% 🔴 184%
execute/
tiny_keccak
374.59µs 372.65µs ⚪ -0.72% 1.16ms 1.23ms 🔴 6.54% 🔴 230%
execute/
trunc_f2i
911.67µs 912.72µs ⚪ 0.07% 2.38ms 2.43ms 🔴 1.98% 🔴 166%
instantiate/
wasm_kernel
58.88µs 60.19µs 🔴 4.33% 95.16µs 67.67µs 🟢 -28.62% 🟢 12%
translate/
erc1155
209.52µs 207.67µs ⚪ -0.54% 375.61µs 377.63µs ⚪ 0.74% 🟡 82%
translate/
erc20
101.83µs 101.37µs ⚪ -0.49% 182.57µs 185.97µs 🔴 1.80% 🟡 83%
translate/
erc721
146.60µs 146.84µs ⚪ 0.02% 266.37µs 268.89µs ⚪ 0.91% 🟡 83%
translate/
spidermonkey
0.00ns 0.00ns ⚪ 0.47% 0.00ns 0.00ns ⚪ 0.91% 🟢 0%
translate/
wasm_kernel
3.79ms 3.77ms ⚪ -0.51% 7.17ms 7.19ms ⚪ 0.32% 🟡 91%

Link to pipeline

@Robbepop
Copy link
Member

Robbepop commented Nov 4, 2022

@OLUWAMUYIWA from a design perspective this PR looks as I expected it on a surface level which is good.
I have not taken a deep look but the general structure looks decent. I am a bit worried about those extreme regressions since I could not find a single piece of diff in this PR that would justify those regressions.

Could you please update to the most recent master branch before I do the proper review?

crates/wasi/Cargo.toml Outdated Show resolved Hide resolved
crates/core/src/host_error.rs Outdated Show resolved Hide resolved
crates/wasi/src/lib.rs Outdated Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Outdated Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Outdated Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Outdated Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Show resolved Hide resolved
@OLUWAMUYIWA
Copy link
Contributor Author

OLUWAMUYIWA commented Nov 6, 2022

Thanks @Robbepop and @athei .
Much has been discussed by @Robbepop and I in chats. And I think i have a fairly good understanding of what he expects.

But first, I suppose I need to create another PR addressing #517 . Namely, Traps.

@Robbepop

This comment was marked as resolved.

crates/wasi/Cargo.toml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #557 (6b999ac) into master (53eac5c) will increase coverage by 0.34%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   79.73%   80.07%   +0.34%     
==========================================
  Files          75       77       +2     
  Lines        6251     6304      +53     
==========================================
+ Hits         4984     5048      +64     
+ Misses       1267     1256      -11     
Impacted Files Coverage Δ
crates/wasi/src/snapshots/preview_1.rs 81.25% <81.25%> (ø)
crates/wasi/tests/wasi_wat.rs 100.00% <100.00%> (ø)
crates/core/src/trap.rs 41.50% <0.00%> (-0.80%) ⬇️
crates/wasmi/src/memory/mod.rs 62.69% <0.00%> (+2.38%) ⬆️
crates/wasmi/src/func/into_func.rs 93.18% <0.00%> (+4.54%) ⬆️
crates/wasmi/src/store.rs 74.04% <0.00%> (+4.58%) ⬆️
crates/wasmi/src/func/caller.rs 52.94% <0.00%> (+41.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

crates/wasi/Cargo.toml Outdated Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Show resolved Hide resolved
crates/wasi/src/snapshots/preview_1.rs Show resolved Hide resolved
crates/wasi/Cargo.toml Show resolved Hide resolved
Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fix the no_std CI failures we also need to add --exclude wasmi_wasi to GitHub Action's rust.yml:

      - name: Build wasmi itself as no_std
        uses: actions-rs/cargo@v1
        with:
          command: build
          args: --workspace --lib --no-default-features --target thumbv7em-none-eabi --exclude wasmi_cli

@Robbepop Robbepop changed the title implemented wasm-wasi based on wasmtimes impl Implement wasm-wasi based on Wasmtime's implementation Nov 12, 2022
@Robbepop Robbepop changed the title Implement wasm-wasi based on Wasmtime's implementation Add support for WASI based on Wasmtime's implementation Nov 12, 2022
Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks a ton for all the hard work to get partial WASI support for wasmi @OLUWAMUYIWA !

Let us merge this. 🥳

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.

Implement support for running wasm32-wasi compiled Wasm binaries
5 participants