Closed Bug 1514664 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
Depends on: 1515351
Attached patch Manual WPT sync patch (obsolete) — 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 #9035563 - 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.

Attachment #9035592 - Flags: review?(VYV03354)
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
See Also: → 1519523
See Also: 1519523

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!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: