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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Nov 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift click to select a range
3850051
perf(ext/web): Avoid changing prototype by setting hostObjectBrand di…
lrowe Nov 27, 2023
c745aad
TMP: Use https://github.com/denoland/deno_core/pull/207
lrowe Nov 28, 2023
08b3050
Use deno_core 0.234.0
mmastrac Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 40,7 @@ repository = "https://github.com/denoland/deno"

[workspace.dependencies]
deno_ast = { version = "0.31.6", features = ["transpiling"] }
deno_core = { version = "0.233.0" }
deno_core = { version = "0.234.0" }

deno_runtime = { version = "0.133.0", path = "./runtime" }
napi_sym = { version = "0.55.0", path = "./cli/napi/sym" }
Expand Down
6 changes: 2 additions & 4 deletions ext/web/13_message_port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 26,6 @@ const {
ArrayPrototypeIncludes,
ArrayPrototypePush,
ObjectPrototypeIsPrototypeOf,
ObjectSetPrototypeOf,
Symbol,
SymbolFor,
SymbolIterator,
Expand Down Expand Up @@ -84,9 83,8 @@ const _enabled = Symbol("enabled");
* @returns {MessagePort}
*/
function createMessagePort(id) {
const port = core.createHostObject();
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

setEventTargetData(port);
port[_id] = id;
return port;
Expand Down