-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
perf(ext/web): Avoid changing prototype by setting hostObjectBrand directly #21358
Conversation
There seem to be some unrelated changes necessary before the deno_core dependency can be updated (or possibly my build is broken.) After those are done I can push a temporary commit to this branch so CI can run with my deno_core branch. |
3284dc5
to
b873ac8
Compare
8682cf4
to
38be48d
Compare
38be48d
to
d880e80
Compare
945f18f
to
2eb03e8
Compare
2eb03e8
to
c745aad
Compare
Are there more places where we could use this new API? |
This was the only place I found using The only objects that are currently transferable are Lines 167 to 187 in 6718be8
Hopefully this change will make it possible to make other objects transferable since it removes the substantial performance penalty from using traditional host objects created outside JS. |
ObjectSetPrototypeOf(port, MessagePortPrototype); | ||
port[webidl.brand] = webidl.brand; | ||
const port = webidl.createBranded(MessagePort); | ||
port[core.hostObjectBrand] = core.hostObjectBrand; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the transfer behavior of MessagePort
is handled by {de,}serializeJsMessageData
because that is the only transferable web API, but if the idea is to make this more generic, it should not be specified at a single global place (which also would not work for serializing CryptoKey
, since that is defined in ext/crypto
). Should we make the value assigned to this symbol key an object with optional serialize
and transfer
methods? (Deserialization would still need some global registration though.)
This, however, would make it possible for users to customize serialization and transfer behavior, and even make custom objects serializable, by extracting the symbol with Object.getOwnPropertySymbols()
. Do we want to allow this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be tackled later when the transfer functionality is generalised, but I think we'll probably be better off thinking about this symbol as simply an opt out of the default fast path for non-host objects. The logic itself should probably live in a global registry keyed on constructor or prototype. Any objects with this symbol but without a corresponding entry in the registry should probably trigger a DOMException: Unsupported object type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the structured serialization algorithm as per the HTML spec doesn't care about prototypes, it uses the WebIDL interface the object was constructed as implementing. I would've sworn there were WPT tests for this (that I wrote!), but I can't find them 😅
I'm not saying Deno must implement structured cloning that way, but if we're going to have disagreements with the spec, they should be intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We should open an issue to track future improvements.
This is currently blocking landing other deno_core changes and should a user discover the symbol, it doesn't really allow them to do anything other than cause themselves grief, so I prefer to get something in now and fix it later.
NOTE: Remove TMP commit before merging.