wasmtime / PR #8531 wasmtime: Make table lazy-init config... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8531 wasmtime: Make table lazy-init config...

Wasmtime GitHub notifications bot (May 02 2024 at 22:39):

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 per call_indirect or table_get. The typed-funcrefs.wat test becomes branch-free, with two loads and an indirect call per call_indirect, making it equivalent to the version that uses globals instead of tables.

cc: @cfallin

Wasmtime GitHub notifications bot (May 02 2024 at 23:44):

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>

Wasmtime GitHub notifications bot (May 02 2024 at 23:44):

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 nested structs).

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.

Learn more.

</details>

Wasmtime GitHub notifications bot (May 03 2024 at 01:13):

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 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 per call_indirect or table_get. The typed-funcrefs.wat test becomes branch-free, with two loads and an indirect call per call_indirect, making it equivalent to the version that uses globals instead of tables.

cc: @cfallin

Wasmtime GitHub notifications bot (May 03 2024 at 19:06):

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?

Wasmtime GitHub notifications bot (May 03 2024 at 19:06):

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

Wasmtime GitHub notifications bot (May 03 2024 at 19:06):

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?

Wasmtime GitHub notifications bot (May 03 2024 at 19:06):

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?

Wasmtime GitHub notifications bot (May 03 2024 at 20:33):

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 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 per call_indirect or table_get. The typed-funcrefs.wat test becomes branch-free, with two loads and an indirect call per call_indirect, making it equivalent to the version that uses globals instead of tables.

cc: @cfallin

Wasmtime GitHub notifications bot (May 03 2024 at 23:07):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (May 03 2024 at 23:07):

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(), only funcrefs_mut().

StaticFuncTable should maybe put the current size in the data 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 gets elements.as_mut_slice()/unsafe { data.as_mut() } depending on whether the table is static or dynamic, and then afterwards does the unsafe slice::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

Wasmtime GitHub notifications bot (May 06 2024 at 19:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 06 2024 at 19:30):

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

Wasmtime GitHub notifications bot (May 14 2024 at 03:08):

jameysharp updated PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

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 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 per call_indirect or table_get. The typed-funcrefs.wat test becomes branch-free, with two loads and an indirect call per call_indirect, making it equivalent to the version that uses globals instead of tables.

cc: @cfallin

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

jameysharp has marked PR #8531 as ready for review.

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

jameysharp requested fitzgen for a review on PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

jameysharp requested wasmtime-fuzz-reviewers for a review on PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 03:09):

jameysharp requested wasmtime-core-reviewers for a review on PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 03:27):

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?

Wasmtime GitHub notifications bot (May 14 2024 at 04:04):

jameysharp commented on PR #8531:

Okay, I've learned that the table_ops fuzzer can't test this because it only uses externref tables, but lazy-init only applies to funcref tables. And the single-inst module-builder for differential doesn't generate tables or table instructions at all.

But I'm still not sure why wasm-smith isn't finding my asserts with either differential or instantiate. 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.

Wasmtime GitHub notifications bot (May 14 2024 at 04:33):

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 run cargo test --test all with table_lazy_init set false, and the only tests that failed were Winch-related (now with the error message from Config::validate) and the code_too_large_without_panic test again.

So I think this is ready to land.

Wasmtime GitHub notifications bot (May 14 2024 at 06:44):

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>

Wasmtime GitHub notifications bot (May 14 2024 at 15:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 14 2024 at 15:27):

alexcrichton merged PR #8531.

Wasmtime GitHub notifications bot (May 14 2024 at 16:46):

fitzgen commented on PR #8531:

We should really add a top-down generation mode to wasm-smith...

Last updated: Oct 23 2024 at 20:03 UTC