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

Enable ref types and bulk memory operations by default #2118

Merged
merged 6 commits into from Aug 8, 2020

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Aug 7, 2020

Both have spent a bit of time enabled in our fuzzers by default and we haven't run into any issues in a while. The one catch was the potential slowdown due to #1883, but

  • I measured and, while it did exist, it was on the order of milliseconds when dealing with a 270K wasm module, and
  • I've added a commit that fixes the slowdown (and is otherwise generally a tiny speed up by doing a little manual LICM that the compiler failed to do because of a trait method call).

@alexcrichton
Copy link
Member

The only prior experience we have for this is multi-value, and we didn't actually enable that by default until it was merged into the upstream spec itself. In that sense this is much earlier to enable-by-default than our precedent.

Given that, I wonder if we might want to take this moment to discuss/agree about when to enable new features by default? I personally feel that merging into the spec is probably too slow, and stage 4 (which these proposals are at) feels like it's the right time. It's what Firefox did!

If others agree, could this also add some documentation in the book to when we turn a feature on by default? I would say the requirements for a Wasmtime wasm feature to be on by default would be:

  • Upstream proposal in stage 4
  • Complete implementation in Wasmtime
  • No major bugs or design work needs to be done
  • Fuzzing enabled for awhile (week or so?)

In terms of the content of this PR, r=me, just wanted to point out the procedural point!

@alexcrichton
Copy link
Member

Actually some other requirements I would add are:

  • Implemented in the wasmtime crate's API
  • Implemented in the C API
  • Implemented in at least one language binding (e.g. Go, Python, .NET)

@fitzgen
Copy link
Member Author

fitzgen commented Aug 7, 2020

This is a good point to bring up, thanks Alex. Once we find consensus on what is required here, we should probably document it in our contributing guide.


You proposed requirements sound good to me, but I would even expand the last point about fuzzing to include a review of the ways that we've exposed the feature to the fuzzers.

  • Is it just enabled in the "compile" fuzz target's config or did we write custom fuzz targets to explicitly exercise running (rather than just compiling) this feature?
  • Did we add seeds to our corpora that use the feature?
  • Generally, how confident are we that the fuzzers have actually exercised this stuff? (e.g. does introducing a panic on purpose to some corner case get caught by the fuzzer pretty quickly?)

In particular, if we aren't pretty confident about that last point, I don't think we are ready to turn the feature on by default yet.

According to these standards, I feel good about reference types, and okay about bulk memory. I think bulk memory could probably use more runtime testing, rather than just "can we compile modules that use it" testing. For reference types, we have specialized fuzz targets that actually run the modules, not just compile them.

@fitzgen fitzgen force-pushed the enable-ref-types-by-default branch from 9307e47 to e0c014c Compare August 7, 2020 19:37
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself labels Aug 7, 2020
@alexcrichton
Copy link
Member

Oh yeah and to be clear I also feel good about the implementation we have of reference types and bulk memory myself.

Also, as an aside, would you be up (after this lands) to send a similar PR to wasm-tools to update Default for WasmFeatures in wasmparser?

@github-actions
Copy link

github-actions bot commented Aug 7, 2020

Subscribe to Label Action

cc @bnjbvr, @peterhuene

This issue or pull request has been labeled: "cranelift", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift
  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton
Copy link
Member

If you want to add some docs to https://bytecodealliance.github.io/wasmtime/stability.html (or that section, maybe not exactly that page), that might be a good place to add some info?

@fitzgen fitzgen force-pushed the enable-ref-types-by-default branch from e0c014c to ef13e80 Compare August 7, 2020 23:56
@fitzgen
Copy link
Member Author

fitzgen commented Aug 7, 2020

@alexcrichton okay I added two new bits of docs:

  1. A table, similar to https://webassembly.org/roadmap/, documenting our wasm proposal support. This is intended to be mostly outward facing.

  2. A couple checklists for implementing and enabling-by-default new wasm proposals. These are intended to be developer/reviewer focused.

Want to take another look?

@alexcrichton
Copy link
Member

Thanks for writing all that up!

@fitzgen fitzgen merged commit 5f36d7e into bytecodealliance:main Aug 8, 2020
@fitzgen fitzgen deleted the enable-ref-types-by-default branch August 8, 2020 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants