Skip to content

Conversation

@littledivy
Copy link
Member

  • Avoid copying buffers.

https://encoding.spec.whatwg.org/#dom-textdecoder-decode

Implementations are strongly encouraged to use an implementation strategy that avoids this copy. When doing so they will have to make sure that changes to input do not affect future calls to decode().

  • Special op to avoid string label deserialization and parsing. (Ideally we should map labels to integers in JS)
  • Avoid webidl Object.assign when options is undefined.

Comment on lines +95 to +102
let stream = false;
if (options !== undefined) {
options = webidl.converters.TextDecodeOptions(options, {
prefix,
context: "Argument 2",
});
stream = options.stream;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should come up with a general improvement in webidl for cases like this. I think the optimal way would require new Function codegen.

Copy link

@ghost ghost Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, Evan (Discord) had been saying that for over a year. The entire WebIDL bindings setup should be via JiT-compiled codegen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah :(

never too late though

Comment on lines +121 to +123
input.buffer,
input.byteOffset,
input.byteLength,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... now these are all polymorphic...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't this already the case before this patch?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! ...No one fixed it

Comment on lines 110 to 111
ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
ObjectPrototypeIsPrototypeOf(
SharedArrayBufferPrototype,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please defer these primordials changes? I don't want to make the code any slower

Copy link

@ghost ghost Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; I'll introduce these next month, myself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets see what others think. i personally don't like primordials in fast paths for mitigating a hypothetical case. Node.js has also removed primoridals in hot paths previously nodejs/node#38248

ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
input.buffer,
input || input.buffer,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is polymorphic

@littledivy littledivy merged commit 38f0b41 into denoland:main Nov 11, 2022
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Nov 12, 2022
- [x] Avoid copying buffers.

https://encoding.spec.whatwg.org/#dom-textdecoder-decode

> Implementations are strongly encouraged to use an implementation
strategy that avoids this copy. When doing so they will have to make
sure that changes to input do not affect future calls to
[decode()](https://encoding.spec.whatwg.org/#dom-textdecoder-decode).

- [x] Special op to avoid string label deserialization and parsing.
(Ideally we should map labels to integers in JS)
- [x] Avoid webidl `Object.assign` when options is undefined.
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.

3 participants