Closed Bug 1514664 Opened 6 years ago Closed 6 years ago

Implement TextEncoder.encodeInto() for faster JS/Wasm interop

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Manual WPT sync patch, v2
(deleted), patch
hsivonen
: review+
Details | Diff | Splinter Review
Implement encodeInto
(deleted), patch
emk
: review+
Details | Diff | Splinter Review
TextEncoder.encode() returns a DOM implementation-created buffer. This involves coping internally in the implementation (in principle, this copy could be optimized away with internal-only API changes) and then yet another copy from the returned buffer into Wasm memory (optimizing away this copy needs a Web-visible change anyway). TextEncoder.encodeInto() avoids both copies. https://bugzilla.mozilla.org/show_bug.cgi?id=1449849 combined with TextEncoder.encodeInto() is expected to avoid yet more copying. The expectation is that passing strings from JS to Wasm is / is going to be common enough to be worthwhile to optimize.
Method needs adding to MDN.
Keywords: dev-doc-needed
The patch requires some encoding_rs-side changes to how closely encoding_rs fills the available output space.
Attached patch Manual WPT sync patch (obsolete) (deleted) — — Splinter Review

Test by annevk. r=hsivonen.

Attachment #9035561 - Flags: review+

Comment on attachment 9035561 [details] [diff] [review]
Manual WPT sync patch

Oops. Wrong diff.

Attachment #9035561 - Attachment is obsolete: true
Attachment #9035561 - Flags: review+
Attachment #9031909 - Attachment is obsolete: true

Hmm. Somehow annevk wasn't CCed yet. Anyway, attachment 9035563 [details] [diff] [review] was based on IRC discussion.

Comment on attachment 9035592 [details] [diff] [review] Implement encodeInto Review of attachment 9035592 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/string/nsReadableUtils.h @@ +153,5 @@ > + * The output is always valid UTF-8 ending on scalar value boundary > + * even in the case of partial conversion. > + * > + * Returns the number of code units read and the number of code > + * units written written. nit: fix typo
Attachment #9035592 - Flags: review?(VYV03354) → review+

Thanks for the r+. Typo fixed locally. Trying Mac WPT once more with a different base revision before pushing for real:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a33bf4bd42008da76126847476262013cb9c86

Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ecf49bb4c0 Implement TextEncoder.encodeInto(). r=emk. https://hg.mozilla.org/integration/mozilla-inbound/rev/3c73631c8237 test - Manually sync encoding/encodeInto.any.js from WPT. r=hsivonen.

https://hg.mozilla.org/mozilla-central/rev/f7ecf49bb4c0
https://hg.mozilla.org/mozilla-central/rev/3c73631c8237

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Note to MDN team — I've added a note about this update to the 66 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66#APIs

When actually documenting this, you'll need to add a page for the new method, but you'll also want to consider if we need to talk about it somewhere suitable in the WebAssembly docs.

I've documented this:

Page added for encodeInto():
https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder#Methods
https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder/encodeInto

PR filed to add compat data entry for it:
https://github.com/mdn/browser-compat-data/pull/3581

I'm leaving WebAssembly doc additions for now, as we are aiming to do an in-depth update later on in the year anyway.

Let me know if that's OK. Thanks!

Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: