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

refactor: improve call wasm #508

Merged
merged 14 commits into from
Oct 31, 2022

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Oct 12, 2022

  • Make the logic more clean and clear.
    - check recursion_limit by >=

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Oct 12, 2022

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.27ms 1.27ms ⚪ 0.43% 1.12ms 1.09ms 🟢 -2.98% 🟢 -14%
execute/
bare_call_0/typed
698.58µs 705.30µs ⚪ 0.90% 547.87µs 551.10µs ⚪ 0.60% 🟢 -22%
execute/
bare_call_1
1.40ms 1.40ms 🟢 -0.82% 1.37ms 1.35ms 🟢 -1.58% 🟢 -4%
execute/
bare_call_16
3.13ms 2.90ms 🟢 -7.07% 4.99ms 5.00ms 🟢 0.32% 🟡 72%
execute/
bare_call_16/typed
1.32ms 1.29ms 🟢 -2.45% 2.34ms 2.28ms 🟢 -2.44% 🟡 77%
execute/
bare_call_1/typed
800.90µs 799.92µs ⚪ -0.08% 874.15µs 865.19µs ⚪ -0.96% 🟢 8%
execute/
bare_call_4
1.76ms 1.78ms 🔴 1.39% 2.14ms 2.16ms 🟢 1.12% 🟢 21%
execute/
bare_call_4/typed
891.43µs 886.81µs ⚪ -0.47% 1.00ms 984.30µs 🟢 -1.56% 🟢 11%
execute/
br_table
983.74µs 908.23µs 🟢 -7.64% 1.14ms 1.10ms 🟢 -4.01% 🟢 21%
execute/
count_until
903.65µs 903.43µs ⚪ 0.05% 2.40ms 2.21ms 🟢 -7.87% 🔴 144%
execute/
factorial_iterative
365.06µs 376.29µs 🔴 2.97% 946.61µs 901.84µs 🟢 -4.67% 🔴 140%
execute/
factorial_recursive
730.68µs 711.02µs 🟢 -2.72% 1.58ms 1.45ms 🟢 -8.78% 🔴 103%
execute/
fib_iterative
1.79ms 1.79ms ⚪ 0.03% 5.00ms 4.90ms 🟢 -1.89% 🔴 174%
execute/
fib_recursive
7.05ms 6.75ms 🟢 -4.33% 14.29ms 13.68ms 🟢 -4.27% 🔴 103%
execute/
global_bump
1.36ms 1.38ms ⚪ 1.05% 3.65ms 3.35ms 🟢 -8.26% 🔴 143%
execute/
global_const
999.89µs 999.49µs ⚪ -0.02% 2.83ms 2.52ms 🟢 -11.15% 🔴 152%
execute/
host_calls
32.63µs 32.39µs ⚪ -0.77% 44.69µs 44.82µs ⚪ 0.48% 🟢 38%
execute/
memory_fill
1.55ms 1.55ms ⚪ 0.09% 4.20ms 4.07ms 🟢 -3.00% 🔴 162%
execute/
memory_sum
1.55ms 1.55ms ⚪ 0.10% 4.18ms 4.05ms 🟢 -3.10% 🔴 161%
execute/
memory_vec_add
3.11ms 3.05ms 🟢 -1.86% 8.75ms 8.02ms 🟢 -8.39% 🔴 163%
execute/
recursive_is_even
1.29ms 1.29ms ⚪ 0.15% 2.63ms 2.50ms 🟢 -5.15% 🟡 93%
execute/
recursive_ok
170.56µs 167.63µs 🟢 -1.72% 377.40µs 359.20µs 🟢 -4.95% 🔴 114%
execute/
recursive_scan
213.76µs 209.21µs 🟢 -2.13% 466.29µs 451.00µs 🟢 -3.26% 🔴 116%
execute/
recursive_trap
16.93µs 16.26µs 🟢 -3.98% 36.09µs 34.52µs 🟢 -4.36% 🔴 112%
execute/
regex_redux
669.97µs 662.32µs 🟢 -1.24% 1.60ms 1.45ms 🟢 -9.38% 🔴 118%
execute/
rev_complement
590.19µs 595.15µs ⚪ 0.94% 1.56ms 1.43ms 🟢 -8.18% 🔴 141%
execute/
tiny_keccak
450.54µs 482.95µs 🔴 7.28% 1.39ms 1.29ms 🟢 -7.12% 🔴 167%
execute/
trunc_f2i
1.08ms 1.01ms 🟢 -6.67% 2.66ms 2.44ms 🟢 -8.11% 🔴 142%
instantiate/
wasm_kernel
67.57µs 70.66µs ⚪ 4.00% 91.92µs 94.48µs 🔴 2.72% 🟢 34%
translate/
wasm_kernel
4.19ms 4.22ms ⚪ 0.74% 7.74ms 7.88ms 🔴 1.80% 🟡 87%

Link to pipeline

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.

Please revert >= change, rest of the PR looks fine to me. :)

crates/wasmi/src/engine/stack/frames.rs Outdated Show resolved Hide resolved
@Robbepop Robbepop changed the title chore: improve call wasm refactor: improve call wasm Oct 13, 2022
@Robbepop
Copy link
Member

I checked out your PR locally after seeing some regressions according to our benchmarking CI and unfortunately I can confirm locally that some benchmarks are slightly regressed with those changes.
I see that they are some kind of a clean up but I am hesistant merging a change if it introduces regressions that I do not understand.

@yjhmelody
Copy link
Contributor Author

@Robbepop I do not see regressions in my local

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #508 (ec90097) into master (0d087e2) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   79.73%   79.73%   -0.01%     
==========================================
  Files          75       75              
  Lines        6292     6291       -1     
==========================================
- Hits         5017     5016       -1     
  Misses       1275     1275              
Impacted Files Coverage Δ
crates/wasmi/src/engine/mod.rs 83.52% <100.00%> (ø)
crates/wasmi/src/engine/stack/frames.rs 92.00% <100.00%> (-0.60%) ⬇️
crates/wasmi/src/engine/stack/mod.rs 79.71% <100.00%> (+0.29%) ⬆️

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

@yjhmelody
Copy link
Contributor Author

:( execute/memory_vec_add meet a lot regressions. Why?

@Robbepop
Copy link
Member

:( execute/memory_vec_add meet a lot regressions. Why?

I don't know. Could be an artifact. Unfortunately wasmi these days is extremely sensible to performance regressions.

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Oct 18, 2022

Hi, seems now it at least not make regressions. @Robbepop

@yjhmelody
Copy link
Contributor Author

Why is red when improved? Such as 🔴 -3.70% 🔴 -0.35%

@yjhmelody
Copy link
Contributor Author

Strange, wasm_kernel is not stable, I see a big improvement last time.

@yjhmelody
Copy link
Contributor Author

@Robbepop It now seems is a good improvement that could be merged now.

@Robbepop
Copy link
Member

Robbepop commented Oct 31, 2022

@Robbepop It now seems is a good improvement that could be merged now.

The problem with those benchmarks is that they are currently extremely flaky/noisy for this PR in particular. I don't know why that is but we cannot really trust those benchmarks fully with respect to this PR. I will take a look today on it and decide if we want to merge it.
Generally I think the code of the PR is an improvement, however, I can remember that I programmed the structure so far in this way due to performance regressions back then. However, code has changed a lot since then and FuncFrame also became much smaller and therefore moving a FuncFrame around is no longer that bad.

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.

I checked out benchmarks locally for this PR and it now seems to be fine. LGTM therefore.

@Robbepop Robbepop merged commit f5e1c46 into wasmi-labs:master Oct 31, 2022
@yjhmelody yjhmelody deleted the chore-improve-call-wasm branch October 31, 2022 10:01
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.

4 participants