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

Overhaul Pages and Bytes types #449

Merged
merged 17 commits into from
Oct 21, 2022
Merged

Overhaul Pages and Bytes types #449

merged 17 commits into from
Oct 21, 2022

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Sep 18, 2022

Problems with previous types were found during fuzzy testing.
These types make it simpler to fix linear memory allocation problems in wasmi for 32-bit and 64-bit architectures for edge cases.

Also we get rid of the memory_units dependency which did a lot more things that we actually needed in wasmi.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2022

Codecov Report

Merging #449 (889b301) into master (753b05d) will decrease coverage by 0.11%.
The diff coverage is 69.73%.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   79.44%   79.33%   -0.12%     
==========================================
  Files          71       72       +1     
  Lines        6193     6241      +48     
==========================================
+ Hits         4920     4951      +31     
- Misses       1273     1290      +17     
Impacted Files Coverage Δ
wasmi_v1/src/memory/byte_buffer.rs 71.42% <63.63%> (-7.52%) ⬇️
core/src/units.rs 66.66% <66.66%> (ø)
wasmi_v1/src/engine/executor.rs 98.26% <71.42%> (-0.15%) ⬇️
wasmi_v1/src/memory/mod.rs 59.34% <77.77%> (+0.87%) ⬆️
wasmi_v1/src/module/utils.rs 86.36% <100.00%> (ø)
wasmi_v1/tests/spec/context.rs 77.98% <100.00%> (ø)

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

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Sep 18, 2022

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.29ms 1.32ms 🔴 1.96% 1.12ms 1.10ms 🔴 -1.89% 🟢 -17%
execute/
bare_call_0/typed
703.72µs 754.52µs 🔴 7.16% 537.68µs 544.35µs 🔴 1.19% 🟢 -28%
execute/
bare_call_1
1.40ms 1.44ms 🔴 3.33% 1.38ms 1.36ms 🟢 -1.51% 🟢 -6%
execute/
bare_call_16
3.03ms 3.07ms ⚪ 2.39% 5.23ms 5.09ms 🟢 -2.53% 🟡 66%
execute/
bare_call_16/typed
1.30ms 1.29ms ⚪ -0.91% 2.53ms 2.37ms 🟢 -6.35% 🟡 84%
execute/
bare_call_1/typed
805.39µs 874.88µs 🔴 8.71% 868.49µs 868.98µs ⚪ -0.21% 🟢 -1%
execute/
bare_call_4
1.77ms 1.75ms 🔴 -0.78% 2.18ms 2.14ms 🟢 -2.02% 🟢 22%
execute/
bare_call_4/typed
889.30µs 916.20µs 🔴 2.98% 1.01ms 985.45µs 🟢 -2.22% 🟢 8%
execute/
br_table
907.59µs 945.51µs 🔴 4.13% 1.10ms 1.11ms ⚪ 0.99% 🟢 18%
execute/
count_until
886.66µs 901.67µs 🔴 1.73% 2.21ms 2.33ms 🔴 5.64% 🔴 159%
execute/
factorial_iterative
362.63µs 363.73µs ⚪ 0.35% 895.64µs 930.20µs 🔴 3.80% 🔴 156%
execute/
factorial_recursive
747.18µs 716.73µs 🟢 -3.98% 1.50ms 1.54ms 🔴 2.58% 🔴 115%
execute/
fib_iterative
1.79ms 1.79ms ⚪ -0.22% 4.92ms 5.47ms 🔴 11.38% 🔴 206%
execute/
fib_recursive
7.12ms 8.16ms 🔴 14.62% 13.59ms 14.17ms 🔴 4.25% 🟡 74%
execute/
global_bump
1.45ms 1.36ms 🟢 -5.57% 3.50ms 3.83ms 🔴 9.47% 🔴 181%
execute/
host_calls
33.06µs 34.74µs ⚪ 1.34% 45.26µs 45.49µs ⚪ 0.58% 🟢 31%
execute/
memory_fill
1.53ms 1.56ms 🔴 1.33% 4.23ms 4.24ms ⚪ 0.17% 🔴 172%
execute/
memory_sum
1.52ms 1.56ms 🔴 2.86% 4.17ms 4.25ms 🔴 1.93% 🔴 172%
execute/
memory_vec_add
4.76ms 3.06ms 🟢 -36.16% 8.22ms 8.67ms 🔴 5.35% 🔴 183%
execute/
recursive_is_even
1.31ms 1.32ms ⚪ 1.02% 2.55ms 2.59ms 🔴 1.87% 🟡 96%
execute/
recursive_ok
175.60µs 172.96µs 🟢 -1.43% 351.76µs 362.25µs 🔴 2.93% 🔴 109%
execute/
recursive_scan
219.90µs 215.17µs 🟢 -2.03% 454.72µs 468.96µs 🔴 3.31% 🔴 118%
execute/
recursive_trap
17.34µs 17.33µs ⚪ -0.16% 34.82µs 36.40µs 🔴 4.73% 🔴 110%
execute/
regex_redux
652.39µs 675.53µs 🔴 3.47% 1.57ms 1.60ms 🔴 2.30% 🔴 136%
execute/
rev_complement
591.18µs 596.97µs ⚪ 1.01% 1.52ms 1.54ms ⚪ 1.14% 🔴 158%
execute/
tiny_keccak
481.94µs 471.69µs ⚪ -1.83% 1.33ms 1.37ms 🔴 2.98% 🔴 191%
execute/
trunc_f2i
1.07ms 1.07ms ⚪ 0.55% 2.41ms 2.65ms 🔴 10.16% 🔴 147%
instantiate/
wasm_kernel
68.36µs 72.02µs 🔴 5.69% 107.28µs 120.26µs 🔴 12.44% 🟡 67%
translate/
wasm_kernel
4.11ms 4.16ms ⚪ 1.13% 7.83ms 7.89ms ⚪ 0.67% 🟡 90%

Link to pipeline


#[cfg(target_pointer_width = "64")]
const fn max_len() -> u64 {
u32::MAX as u64 + 1
Copy link
Contributor

@yjhmelody yjhmelody Oct 8, 2022

Choose a reason for hiding this comment

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

Sorry, I could not find this value in spec. Could you give me a ref?

@yjhmelody
Copy link
Contributor

Could you also consider to extract memoryEntity type as a new crate?

@Robbepop
Copy link
Member Author

Could you also consider to extract memoryEntity type as a new crate?

i do not see a reason for this. also what does this has to do with this PR?

@yjhmelody
Copy link
Contributor

Could you also consider to extract memoryEntity type as a new crate?

i do not see a reason for this. also what does this has to do with this PR?

I think it may be better to put Pages/Bytes into a wasmi_memory crate.

@Robbepop
Copy link
Member Author

Could you also consider to extract memoryEntity type as a new crate?

i do not see a reason for this. also what does this has to do with this PR?

I think it may be better to put Pages/Bytes into a wasmi_memory crate.

Yeah I got that but can you give me a compelling example as to why?

@yjhmelody
Copy link
Contributor

yjhmelody commented Oct 20, 2022

Yeah I got that but can you give me a compelling example as to why?

No examples, simply because I need this crate too :(

@Robbepop
Copy link
Member Author

Yeah I got that but can you give me a compelling example as to why?

No examples, simply because I need this crate too :(

Cannot you use wasmi_core instead?

@Robbepop Robbepop merged commit 3a405bd into master Oct 21, 2022
@Robbepop Robbepop deleted the rf-units branch October 21, 2022 11:37
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