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

Support for ResourceLimiter API #737

Merged
merged 23 commits into from
Jul 4, 2023
Merged

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Jun 29, 2023

Closes #728.

This implements the wasmtime ResourceLimiter API, as discussed in bug #728 .

Current limitations, caveats and points to discuss/review:

  • Missing tests
  • Missing docs
  • Missing convenience types StoreLimits and StoreLimitsBuilder
  • To keep #[derive(Debug)] working I had to add a couple wrapper types.
  • As much as possible, I tried to track wasmtime's implementation: the {instance,memory,table}_{count,limit} fields, the Store::bump_resource_counts method, the limits.rs file, the global defaults returned from the default methods, etc. etc. This all feels like a fairly weird design to me (information cached or returned from multiple places) but I figured you'd prefer to follow along their lead rather than make something gratuitously incompatible. Any deviations (see following points) were forced by differences between wasmtime and wasmi.
  • Awkwardly, because wasmi does not thread the full typed Store<T> into its executor context, only StoreInner, it's not possible to call the typed FnMut(&mut T) -> &mut dyn ResourceLimiter query API on StoreInner (it has no data: T element to pass as an argument) so I had to hoist the query up the call stack to caller sites that still have a Store<T> and then pass through a new ResourceLimiterRef type carrying the resulting ref. This seems to work but it's a bit ugly. The other options (refactoring Store and/or Executor) seemed worse.
  • Unfortunately wasmtime just uses anyhow::Error everywhere, so wasmi's specific error types all deviate. I did not do a lot of careful design work on the errors, I just reused those that already seemed to cover the cases and added new ones where necessary. They could theoretically be reorganized or refactored -- I don't care how they're represented at all, happy to do whatever you prefer.
  • The wasmtime ResourceLimiter API actually seems .. fairly bad? I mean, it operates in terms of bytes (weird) and has the option of both returning an Err(...)-which-means-trap (which I didn't even try to support) and returning Ok(false)-which-means-return-your-own-error. I am not sure how best to map its seemingly-bad-API-choices onto wasmi, I did my best.
  • It also doesn't cap the number of globals, imports or exports, or any other sources of "too much stuff" in a wasm module. No explanation why. I think it probably should?
  • Because ResourceLimiter wants to be called both before any inherent checks and after any error occurs (including those resulting from inherent checks) I had to refactor the arithmetic in the memory and table growth functions, and repeat some code between initialization and growth. I believe I kept the logic the same. I might be able to factor some of this more, but it seems like diminishing returns.

Anyway, all that aside I think this holds together. Feedback welcome!

@graydon graydon marked this pull request as draft June 29, 2023 06:50
@Robbepop Robbepop marked this pull request as ready for review June 29, 2023 07:23
@Robbepop Robbepop marked this pull request as draft June 29, 2023 07:23
@Robbepop

This comment was marked as outdated.

crates/wasmi/src/memory/mod.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/table/mod.rs Outdated Show resolved Hide resolved
Robbepop

This comment was marked as outdated.

@Robbepop Robbepop changed the title Sketch of support for ResourceLimiter API Sketch of support for ResourceLimiter API Jun 29, 2023
@graydon

This comment was marked as resolved.

@graydon

This comment was marked as resolved.

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Jun 30, 2023

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.55ms 1.56ms ⚪ 1.70% 993.55µs 1.02ms 🔴 0.47% 🟢 -35%
execute/
bare_call_0/typed
1.16ms 1.17ms ⚪ 0.49% 652.56µs 694.55µs 🔴 6.64% 🟢 -40%
execute/
bare_call_1
1.60ms 1.60ms ⚪ -0.22% 1.12ms 1.15ms 🔴 2.23% 🟢 -28%
execute/
bare_call_16
2.55ms 2.57ms ⚪ 0.77% 3.34ms 3.15ms 🟢 -5.66% 🟢 23%
execute/
bare_call_16/typed
1.54ms 1.55ms ⚪ 0.52% 1.76ms 1.49ms 🟢 -15.38% 🟢 -4%
execute/
bare_call_1/typed
1.23ms 1.24ms ⚪ 0.28% 906.75µs 941.82µs 🔴 3.91% 🟢 -24%
execute/
bare_call_4
1.76ms 2.18ms 🔴 50.71% 1.51ms 1.59ms 🔴 4.63% 🟢 -27%
execute/
bare_call_4/typed
1.19ms 1.22ms 🔴 2.63% 921.16µs 918.43µs ⚪ -0.44% 🟢 -25%
execute/
br_table
1.29ms 1.33ms 🔴 2.78% 1.09ms 1.18ms 🔴 8.13% 🟢 -11%
execute/
count_until
592.18µs 668.29µs 🔴 11.94% 1.30ms 1.13ms 🟢 -13.64% 🟡 69%
execute/
factorial_iterative
321.22µs 332.97µs 🔴 3.84% 519.11µs 441.90µs 🟢 -14.77% 🟢 33%
execute/
factorial_recursive
493.45µs 505.39µs 🔴 2.40% 755.22µs 628.27µs 🟢 -16.83% 🟢 24%
execute/
fibonacci_iter
1.39ms 1.42ms 🔴 1.61% 2.84ms 2.20ms 🟢 -22.30% 🟡 55%
execute/
fibonacci_rec
3.92ms 3.93ms ⚪ -0.01% 6.42ms 5.97ms 🟢 -6.22% 🟡 52%
execute/
fibonacci_tail
862.98µs 882.86µs 🔴 2.37% 1.45ms 1.31ms 🟢 -9.70% 🟢 48%
execute/
global_bump
745.26µs 753.48µs ⚪ 1.08% 1.69ms 1.51ms 🟢 -10.55% 🔴 101%
execute/
global_const
660.78µs 711.73µs 🔴 7.71% 1.36ms 1.19ms 🟢 -12.59% 🟡 67%
execute/
host_calls
36.48µs 36.65µs ⚪ 0.46% 39.58µs 38.31µs 🟢 -3.11% 🟢 5%
execute/
memory_fill
1.16ms 1.16ms ⚪ 0.08% 2.28ms 1.99ms 🟢 -12.56% 🟡 72%
execute/
memory_sum
1.14ms 1.14ms ⚪ -0.98% 2.31ms 1.93ms 🟢 -16.47% 🟡 70%
execute/
memory_vec_add
2.36ms 2.35ms ⚪ -0.11% 4.58ms 4.16ms 🟢 -9.46% 🟡 77%
execute/
recursive_is_even
689.62µs 689.72µs ⚪ 0.09% 1.05ms 950.22µs 🟢 -9.69% 🟢 38%
execute/
recursive_ok
95.84µs 95.77µs ⚪ 0.06% 158.17µs 138.28µs 🟢 -12.47% 🟢 44%
execute/
recursive_scan
131.58µs 130.84µs ⚪ -0.58% 202.38µs 182.10µs 🟢 -9.92% 🟢 39%
execute/
recursive_trap
9.03µs 9.05µs ⚪ 0.32% 15.73µs 13.03µs 🟢 -17.09% 🟢 44%
execute/
regex_redux
457.44µs 457.76µs ⚪ 0.09% 831.72µs 779.09µs 🟢 -6.48% 🟡 70%
execute/
rev_complement
423.11µs 419.96µs ⚪ -0.72% 811.30µs 732.94µs 🟢 -9.61% 🟡 75%
execute/
tiny_keccak
335.51µs 347.48µs 🔴 3.72% 681.53µs 561.27µs 🟢 -17.86% 🟡 62%
execute/
trunc_f2i
733.65µs 737.20µs ⚪ 0.51% 1.63ms 1.47ms 🟢 -9.64% 🟡 100%
instantiate/
wasm_kernel
55.51µs 54.21µs 🟢 -4.37% 60.08µs 55.32µs 🟢 -5.35% 🟢 2%
translate/
erc1155
196.88µs 198.20µs ⚪ 0.73% 334.20µs 333.01µs ⚪ -0.18% 🟡 68%
translate/
erc20
97.24µs 97.95µs ⚪ 0.67% 162.23µs 160.47µs ⚪ -0.60% 🟡 64%
translate/
erc721
137.15µs 140.50µs ⚪ 1.64% 232.71µs 231.94µs ⚪ -0.39% 🟡 65%
translate/
spidermonkey
62.07ms 62.10ms ⚪ -0.14% 0.00ns 0.00ns ⚪ -0.85% 🟢 -100%
translate/
wasm_kernel
4.07ms 4.08ms ⚪ 0.76% 6.15ms 6.11ms ⚪ -0.84% 🟢 49%

Link to pipeline

@Robbepop

This comment was marked as resolved.

crates/core/src/trap.rs Outdated Show resolved Hide resolved
crates/wasmi/src/engine/executor.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
@Robbepop

This comment was marked as resolved.

@graydon

This comment was marked as resolved.

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #737 (25b4e7a) into master (799995d) will increase coverage by 0.16%.
The diff coverage is 72.46%.

@@            Coverage Diff             @@
##           master     #737      +/-   ##
==========================================
+ Coverage   79.08%   79.25%   +0.16%     
==========================================
  Files         102      104       +2     
  Lines        8755     9046     +291     
==========================================
+ Hits         6924     7169     +245     
- Misses       1831     1877      +46     
Impacted Files Coverage Δ
crates/core/src/trap.rs 51.47% <0.00%> (-0.77%) ⬇️
crates/wasmi/src/func/typed_func.rs 58.97% <0.00%> (-1.56%) ⬇️
crates/wasmi/src/linker.rs 61.51% <ø> (+0.27%) ⬆️
crates/wasmi/src/memory/error.rs 21.42% <0.00%> (-3.58%) ⬇️
crates/wasmi/src/module/instantiate/error.rs 18.51% <0.00%> (+6.98%) ⬆️
crates/wasmi/src/table/error.rs 0.00% <0.00%> (ø)
crates/wasmi/src/engine/mod.rs 77.35% <50.00%> (-0.37%) ⬇️
crates/wasmi/src/table/mod.rs 62.50% <52.27%> (-2.16%) ⬇️
crates/wasmi/src/engine/executor.rs 80.21% <54.54%> (+1.66%) ⬆️
crates/wasmi/src/module/instantiate/mod.rs 79.74% <57.14%> (-0.26%) ⬇️
... and 4 more

... and 9 files with indirect coverage changes

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

crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
@graydon

This comment was marked as resolved.

@Robbepop

This comment was marked as resolved.

@graydon
Copy link
Contributor Author

graydon commented Jul 1, 2023

I tried something different. Just a hunch, but it seemed weird to me that it was doing all that spilling and reloading, when it should store the struct in registers -- one of LLVM's most important passes is "SROA" (scalar replacement of aggregates) where it "scalarizes" the individual fields of structs, breaking them into separate values -- which is how Executor's fields all wind up in registers. I hypothesized that the new reference was pushing the Executor over some kind of threshold that broke SROA. So I just moved the new reference out of the struct, as a separate argument to the function, and that seems to have helped a lot!

  1ec530:       41 8b 46 04             mov    0x4(%r14),%eax
  1ec534:       48 c1 e0 03             shl    $0x3,%rax
  1ec538:       48 f7 d8                neg    %rax
  1ec53b:       49 8b 04 04             mov    (%r12,%rax,1),%rax
  1ec53f:       49 89 04 24             mov    %rax,(%r12)
  1ec543:       49 83 c4 08             add    $0x8,%r12
  1ec547:       49 83 c6 08             add    $0x8,%r14
  1ec54b:       41 0f b6 06             movzbl (%r14),%eax
  1ec54f:       49 63 04 82             movslq (%r10,%rax,4),%rax
  1ec553:       4c 01 d0                add    %r10,%rax
  1ec556:       ff e0                   jmp    *%rax

I'll wait for the benchmark bot to run, but on my workstation it seems to solve the problem. We'll see!

@Robbepop

This comment was marked as resolved.

crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
@Robbepop

This comment was marked as resolved.

crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/table/error.rs Outdated Show resolved Hide resolved
@graydon
Copy link
Contributor Author

graydon commented Jul 3, 2023

I believe I've addressed all the remaining comments and CI failures and this is ready to go now.

@graydon graydon marked this pull request as ready for review July 3, 2023 20:23
@graydon graydon changed the title Sketch of support for ResourceLimiter API Support for ResourceLimiter API Jul 3, 2023
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Outdated Show resolved Hide resolved
crates/wasmi/src/store.rs Show resolved Hide resolved
crates/core/src/trap.rs Outdated Show resolved Hide resolved
crates/wasmi/src/limits.rs Show resolved Hide resolved
crates/wasmi/src/module/instantiate/mod.rs Show resolved Hide resolved
@Robbepop

This comment was marked as outdated.

@Robbepop

This comment was marked as resolved.

@graydon

This comment was marked as resolved.

@Robbepop

This comment was marked as resolved.

@Robbepop
Copy link
Member

Robbepop commented Jul 4, 2023

CI seems to be happy now so I guess we can merge this PR @graydon.
@graydon Great work and thanks a lot for the contribution. Very much appreciated!

@Robbepop Robbepop merged commit 4860ecc into wasmi-labs:master Jul 4, 2023
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.

Add ResourceLimiter to wasmi::Store
4 participants