Closed Bug 852143 Opened 10 years ago Closed 10 years ago

[OS.File] Writing strings efficiently


(Toolkit :: OS.File, defect)

Not set





(Reporter: Yoric, Assigned: Yoric)


(Keywords: perf)


(1 file, 1 obsolete file)

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.
Attached patch Writing strings efficiently (obsolete) — Splinter Review
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.
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.

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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.