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(web): optimize single pass utf8 decoding #16593

Merged
merged 6 commits into from Nov 11, 2022

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
Contributor

@vimirage vimirage 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

ext/web/08_text_encoding.js Outdated Show resolved Hide resolved
ext/web/lib.rs Show resolved Hide resolved
ops/lib.rs Outdated Show resolved Hide resolved
Comment on lines +121 to +123
input.buffer,
input.byteOffset,
input.byteLength,
Copy link
Contributor

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
Contributor

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

ops/lib.rs Outdated Show resolved Hide resolved
ops/lib.rs Outdated Show resolved Hide resolved
Comment on lines 110 to 111
ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
Copy link
Contributor

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
Contributor

@vimirage vimirage 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

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

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.

None yet

4 participants