Implement rustwasm/rfcs#5, implement Deref for imports and structural by default
#1019
Conversation
This commit implements the first half of [RFC rustwasm#5] where the `Deref` trait is implemented for all imported types. The target of `Deref` is either the first entry of the list of `extends` attribute or `JsValue`. All examples using `.as_ref()` with various `web-sys` types have been updated to the more ergonomic deref casts now. Additionally the `web-sys` generation of the `extends` array has been fixed slightly to explicitly list implementatoins in the hierarchy order to ensure the correct target for `Deref` is chosen. [RFC rustwasm#5]: https://github.com/rustwasm/rfcs/blob/master/text/005-structural-and-deref.md
Previously `arguments` was used to pass around an array of arguments, but this wasn't actually a `js_sys::Array` but rather a somewhat esoteric internal object. When switching over `Array` methods to be `structural` this caused issues because the inherent methods on an `arguments` object were different than that of `js_sys::Array`.
This was a copy/paste typo!
This commit removes shims, where possible, for `structural` items.
Instead of generating code that looks like:
const target = function() { this.foo(); };
exports.__wbg_thing = function(a) { target.call(getObject(a)); };
we now instead generate:
exports.__wbg_thing = function(a) { getObject(a).foo(); };
Note that this only applies to `structural` bindings, all default
bindings (as of this commit) are still using imported targets to ensure
that their binding can't change after instantiation.
This change was [detailed in RFC rustwasm#5][link] as an important optimization
for `structural` bindings to ensure they've got performance parity with
today's non-`structural` default bindings.
[link]: https://rustwasm.github.io/rfcs/005-structural-and-deref.html#why-is-it-ok-to-make-structural-the-default
The wasm spec defines boolean conversion when crossing to the wasm type i32 as 1 for `true` and 0 for `false`, so no need for us to do it ourselves!
|
A lot more information for this PR can also be found in the commit messages! |
This commit switches all imports of JS methods to `structural` by default. Proposed in [RFC 5] this should increase the performance of bindings today while also providing future-proofing for possible confusion with the recent addition of the `Deref` trait for all imported types by default as well. A new attribute, `host_binding`, is introduced in this PR as well to recover the old behavior of binding directly to an imported function which will one day be the precise function on the prototype. Eventually `web-sys` will switcsh over entirely to being driven via `host_binding` methods, but for now it's been measured to be not quite as fast so we're not making that switch yet. Note that `host_binding` differs from the proposed name of `final` due to the controversy, and its hoped that `host_binding` is a good middle-ground! [RFC 5]: https://rustwasm.github.io/rfcs/005-structural-and-deref.html
| .style() | ||
| .set_property("border", "solid") | ||
| .unwrap(); | ||
| canvas.style().set_property("border", "solid")?; |
| @@ -0,0 +1,55 @@ | |||
| # Inheritance in `web-sys` | |||
|
|
|||
| Inheritance between JS classes is the bread and butter of how the DOM works on | |||
|
Tests added and updated! |
|
I don't like the name But since it's such a niche and rarely used feature, I don't have a particularly strong opinion, so I'm not going to block on it. |
| ideally has no user-visible effect, and well-typed `structural` imports (the | ||
| default) should be able to transparently switch to `host_binding` eventually. | ||
|
|
||
| The eventual performance aspect is that with the [host bindings |
There was a problem hiding this comment.
I think this is also misleading, since structural is generally faster, and the amount of JS shims is the same (see my comment below comparing structural and host_binding).
The only potential performance difference is whether it needs to lookup the prototype chain once (with host_binding) or multiple times (with structural).
There was a problem hiding this comment.
I think this is appropriately worded as-is because it explicitly says the "eventual performance", not commenting on today's performance. This is quickly becoming an incredibly niche feature that basically no one will use, so I don't think it's necessary to have absolutely exhaustive documentation for such a feature which goes into 100% of the nuances of why one might or might not want to use the attribute
| ``` | ||
|
|
||
| and voila! We, with [reference types][reference-types] and [host | ||
| bindings][host-bindings], now have no JS shim at all necessary to call the |
There was a problem hiding this comment.
I would still call that a JS shim. It is JS glue code which is needed to convert a JS value (Foo.prototype.bar) into something which is capable of being imported by wasm, since wasm cannot import it directly.
Also, I think it should mention what the structural version would look like:
export function __wbg_bar_a81456386e6b526f(varg1) { return this.bar(varg1); }Quite short and simple, and also just as performant.
There was a problem hiding this comment.
I've switched to phrasing this as "JS function shim", I think being pedantic here about what's a JS shim and what isn't is detracting from the point of the documentatino.
And again to be clear, this isn't trying to describe the performance today, it's describing the eventual performance where if you have any JS shim at all then wasm engines will be unable to route calls to C++ dom functions directly to C++. The whole point of remove the JS shim is to ensure that DOM calls can go straight to C++ with as little fuss as possible.
There was a problem hiding this comment.
Oh! I see! I finally understand our confusion.
You've been thinking about this as exclusively about host built-ins (like Node.prototype.appendChild), whereas I've been thinking about it as a general purpose mechanism (which can be used on anything, including JS methods).
From the perspective of built-ins, you're completely right that adding in any sort of indirection (i.e. anything other than export const bar = Foo.prototype.bar;) is bad for performance, since it can't directly call the built-in C++ function (at least not without a bit of VM magic).
But on the other hand, from the perspective of regular JS classes, you won't gain any performance by doing export const bar = Foo.prototype.bar;, since it still has to call into a JS method.
So from a general standpoint, it is indeed a JS shim, but from the built-ins standpoint it isn't a JS shim. Hence our disagreement and confusion.
I think we could use a better term for it ("direct" vs "indirect" JS call, maybe? Or "built-in" vs "non-built-in" method?), but at least now the confusion has been resolved.
As for the name, none of them are great, and we can easily spend months bikeshedding it, so let's just go with final.
There was a problem hiding this comment.
Ah yes indeed, this would cause confusion!
I don't know what this will mean for regular JS classes. I think it's too early to say whether it will or won't have a difference.
|
It's a good point that |
This was an intended change from rustwasm#1019, but we forgot to apply it! Closes rustwasm#1095
This commit implements rustwasm/rfcs#5, namely implementing two large changes:
Dereftrait is now implemented unconditionally for all imported JS typesstructuralattribute is now the defaultBoth of those changes have lots of discussion in the RFC, and to support those changes this PR implements a few other changes as well:
structuralbindings has been optimized to remove almost all of the shims that were inserted. The JS glue should be much more optimal and perform better (as measured in Add RFC forDerefinweb-sysrfcs#5)Closes #1018