[OS.File] Writing strings efficiently

RESOLVED FIXED in mozilla22

Status

()

Toolkit
OS.File
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({perf})

Trunk
mozilla22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 726653 [details] [diff] [review]
Writing strings efficiently

Telemetry upon bug 852452 will be able to tell us whether this patch improves performance.
Assignee: nobody → dteller
Attachment #726653 - Flags: review?(nfroyd)
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?
Attachment #726653 - Flags: review?(nfroyd) → review+
I plan to do some benchmarking, yes. However, I suspect that this will be much less useful than Telemetry.
Created attachment 727132 [details] [diff] [review]
Writing strings efficiently

Same patch, minus bitrot - and minus the SessionStore stuff that should certainly go into another bug.
Attachment #726653 - Attachment is obsolete: true
Attachment #727132 - Flags: review+
Early benchmarking seems to indicate that sending as a string is longer than encoding the string and transferring the buffer.
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.
Keywords: checkin-needed
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]
I'm almost innocent :)
Giving a hand for debugging the patch for Bug 854350.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/685c53d8b16e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.