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

perf(ext/web): Avoid changing prototype by setting hostObjectBrand directly #21358

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

lrowe
Copy link
Contributor

@lrowe lrowe commented Nov 27, 2023

NOTE: Remove TMP commit before merging.

@lrowe
Copy link
Contributor Author

lrowe commented Nov 27, 2023

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.

@mmastrac
Copy link
Contributor

mmastrac commented Nov 27, 2023

@lrowe deno_core bump coming up in #21351 bumped

@lrowe lrowe force-pushed the lrowe-custom-host-object branch 3 times, most recently from 8682cf4 to 38be48d Compare November 28, 2023 01:31
@lrowe lrowe changed the title WIP: perf(ext/web): Avoid changing prototype by setting hostObjectBrand directly perf(ext/web): Avoid changing prototype by setting hostObjectBrand directly Nov 28, 2023
@lrowe lrowe marked this pull request as draft November 28, 2023 03:14
@lrowe lrowe marked this pull request as ready for review November 28, 2023 18:29
@bartlomieju
Copy link
Member

Are there more places where we could use this new API?

@lrowe
Copy link
Contributor Author

lrowe commented Nov 29, 2023

Are there more places where we could use this new API?

This was the only place I found using createHostObject.

The only objects that are currently transferable are MessagePort and ArrayBuffer (for which v8::ValueSerializer::Delegate has separate methods), see:

fn serialize_transferables(
state: &mut OpState,
transferables: Vec<Transferable>,
) -> Vec<JsTransferable> {
let mut js_transferables = Vec::with_capacity(transferables.len());
for transferable in transferables {
match transferable {
Transferable::MessagePort(port) => {
let rid = state.resource_table.add(MessagePortResource {
port,
cancel: CancelHandle::new(),
});
js_transferables.push(JsTransferable::MessagePort(rid));
}
Transferable::ArrayBuffer(id) => {
js_transferables.push(JsTransferable::ArrayBuffer(id));
}
}
}
js_transferables
}

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;
Copy link
Contributor

@andreubotella andreubotella Nov 30, 2023

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?

Copy link
Contributor Author

@lrowe lrowe Nov 30, 2023

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.

Copy link
Contributor

@andreubotella andreubotella Nov 30, 2023

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.

Copy link
Contributor

@mmastrac mmastrac left a 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.

@mmastrac mmastrac merged commit 8050cbf into denoland:main Nov 30, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants