wasmtime / issue #3969 When to remove support for disabli... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #3969 When to remove support for disabli...

Wasmtime GitHub notifications bot (Mar 30 2022 at 15:54):

alexcrichton opened issue #3969:

Wasmtime has support for many proposals to the WebAssembly specification, including those that have since been "stabilized" by merging into the official spec repository. Wasmtime, however, still has knobs to disable these features such as:

reference types

bulk memory

simd

multi-value

Wasmtime also has support for proposals that were merged after the original release of the wasm specification such as nontrapping-fp-to-int and sign-extensions, but these proposals do not have Config options to disable them.

Most of the time support for disabling a feature is pretty easy to support as it's just a tweak during validation, but some proposals such as bulk memory change some edge cases about behavior which means wasmtime has to switch on the appropriate behavior.

I don't believe that we have a preexisting policy on this so I wanted to raise this question: when do we remove the Config::wasm_* methods and unconditionally enable a wasm proposal? I would expect that the bare minimum is that the propposal is merged into the upstream wasm specification, but how much further than that do we wait?

I think that the answer here can be somewhat nuanced as well. For example multi-value probably doesn't make sense to keep around any more but reference types/simd may make sense since they're relatively risky proposals in terms of exposing bugs in Wasmtime.

I would personally propose that we remove a feature flag when it's both merged in the upstream specification and we feel that the implementation is stable within Wasmtime itself. I think we can remove the multi-value and bulk-memory feature flags for sure, and we can possibly remove the simd feature flag as well. For reference types though I think we'll want to keep that since we're less confident in the bug-free-ness of the implementation.

Wasmtime GitHub notifications bot (Mar 30 2022 at 16:03):

fitzgen commented on issue #3969:

Why not always have a feature flag? The nontrapping conversions and exporting mutable globals proposals were already merged into the official spec by the time Wasmtime started to become more mature, and that history is reflected in how they don't have have feature flags. But everything since then has a feature flag, and that is kind of nice for users, especially users that want to be conservative. I'd argue that we should actually add feature flags for those early proposals, rather than remove feature flags for newer proposals, for consistency.

Is the fear that this will just become too difficult to wrangle as more and more features are added to Wasm and they depend on earlier features, etc? I don't think we've really hit that complexity yet, but I kind of feel like it is a bridge we should cross if we ever actually get to it (I'm not convinced that this will actually be a significant problem in practice). The bulk memory edge cases aren't the norm, and I doubt there will be too many more things like that in the future.

Wasmtime GitHub notifications bot (Mar 30 2022 at 16:09):

bjorn3 commented on issue #3969:

What about someone who for example want to allow users of their program to write extensions in wasm. If this program runs both inside a browser (compiled to wasm itself) and natively, they could use the browser itself for running the wasm in case of running the program in the browser and use wasmtime in case of running natively. To ensure extensions always work in both environments they may want to disable non-core features to maximize browser compatibility.

Wasmtime GitHub notifications bot (Mar 30 2022 at 16:13):

alexcrichton commented on issue #3969:

I'm mostly worried about the support necessary to toggling features on/off. For bulk-memory in particular the check_init_bounds function is effectively dead code as I don't think our fuzzing or our tests actually cover it any more. It's true that this hasn't historically been a problem but that's mostly just because it's "hidden" in wasmparser which deals with everything, but eventually I suspect that will be its own problem as well. I would ideally not like to require that Wasmtime's tooling indefinitely retains the ability to artificially turn off features, just for the purposes of Wasmtime itself (there are other use-cases for wasmparser to satisfy beyond Wasmtime of course).

As a runtime I feel like it's our job to provide a polished implementation of wasm with some default set of features and I don't think it makes sense for us to preserve the ability to toggle features in perpetuity. Wasmtime's job is to take a wasm file and run it well. As a producer of wasm I would expect that features are quite important since the wasm may run in multiple locations.

I'm hesitant to wait for the support of disabling stable features to be too onerous as a reason to remove it because that runs the risk of entrenching the need to disable stable features and making it that much harder to remove. For example if someone really wants to rely on the difference in behavior when toggling bulk-memory that makes it that much harder for us to actually go and remove it. Having a clear policy around when to remove the toggle mitigates this because we aren't beholden to old decisions forever and users clearly understand that they can't rely on a feature toggle being present indefinitely.

Wasmtime GitHub notifications bot (Mar 30 2022 at 16:17):

bjorn3 commented on issue #3969:

Would an acceptable compromise be to make behavior changes always on at some point (eg the bulk-memory check_init_bounds thing) but keep verifier changes behind a feature?

Wasmtime GitHub notifications bot (Mar 30 2022 at 16:59):

cfallin commented on issue #3969:

The test/fuzz-coverage argument above is pretty compelling to me: mostly-dead code supporting a rare configuration is going to be more error-prone than a single, always-exercised merged/canonical configuration.

(This is a somewhat frequent argument for removing configuration options in software in general and can be taken too far, and we should be careful not to let the secondary benefits define our main goals here, but it's definitely a real consideration IMHO.)

Simplicity (which begets reliability) also pushes in that direction: fewer combinations to consider, easier to reason about and harder to miss unexpected interactions.

The "check if it still works in MVP (or with less features)" use-case that @bjorn3 suggests seems to me to be more of a validation question. Put another way, a Wasmtime that has an all-features-on runtime but still rejects non-MVP modules on load would satisfy that use case just as well.

Potential mitigating factors to this, and reasons we may want to keep flags (beyond just features that are "not yet stabilized"):

Performance penalties. E.g., do we incur any extra overhead when reftypes support is enabled? Will our eventual exception-handling support imply any setup overhead, extra CFG edges that block optimization, or something of the sort?

Behavior changes rather than just additional opcodes (that are benign if unused). In other words, if a module that would validate and run without feature X (because it doesn't use X), behaves differently with X enabled, then the on-by-default is observable beyond just the modules that directly care about it. I don't know enough about e.g. the bulk-memory changes to know if this actually matters though?

Runtime size considerations, if we also make it a Cargo feature flag. If (just for sake of argument, I don't know if this will eventually be the case) something like EH requires libunwind and this adds significant code size and/or platform dependencies, it seems better for it to remain a programmatically-switchable option (and make the Config method disappear if the feature flag is turned off) rather than have the Wasm feature itself silently disappear and modules fail to load.

Wasmtime GitHub notifications bot (Mar 30 2022 at 19:12):

alexcrichton commented on issue #3969:

To be clear I'm not necessarily thinking we need to aggressively remove flags, what I'm hoping to get to is a point where we can guarantee that all feature flags have a possible removal date, even if it's far in the future.

I don't think it's Wasmtime's job to perform validation that a binary doesn't use some particular wasm proposal. Crates like wasmparser can cater to that use case or other wasm tooling but as a runtime it feels out of our purview to maintain such a role. There's nothing stopping us from doing so but that's more complexity to buy into with what I perceive as very little to gain.

Personally I am not worried about performance penalties. If enabling a feature incurs a performance penalty then I would consider the feature as not being finished or ready to enable by default. We should naturally never have a best practice of using Wasmtime with a recommendation to disable a particular proposal. I'm actually worried about the opposite as well. Having all these feature flags surely slows down wasmparser's decoding of a module. The wasmparser crate already suffers from performance problems and having lots of branch code checking I would assume contributes to at least a nontrivial portion of the slowness.

For behavior changes that's so far either been a "bug" in the proposal (e.g. module linking's extra validation of import strings) or it's intentional in a proposal assuming no one cares (e.g. the change in out-of-bounds behavior during instantiation with bulk memory). In both cases personally I don't think we should be motivated to support both modes of behavior indefinitely.

Finally for runtime size AFAIK that's only possible with cargo feature flags. Runtime options to disable things would rely on const-propagation at the LLVM layer to enable dead code elmination at the linker layer. I don't think that's really possible to do feasibly. In that sense if we actually want binary size wins we won't get that from Config options but instead Cargo compile-time options. I think that would have a pretty different shape than how we make features conditionally available today.

Wasmtime GitHub notifications bot (Mar 30 2022 at 19:34):

cfallin commented on issue #3969:

I don't think it's Wasmtime's job to perform validation that a binary doesn't use some particular wasm proposal. Crates like wasmparser can cater to that use case or other wasm tooling but as a runtime it feels out of our purview to maintain such a role. There's nothing stopping us from doing so but that's more complexity to buy into with what I perceive as very little to gain.

Right, to be clear I'm thinking this could be served by a separate tool just as well; was suggesting this wouldn't be a reason to block removal of feature conditionals :-)

Finally for runtime size AFAIK that's only possible with cargo feature flags.

Yep, exactly, that's what I was suggesting; I wasn't thinking that we would somehow ask LLVM to DCE a feature just based on a const bool, but rather assuming we might have Cargo feature flags and then tracing out the interactions with this suggestion.

To be a bit more explicit about the argument I see: consider the implications if we do have a Cargo feature flag for a feature (let's say it's big and complex -- EH, or GC, or something, and that the embedder may not want to include at all). If we have a Config method to turn the feature on and off, then if the Cargo feature flag is excluded for whatever reason, that method disappears and the embedder realizes the problem as a build error. On the other hand, if the runtime's support for the feature just invisibly turns on/off based on the feature, then the embedder may see mysterious validation errors instead.

In other words it seems better to me that if we have conditional compilation of features, that we expose that choice explicitly as a programmatic feature flag as well. The complexity/testing burden doesn't seem like it would be worse for that, as we'd have to think about the with- and without-cases anyway with the Cargo feature flag.

(In the absence of any Cargo feature flag-gated functionality, the above argument constant-evaluates to "nop" and can be removed as dead code :-) )

Wasmtime GitHub notifications bot (Mar 31 2022 at 21:33):

alexcrichton commented on issue #3969:

https://github.com/bytecodealliance/wasm-tools/issues/540 reminded me of another aspect where proposals are actually changing the existing binary decoding of modules in ways that are subtle and difficult to continue to support over time. For example all this branching on forbid_bulk_memory I find pretty hard to follow and only exists to give the correct error message on a historical version of the spec test suite. That's all otherwise dead and overly confusing code.

This sort of reinterpreting what's already there in the binary encoding happens relatively frequently too. For example wasmparser decodes a memarg differently depending on whether the memory64 proposal is enabled or not. This again is entirely to give the correct error message on spec tests for historical versions (well, for now it's "current") of the spec test suite.

Personally I don't want to commit to supporting any of that indefinitely. I'm fine leaving it all in there so long as the proposal isn't merged to the spec itself but I would actually like to remove all bulk memory conditional handling in wasmparser as well.

I might actually go so far that we should extend the conclusion in this issue to wasmparser as well. If users really want to verify that a binary runs on some older engine then I think we should recommend actually running it on that older or alternative engine. Supporting all the various ways to encode binaries seems like it's going to be a mess as things just keep getting added.

Finally, I'd reiterate that I feel pretty firmly that the use cases here are few and far between. I don't want any one niche use case to force us to maintain a bunch of wonky code. I feel like our job is to provide an implementation of the wasm-specification as is, including all merged proposals, as well as a nice way to implement and work with in-progress proposals that aren't merged to the spec yet. Anything about historical permutations of specifications I don't think makes sense and once something is merged to the specification I feel we should consider that the "new MVP" of wasm and the bare minimum that Wasmtime needs to support. Wasmtime can of course always provide configuration options like "please compile our support for this because it's big", for example we could even do that for all simd instructions one day. But "please retain the ability to decode this year-old module which is no longer considered valid in the current specification for this one test" does not seem useful.

Wasmtime GitHub notifications bot (Mar 31 2022 at 21:46):

cfallin commented on issue #3969:

+1 to all of what @alexcrichton says above, for features like memory64 or multimemory or that sort of thing: the complexity is just not worth it IMHO.

In the cases I was raising above like GC or EH, where runtime size could be an issue and we have feature flags to remove them (then expose the choice as a programmatic flag to surface their absence if excluded), on further thought I suspect we could stub out the bits that are actually problematic if needed. For example, if EH causes problems because if requires libunwind -- let's build a cheaper mechanism that doesn't need libunwind (#2459). If a "real" GC implementation pulls in a large GC engine (mmtk or somesuch) -- make that dependency optional. But the Wasm-facing bits, and core runtime bits, are simpler if they're integrated and treated as one monolithic thing.

To add more anecdata, the feature-flag pain around memfd and uffd caused a ton of headaches, and I'd rather keep feature flags to a minimum as well.

Coming back to @fitzgen's points above, "bridge that we should cross if we get to it" -- this and Alex's experience indicate to me that we should take the "configuration combinatorial explosion" problem seriously as an issue today-ish... thoughts?

Wasmtime GitHub notifications bot (Mar 31 2022 at 22:01):

nagisa commented on issue #3969:

https://github.com/WebAssembly/spec/issues/870 is probably a relevant discussion.

Wasmtime GitHub notifications bot (Mar 31 2022 at 22:07):

sunfishcode commented on issue #3969:

This also helps the producer ecosystem. It simplifies toolchains when features become "part of wasm", that everyone running wasm today is expected to be able to run, rather than perpetually optional things that toolchains have to perpetually feature-detect, or even just avoid altogether.

Some features will always be optional, like threads. And some features will have significant implementation costs and will make sense to keep optional. And what it means to be "part of wasm" is ultimately up to the overall ecosystem. But we can do our part, by enabling features that are appropriate to enable, and giving producers one less thing to worry about "what if someone decides they want to disable the bulk-memory flag today?"

Wasmtime GitHub notifications bot (Apr 04 2022 at 18:30):

fitzgen commented on issue #3969:

One thing to note about switching from config methods to cargo features is that this will have an impact on CI builders we need to have and CI time.

But yeah, I've come around and I'm roughly in agreement with most things stated in this thread.

Last updated: Nov 22 2024 at 17:03 UTC