jameysharp opened PR #8531 from jameysharp:table-lazy-init
to bytecodealliance:main
:
Lazy initialization of tables has trade-offs that we haven't explored in a while. Making it configurable makes it easier to test the effects of these trade-offs on a variety of WebAssembly programs, and allows embedders to decide whether the trade-offs are worth-while for their use cases.
TODO:
- [ ] Finish documentation comments
- [ ] Either implement support in Winch, or force lazy-init on for Winch
- [ ] Extend the test suite
I've tested this by temporarily setting the flag to disable lazy-init by default and verifying that the test suite still passes, except for a bunch of Winch tests (as expected), as well as the amusing case of
code_too_large::code_too_large_without_panic
, which fails because disabling lazy-init deletes enough code that the code is no longer too large.With lazy-init disabled, the
disas
tests show that this shaves off six or seven CLIF instructions and a cold block percall_indirect
ortable_get
. Thetyped-funcrefs.wat
test becomes branch-free, with two loads and an indirect call percall_indirect
, making it equivalent to the version that uses globals instead of tables.cc: @cfallin
github-actions[bot] commented on PR #8531:
Subscribe to Label Action
cc @saulecabrera
<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config", "winch"Thus the following users have been cc'd because of the following labels:
- saulecabrera: winch
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
github-actions[bot] commented on PR #8531:
Label Messager: wasmtime:config
It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:
[ ] If you added a new
Config
method, you wrote extensive documentation for
it.<details>
Our documentation should be of the following form:
```text
Short, simple summary sentence.More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.Is this method fallible? If so, when can it return an error?
Can this method panic? If so, when does it panic?
Example
Optional example here.
```</details>
[ ] If you added a new
Config
method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.<details>
For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config
][fuzzing-config] (or one
of its nestedstruct
s).Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.</details>
[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html
<details>
To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.
To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.</details>
jameysharp edited PR #8531:
Lazy initialization of tables has trade-offs that we haven't explored in a while. Making it configurable makes it easier to test the effects of these trade-offs on a variety of WebAssembly programs, and allows embedders to decide whether the trade-offs are worth-while for their use cases.
TODO:
- [ ] Finish documentation comments
- [ ] Either implement support in Winch, or force lazy-init on for Winch
- [ ] Extend the test suite
I've tested this by temporarily setting the flag to disable lazy-init by default (in
crates/environ/src/tunables.rs
) and verifying that the test suite still passes, except for a bunch of Winch tests (as expected), as well as the amusing case ofcode_too_large::code_too_large_without_panic
, which fails because disabling lazy-init deletes enough code that the code is no longer too large.With lazy-init disabled, the
disas
tests show that this shaves off six or seven CLIF instructions and a cold block percall_indirect
ortable_get
. Thetyped-funcrefs.wat
test becomes branch-free, with two loads and an indirect call percall_indirect
, making it equivalent to the version that uses globals instead of tables.cc: @cfallin
alexcrichton submitted PR review:
I like the idea of adding this, good idea!
Some other bits I might recommend:
- Mind adding a CLI flag under
-O
for this as well?- For testing it might also be good enough to "just" hook this up to fuzzing
- Want to add a
disas
test or two with this flag?
alexcrichton created PR review comment:
What do you think about perhaps returning
enum Funcrefs<'a> { Tagged(&'a [TaggedFuncRef]), Untagged(&'a [*mut VMFuncRef]) }
here? That feels a bit more "type safe" if a bit wordier and reflects how this is a boolean decision at runtime as well
alexcrichton submitted PR review:
I like the idea of adding this, good idea!
Some other bits I might recommend:
- Mind adding a CLI flag under
-O
for this as well?- For testing it might also be good enough to "just" hook this up to fuzzing
- Want to add a
disas
test or two with this flag?
alexcrichton created PR review comment:
If this remains (which I think is fine) mind adding a check in
Config::validate
that returns an error if winch is enabled but lazy init is disabled?
jameysharp edited PR #8531:
Lazy initialization of tables has trade-offs that we haven't explored in a while. Making it configurable makes it easier to test the effects of these trade-offs on a variety of WebAssembly programs, and allows embedders to decide whether the trade-offs are worth-while for their use cases.
TODO:
- [ ] Finish documentation comments
- [ ] Add CLI flag in the
-O
group- [ ] Either implement support in Winch, or check in
Config::validate
that when Winch is enabled so is lazy-init- [ ] Add
disas
tests with lazy init disabled- [ ] Hook up this flag in fuzzing
I've tested this by temporarily setting the flag to disable lazy-init by default (in
crates/environ/src/tunables.rs
) and verifying that the test suite still passes, except for a bunch of Winch tests (as expected), as well as the amusing case ofcode_too_large::code_too_large_without_panic
, which fails because disabling lazy-init deletes enough code that the code is no longer too large.With lazy-init disabled, the
disas
tests show that this shaves off six or seven CLIF instructions and a cold block percall_indirect
ortable_get
. Thetyped-funcrefs.wat
test becomes branch-free, with two loads and an indirect call percall_indirect
, making it equivalent to the version that uses globals instead of tables.cc: @cfallin
jameysharp submitted PR review.
jameysharp created PR review comment:
I like this idea, but while testing it out, I found so many more things I want to change about how the lazy-tagging invariants are maintained in these modules that I don't think I want to change any of them in this PR. :sweat_smile: A few:
- Getting a table reference should return enough information that the caller can initialize any elements it needs to, instead of passing in a range of indexes to init.
Table::get
and table copies should take&mut src
and init the elements they need in-place; the caller always has a mutable pointer.- Therefore, there's no need for
funcrefs()
, onlyfuncrefs_mut()
.StaticFuncTable
should maybe put the current size in thedata
slice and the capacity in a separate field; it's currently the other way around.funcrefs_mut()
is shorter and easier to reason about if it first getselements.as_mut_slice()
/unsafe { data.as_mut() }
depending on whether the table is static or dynamic, and then afterwards does the unsafeslice::from_raw_parts_mut
. I mixed up*mut [T]
with*mut [*mut [T]]
at one point and was afraid the.cast()
would just silently "fix" it for me- I'm imagining something resembling
MaybeUninit
as an interface for tracking at the type level whether it's okay to read a range of the table
alexcrichton submitted PR review.
alexcrichton created PR review comment:
That all sounds great to me yeah, and seems fine to leave this as-is returning a bool in the meantime
jameysharp updated PR #8531.
jameysharp edited PR #8531:
Lazy initialization of tables has trade-offs that we haven't explored in a while. Making it configurable makes it easier to test the effects of these trade-offs on a variety of WebAssembly programs, and allows embedders to decide whether the trade-offs are worth-while for their use cases.
TODO:
- [x] Finish documentation comments
- [x] Add CLI flag in the
-O
group- [x] Either implement support in Winch, or check in
Config::validate
that when Winch is enabled so is lazy-init- [x] Add
disas
tests with lazy init disabled- [x] Hook up this flag in fuzzing
I've tested this by temporarily setting the flag to disable lazy-init by default (in
crates/environ/src/tunables.rs
) and verifying that the test suite still passes, except for a bunch of Winch tests (as expected), as well as the amusing case ofcode_too_large::code_too_large_without_panic
, which fails because disabling lazy-init deletes enough code that the code is no longer too large.With lazy-init disabled, the
disas
tests show that this shaves off six or seven CLIF instructions and a cold block percall_indirect
ortable_get
. Thetyped-funcrefs.wat
test becomes branch-free, with two loads and an indirect call percall_indirect
, making it equivalent to the version that uses globals instead of tables.cc: @cfallin
jameysharp has marked PR #8531 as ready for review.
jameysharp requested fitzgen for a review on PR #8531.
jameysharp requested wasmtime-fuzz-reviewers for a review on PR #8531.
jameysharp requested wasmtime-compiler-reviewers for a review on PR #8531.
jameysharp requested wasmtime-core-reviewers for a review on PR #8531.
jameysharp commented on PR #8531:
I think I've addressed all of your suggestions, Alex; thank you for them!
I'm trying to coax the fuzzer to discover an artificial bug I added in the non-lazy-init configuration just to make sure it can, and it's been running for a while without finding it. So maybe you (or whoever reviews this) can double-check if I did something wrong in the fuzzer configuration?
jameysharp commented on PR #8531:
Okay, I've learned that the
table_ops
fuzzer can't test this because it only usesexternref
tables, but lazy-init only applies tofuncref
tables. And thesingle-inst
module-builder fordifferential
doesn't generate tables or table instructions at all.But I'm still not sure why
wasm-smith
isn't finding my asserts with eitherdifferential
orinstantiate
. I tried both asserting that lazy-init is enabled, and that it's disabled, during codegen in wasmtime-cranelift, and neither one is getting hit.
jameysharp commented on PR #8531:
Nevermind,
instantiate
found my assert a few minutes after I wrote that, and then found the opposite assert after another 20 minutes. And I've also once again runcargo test --test all
withtable_lazy_init
set false, and the only tests that failed were Winch-related (now with the error message fromConfig::validate
) and thecode_too_large_without_panic
test again.So I think this is ready to land.
github-actions[bot] commented on PR #8531:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton submitted PR review.
alexcrichton merged PR #8531.
fitzgen commented on PR #8531:
We should really add a top-down generation mode to
wasm-smith
...
Last updated: Dec 23 2024 at 13:07 UTC