Skip to content

perf(web): optimize single pass utf8 decoding#16593

Merged
littledivy merged 6 commits intodenoland:mainfrom
littledivy:single_pass_utf8_decode
Nov 11, 2022
Merged

perf(web): optimize single pass utf8 decoding#16593
littledivy merged 6 commits intodenoland:mainfrom
littledivy:single_pass_utf8_decode

Conversation

@littledivy
Copy link
Copy Markdown
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 thread ext/web/08_text_encoding.js Outdated
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
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 thread ext/web/08_text_encoding.js Outdated
Comment thread ext/web/lib.rs
Comment thread ext/web/lib.rs
Comment thread ops/lib.rs Outdated
Comment thread ext/web/lib.rs
Comment on lines +121 to +123
input.buffer,
input.byteOffset,
input.byteLength,
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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 thread ops/lib.rs Outdated
Comment thread ops/lib.rs Outdated
Comment on lines 110 to 111
ObjectPrototypeIsPrototypeOf(
SharedArrayBuffer.prototype,
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

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