wasmtime: Make table lazy-init configurable

[jameysharp]

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

[github-actions]

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.

[github-actions]

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.

[alexcrichton]

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]

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

[jameysharp]

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

[alexcrichton]

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

[alexcrichton]

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]

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]

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]

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]

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.

[fitzgen]

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