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

WIP: Clean up wasmi crate structure #647

Merged
merged 11 commits into from
Feb 3, 2023
Merged

WIP: Clean up wasmi crate structure #647

merged 11 commits into from
Feb 3, 2023

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Feb 3, 2023

This PR will contain misc refactorings and cleanup throughout the entire wasmi crate.

RefNull now stores the ValueType information directly so it has no longer be fed extenally.
I cannot remember why we dropped this information in the first place - probably an oversight.
Now has the same name as Table::dynamic_ty method. This was probably an oversight.
@Robbepop Robbepop changed the title WIP: Refactor wasmi crate structure WIP: Clean up wasmi crate structure Feb 3, 2023
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Feb 3, 2023

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.41ms 1.40ms ⚪ -0.79% 1.20ms 1.16ms 🔴 -3.69% 🟢 -17%
execute/
bare_call_0/typed
1.02ms 1.02ms ⚪ -0.53% 777.65µs 813.70µs 🔴 4.49% 🟢 -20%
execute/
bare_call_1
1.44ms 1.43ms 🔴 -0.64% 1.41ms 1.48ms 🔴 4.84% 🟢 3%
execute/
bare_call_16
2.41ms 2.42ms 🔴 0.40% 4.47ms 4.39ms 🟢 -1.56% 🟡 82%
execute/
bare_call_16/typed
1.76ms 1.84ms 🔴 4.90% 2.79ms 2.76ms ⚪ -0.84% 🟢 50%
execute/
bare_call_1/typed
1.14ms 1.14ms ⚪ 0.08% 1.21ms 1.21ms ⚪ -0.34% 🟢 6%
execute/
bare_call_4
1.63ms 1.60ms ⚪ -2.29% 2.08ms 2.05ms 🔴 -1.99% 🟢 28%
execute/
bare_call_4/typed
1.15ms 1.14ms ⚪ -1.03% 1.32ms 1.34ms 🔴 1.56% 🟢 18%
execute/
br_table
1.24ms 1.71ms 🔴 36.30% 1.37ms 1.49ms 🔴 8.20% 🟢 -13%
execute/
count_until
682.75µs 653.20µs 🟢 -4.43% 2.19ms 2.29ms 🔴 4.61% 🔴 251%
execute/
factorial_iterative
334.70µs 325.66µs 🟢 -2.55% 947.67µs 953.11µs ⚪ 0.61% 🔴 193%
execute/
factorial_recursive
663.99µs 668.72µs ⚪ 0.66% 1.35ms 1.41ms 🔴 4.38% 🔴 110%
execute/
fib_iterative
1.40ms 1.49ms 🔴 6.20% 5.14ms 5.00ms 🟢 -2.69% 🔴 236%
execute/
fib_recursive
6.03ms 5.97ms ⚪ -1.15% 11.89ms 12.54ms 🔴 5.44% 🔴 110%
execute/
global_bump
987.27µs 978.07µs ⚪ -1.05% 2.45ms 2.68ms 🔴 9.34% 🔴 174%
execute/
global_const
742.96µs 726.14µs 🟢 -2.18% 2.56ms 2.46ms 🟢 -3.91% 🔴 238%
execute/
host_calls
26.29µs 27.36µs 🔴 4.22% 40.10µs 39.96µs ⚪ -0.20% 🟢 46%
execute/
memory_fill
1.26ms 1.31ms 🔴 4.25% 4.14ms 4.17ms ⚪ 0.78% 🔴 219%
execute/
memory_sum
1.19ms 1.30ms 🔴 9.41% 4.15ms 4.15ms ⚪ -0.12% 🔴 218%
execute/
memory_vec_add
2.60ms 2.47ms 🟢 -5.06% 7.59ms 7.81ms 🔴 2.95% 🔴 216%
execute/
recursive_is_even
1.16ms 1.15ms ⚪ -1.21% 2.32ms 2.33ms ⚪ 0.35% 🔴 103%
execute/
recursive_ok
166.55µs 158.84µs 🟢 -4.54% 304.28µs 324.64µs 🔴 6.60% 🔴 104%
execute/
recursive_scan
193.86µs 183.65µs 🟢 -5.00% 366.99µs 406.51µs 🔴 10.60% 🔴 121%
execute/
recursive_trap
15.77µs 15.96µs ⚪ 0.97% 29.27µs 32.37µs 🔴 10.55% 🔴 103%
execute/
regex_redux
526.38µs 527.98µs ⚪ 0.29% 1.39ms 1.42ms 🔴 2.45% 🔴 169%
execute/
rev_complement
487.60µs 500.60µs 🔴 2.72% 1.43ms 1.44ms ⚪ 0.97% 🔴 188%
execute/
tiny_keccak
333.98µs 352.64µs 🔴 5.62% 1.15ms 1.16ms ⚪ 0.41% 🔴 228%
execute/
trunc_f2i
734.54µs 709.61µs 🟢 -3.45% 2.13ms 2.20ms 🔴 3.00% 🔴 209%
instantiate/
wasm_kernel
64.44µs 64.23µs ⚪ -1.24% 69.11µs 71.30µs 🔴 2.62% 🟢 11%
translate/
erc1155
208.89µs 210.50µs ⚪ 0.94% 387.44µs 387.11µs ⚪ -0.13% 🟡 84%
translate/
erc20
102.83µs 104.66µs 🔴 1.79% 190.76µs 189.39µs ⚪ -0.69% 🟡 81%
translate/
erc721
146.76µs 147.91µs ⚪ 0.71% 278.36µs 276.79µs ⚪ -0.70% 🟡 87%
translate/
spidermonkey
0.00ns 0.00ns ⚪ -0.14% 0.00ns 0.00ns ⚪ 1.13% 🟢 0%
translate/
wasm_kernel
3.76ms 3.80ms ⚪ 1.20% 7.22ms 7.20ms ⚪ -0.37% 🟡 89%

Link to pipeline

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #647 (30c23d2) into master (9652bea) will increase coverage by 0.12%.
The diff coverage is 68.62%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage   81.16%   81.28%   +0.12%     
==========================================
  Files          87       87              
  Lines        7729     7707      -22     
==========================================
- Hits         6273     6265       -8     
+ Misses       1456     1442      -14     
Impacted Files Coverage Δ
crates/wasmi/src/func/mod.rs 78.15% <ø> (ø)
crates/wasmi/src/module/builder.rs 99.13% <ø> (-0.04%) ⬇️
crates/wasmi/src/module/instantiate/mod.rs 76.43% <40.00%> (+1.74%) ⬆️
crates/wasmi/src/module/parser.rs 73.29% <55.55%> (-1.57%) ⬇️
crates/wasmi/src/module/init_expr.rs 75.80% <70.00%> (+13.46%) ⬆️
crates/wasmi/src/engine/func_builder/translator.rs 88.72% <100.00%> (-0.02%) ⬇️
crates/wasmi/src/func/func_type.rs 93.84% <100.00%> (ø)
crates/wasmi/src/linker.rs 57.83% <100.00%> (ø)
crates/wasmi/src/memory/mod.rs 64.38% <100.00%> (ø)
crates/wasmi/src/module/global.rs 88.88% <100.00%> (ø)
... and 3 more

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

Having those fields pub(super) isn't necessary for types that are already private.
Now all module index type implement From<u32> and have an into_u32 method. Also their internal index is no longer visible within the rest of the crate.
@Robbepop Robbepop marked this pull request as ready for review February 3, 2023 18:45
@Robbepop Robbepop merged commit 6ad9ea6 into master Feb 3, 2023
@Robbepop Robbepop deleted the rf-refactor-crate branch February 3, 2023 18:47
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.

3 participants