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

wasmtime: Make table lazy-init configurable #8531

Merged
merged 2 commits into from
May 14, 2024

Conversation

jameysharp
Copy link
Contributor

@jameysharp jameysharp commented May 2, 2024

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

Sorry, something went wrong.

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime winch Winch issues or pull requests labels May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Subscribe to Label Action

cc @saulecabrera

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 .github/subscribe-to-label.json configuration file.

Learn more.

Copy link

github-actions bot commented May 2, 2024

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.

    Our documentation should be of the following form:

    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.
    
  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    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 (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 for more 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.


To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

Sorry, something went wrong.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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?

@@ -701,29 +726,60 @@ impl Table {
self.element_type().matches(val)
}

fn funcrefs(&self) -> &[TaggedFuncRef] {
fn funcrefs(&self) -> (&[TaggedFuncRef], bool) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. 😅 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

Copy link
Member

Choose a reason for hiding this comment

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

That all sounds great to me yeah, and seems fine to leave this as-is returning a bool in the meantime

TableStyle::CallerChecksSignature { lazy_init: true } => {
self.emit_lazy_init_funcref(table_index)
}
_ => unimplemented!("Support for eager table init"),
Copy link
Member

Choose a reason for hiding this comment

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

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?

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.
@jameysharp jameysharp marked this pull request as ready for review May 14, 2024 03:09
@jameysharp jameysharp requested review from a team as code owners May 14, 2024 03:09
@jameysharp jameysharp requested review from fitzgen and removed request for a team May 14, 2024 03:09
@jameysharp
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@jameysharp
Copy link
Contributor Author

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.

@github-actions github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label May 14, 2024
Copy link

Subscribe to Label Action

cc @fitzgen

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 .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton added this pull request to the merge queue May 14, 2024
Merged via the queue into bytecodealliance:main with commit 7ea9201 May 14, 2024
22 checks passed
@jameysharp jameysharp deleted the table-lazy-init branch May 14, 2024 15:42
@fitzgen
Copy link
Member

fitzgen commented May 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants