Skip generating JS import shims when unnecessary #1654
Conversation
|
Nice! This looks great to me, I'm gonna poke around with it a bit more locally and see what's what tomorrow. |
There was a problem hiding this comment.
Ok digging in some more today. So the way that this previously worked was a bit of a hack, but did have some good properties to it I think. Previously we'd actually do the whole rigamarole to generate the JS shim, and then if we ended up not actually emitting anything shim-like it was automatically considered candidate for this optimization. That meant that it wasn't possible to get out of sync with what the backend generated, but it did make the check pretty wonky.
The main thing that I did with this PR was execute WASM_BINDGEN_ANYREF=1 WASM_BINDGEN_NO_DEBUG=1 cargo test --target wasm32-unknown-unknown --test wasm and then look for functions in the wasm-bindgen-test.js file which shouldn't have a shim. I don't really have a preference as to how we structure this code so I'm fine fixing these however!
Some cases I found though which still had a shim but didn't need it were:
- imports from a namespace
- functions returning boolean
- functions returning
u32 - functions taking
u8 - some subset of these functions. They generate a bunch of unhelpfully named wrappers like
__wbg_roundtrip_afe3eacfc448f05dso I don't know which is which, but none of them have a meaningful wrapper in JS. Some of them are optimized without a wrapper, but not all?
I think that was what I found? It looks like it's basically a few type conversions (like incoming boolean -> i32, or incoming unsigned long -> i32) missing and maybe something else to pattern match on for the namespace thing?
| })?; | ||
| let js = format!("function{}", js); | ||
| let js = match import { | ||
| AuxImport::Value(AuxValue::Bare(js)) |
There was a problem hiding this comment.
I think that this may not be quite right in terms of matching where technically any value imported is a candidate for not requiring any glue. For example the other values, getters/setters, can all be hooked up directly in theory. They today, however, all require a method invocation style and will fail the Static check below very quickly, so it effectively ends up being the same.
Perhaps though it'd be good to refactor this in a forward-facing way where we match AuxImport::Value(v) here and then have shared code between way below in here about how to turn v into a JS snippet?
|
Oh one other thought I had while reviewing this, in terms of tests it might be nift to assert that a JS shim is "meaningful"? Could we perhaps have an assert of some form when we generate a JS shim that it's somewhat nontrivial? That way we don't necessarily need to test this directly but we could rely on if that assert trips we forgot to handle something, but if the assert always passes then we're good to go. That would also help us cover future cases such as when anyref is on/off or WebIDL bindings get more fleshed out. |
|
For incoming bools we can rely on the auto conversion turning this into For outgoing bools that are actually going into a a Web IDL function, we can also rely on the auto conversion from a numbre to bool. For outgoing bools that are going into random user JS functions, there won't be any auto conversions going on, and so we could pass the number I'm not sure if incoming unsigned numbers are safe. They get converted to signed integers by the JS Wasm interface, and although positive two's complement has the same representation as unsigned, I'm not sure about the edge cases here. Although, again, if our current behavior doesn't do anything, then I guess at most we are preserving bugs... I guess we are assuming that the wasm-level |
This error case is for an invalid free function, not an invalid constructor.
After this change, any import that only takes and returns ABI-safe numbers (signed
integers less than 64 bits and unrestricted floating point numbers) will be a
direct import, and will not have a little JS shim in the middle.
We don't have a great mechanism for testing the generated bindings' contents --
as opposed to its behavior -- but I manually verified that everything here does
the Right Thing and doesn't have a JS shim:
```rust
\#[wasm_bindgen]
extern "C" {
fn trivial();
fn incoming_i32() -> i32;
fn incoming_f32() -> f32;
fn incoming_f64() -> f64;
fn outgoing_i32(x: i32);
fn outgoing_f32(y: f32);
fn outgoing_f64(z: f64);
fn many(x: i32, y: f32, z: f64) -> i32;
}
```
Furthermore, I verified that when our support for emitting native `anyref` is
enabled, then we do not have a JS shim for the following import, but if it is
disabled, then we do have a JS shim:
```rust
\#[wasm_bindgen]
extern "C" {
fn works_when_anyref_support_is_enabled(v: JsValue) -> JsValue;
}
```
Fixes rustwasm#1636.
Heh... yeah actually this breaks if we don't have a shim that does |
|
Oh sorry so in general the functions I gisted above all had a shim in JS but the shim didn't actually do anything, so I figure that we just want to optimize them to ensure that the shim is gone. That's also where I think it'd help to have an assert that if we generate a shim it's nontrivial in one way or another. For booleans for example yeah for outgoing booleans we need a shim, but for incoming booleans I think it's a missing statement here which catches booleans going to i32.
I think this actually works out for incoming u32. It looks like the coercion to In that sense I think we're actually covered for |
|
If it's possible I think it's still worthwhile to have a default always-on assert for all generated bindings that something is nontrivial about the JS shim that we generate. That I think is a much stronger assert than an annotation b/c we're still playing whack-a-mole with the annotation. I think it's of course though fine to add this stronger assert later. |
It is stronger, yes. However, I am a bit less confident in our ability to live up to its guarantees. By adding an assertion, we are explicitly saying that we expect these incoming and outgoing types not to require glue and that we are confident that we can guarantee that forever. I'm less certain about catching every single case that might happen not to require glue for every combination of bindings configurations and types that we support. Additionally, implementing the check seems like it would have to be fairly ad-hoc unless we change the way the builders work a bit, and I'm not certain that the assertion itself wouldn't be super buggy. |
|
Heh good points as well :) In any case let's stick with a tag for now, and maybe add the tag to a more exhaustive set of types going in/out as well? |
yeah working on it now |
|
Ok, I think I got everything -- want to take another look @alexcrichton? |
This should not be used outside of wasm-bindgen's test suite.
|
r=me on everything here, but I think this line may be wrong? While I think 0/1 have the same "truthy value" as |
Fixed! That was supposed to only be for incoming expressions >_< |
Support was previously (re-)added in rustwasm#1654 for importing direct JS values into a WebAssembly module by completely skipping JS shim generation. This commit takes that PR one step further by *also* embedding a direct import in the wasm file, where supported. The wasm file currently largely just imports from the JS shim file that we generate, but this allows it to directly improt from ES modules where supported and where possible. Note that like rustwasm#1654 this only happens when the function signature doesn't actually require any conversions to happen in JS (such as handling closures). For imports from ES modules, local snippets, or inline JS they'll all have their import directives directly embedded into the final WebAssembly binary without any shims necessary to hook it all up. For imports from the global namespace or possibly vendor-prefixed items these still unconditionally require an import shim to be generated because there's no way to describe that import in an ES-friendly way (yet). There's a few consequences of this commit which are also worth noting: * The logic in `wasm-bindgen` where it gracefully handles (to some degree) not-defined items now only is guaranteed to be applied to the global namespace. If you import from a module, it'll be an instantiation time error rather than today's runtime error when the import is called. * Handling imports in the wasm module not registered with `#[wasm_bindgen]` has become more strict. Previously these imports were basically ignored, leaving them up for interpretation depending on the output format. The changes for each output target are: * `bundler` - not much has changed here. Previously these ignored imports would have been treated as ES module imports, and after this commit there might just be some more of these imports for bundlers to resolve. * `web` - previously the ignored imports would likely cause instantiation failures because the import object never actually included a binding for other imports. After this commit though the JS glue which instantiates the module now interprets all unrecognized wasm module imports as ES module imports, emitting an `import` directive. This matches what we want for the direct import functionality, and is also largely what we want for modules in general. * `nodejs` - previously ignored imports were handled in the translation shim for Node to generate `require` statements, so they were actually "correctly handled" sort of with module imports. The handling of this hasn't changed, and reflects what we want for direct imports of values where loading a wasm module in Node ends up translating the module field of each import to a `require`. * `no-modules` - this is very similar to the `web` target where previously this didn't really work one way or the other because we'd never fill in more fields of the import object when instantiating the module. After this PR though this is a hard-error to have unrecognized imports from `#[wasm_bindgen]` with the `no-modules` output type, because we don't know how to handle the imports. Note that this touches on rustwasm#1584 and will likely break the current use case being mentioned there. I think though that this tightening up of how we handle imports is what we'll want in the long run where everything is interpreted as modules, and we'll need to figure out best how wasi fits into this. This commit is unlikely to have any real major immediate effects. The goal here is to continue to inch us towards a world where there's less and less JS glue necessary and `wasm-bindgen` is just a polyfill for web standards that otherwise all already exist. Also note that there's no explicitly added tests for this since this is largely just a refactoring of an internal implementation detail of `wasm-bindgen`, but the main `wasm` test suite has many instances of this path being taken, for example having imports like: (import "tests/wasm/duplicates_a.js" "foo" (func $__wbg_foo_969c253238f136f0 (type 1))) (import "tests/wasm/duplicates_b.js" "foo" (func $__wbg_foo_027958cb2e320a94 (type 0))) (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "trivial" (func $__wbg_trivial_75e27c84882af23b (type 1))) (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "incoming_bool" (func $__wbg_incomingbool_0f2d9f55f73a256f (type 0)))
Support was previously (re-)added in rustwasm#1654 for importing direct JS values into a WebAssembly module by completely skipping JS shim generation. This commit takes that PR one step further by *also* embedding a direct import in the wasm file, where supported. The wasm file currently largely just imports from the JS shim file that we generate, but this allows it to directly improt from ES modules where supported and where possible. Note that like rustwasm#1654 this only happens when the function signature doesn't actually require any conversions to happen in JS (such as handling closures). For imports from ES modules, local snippets, or inline JS they'll all have their import directives directly embedded into the final WebAssembly binary without any shims necessary to hook it all up. For imports from the global namespace or possibly vendor-prefixed items these still unconditionally require an import shim to be generated because there's no way to describe that import in an ES-friendly way (yet). There's a few consequences of this commit which are also worth noting: * The logic in `wasm-bindgen` where it gracefully handles (to some degree) not-defined items now only is guaranteed to be applied to the global namespace. If you import from a module, it'll be an instantiation time error rather than today's runtime error when the import is called. * Handling imports in the wasm module not registered with `#[wasm_bindgen]` has become more strict. Previously these imports were basically ignored, leaving them up for interpretation depending on the output format. The changes for each output target are: * `bundler` - not much has changed here. Previously these ignored imports would have been treated as ES module imports, and after this commit there might just be some more of these imports for bundlers to resolve. * `web` - previously the ignored imports would likely cause instantiation failures because the import object never actually included a binding for other imports. After this commit though the JS glue which instantiates the module now interprets all unrecognized wasm module imports as ES module imports, emitting an `import` directive. This matches what we want for the direct import functionality, and is also largely what we want for modules in general. * `nodejs` - previously ignored imports were handled in the translation shim for Node to generate `require` statements, so they were actually "correctly handled" sort of with module imports. The handling of this hasn't changed, and reflects what we want for direct imports of values where loading a wasm module in Node ends up translating the module field of each import to a `require`. * `no-modules` - this is very similar to the `web` target where previously this didn't really work one way or the other because we'd never fill in more fields of the import object when instantiating the module. After this PR though this is a hard-error to have unrecognized imports from `#[wasm_bindgen]` with the `no-modules` output type, because we don't know how to handle the imports. Note that this touches on rustwasm#1584 and will likely break the current use case being mentioned there. I think though that this tightening up of how we handle imports is what we'll want in the long run where everything is interpreted as modules, and we'll need to figure out best how wasi fits into this. This commit is unlikely to have any real major immediate effects. The goal here is to continue to inch us towards a world where there's less and less JS glue necessary and `wasm-bindgen` is just a polyfill for web standards that otherwise all already exist. Also note that there's no explicitly added tests for this since this is largely just a refactoring of an internal implementation detail of `wasm-bindgen`, but the main `wasm` test suite has many instances of this path being taken, for example having imports like: (import "tests/wasm/duplicates_a.js" "foo" (func $__wbg_foo_969c253238f136f0 (type 1))) (import "tests/wasm/duplicates_b.js" "foo" (func $__wbg_foo_027958cb2e320a94 (type 0))) (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "trivial" (func $__wbg_trivial_75e27c84882af23b (type 1))) (import "./snippets/wasm-bindgen-3dff2bc911f0a20c/inline0.js" "incoming_bool" (func $__wbg_incomingbool_0f2d9f55f73a256f (type 0)))
After this change, any import that only takes and returns ABI-safe numbers (signed integers less than 64 bits and unrestricted floating point numbers) will be a direct import, and will not have a little JS shim in the middle.
We don't have a great mechanism for testing the generated bindings' contents -- as opposed to its behavior -- but I manually verified that everything here does the Right Thing and doesn't have a JS shim:
Furthermore, I verified that when our support for emitting native
anyrefis enabled, then we do not have a JS shim for the following import, but if it is disabled, then we do have a JS shim:Fixes #1636.