852143 - [OS.File] Writing strings efficiently

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

At the moment, to write strings with OS.File, the "standard" approach is the following:

let s = somePossiblyLongString;
let encoder = new TextEncoder(); // We generally want to write files as UTF-8
let encoded = encoder.encode(s); // Long conversion from UTF-16 to UTF-8, blocks the main thread
OS.File.writeAtomic(encoded); // or myFile.write(encoded)

Now, |encoder.encode| is a costly operation that we would prefer doing off the main thread.

Now, if we let |OS.File.writeAtomic| perform the encoding, we might be able to gain performance.

let s = somePossiblyLongString;
OS.File.writeAtomic(s, {encoding: "utf-8"}); // Copies the string to the worker thread

The objective of this bug is to:
- write the code;
- benchmark it, to determine whether it is useful.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Writing strings efficiently

Telemetry upon bug 852452 will be able to tell us whether this patch improves performance.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 726653 [details] [diff] [review]
Writing strings efficiently

Review of attachment 726653 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.  I assume you're going to benchmark this before actually landing it?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I plan to do some benchmarking, yes. However, I suspect that this will be much less useful than Telemetry.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Writing strings efficiently

Same patch, minus bitrot - and minus the SessionStore stuff that should certainly go into another bug.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Early benchmarking seems to indicate that sending as a string is longer than encoding the string and transferring the buffer.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

More benchmarking seems to contradict first tests (see bug 852187). Transferring as string is a little faster than encoding as string. Given that bug 852187 should further improve the speed of this patch, let's land it.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6fba6c2878

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is it bad that I even said to myself "No Try push, what could possibly go wrong?" before landing this and bug 854350? Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3afa561d8af

https://tbpl.mozilla.org/php/getParsedLog.php?id=21161749&tree=Mozilla-Inbound

08:39:57     INFO -  14278 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | read_write: Uncaught error [object WorkerErrorEvent]
08:39:58     INFO -  14528 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_async.xul | position: Uncaught error [object WorkerErrorEvent]

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=aa5786f761de

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I'm almost innocent :)
Giving a hand for debugging the patch for Bug 854350.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/685c53d8b16e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/685c53d8b16e