Closed Bug 1488697 Opened 2 years ago Closed 2 years ago

StringBuilder::ToString should use BulkWrite

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

StringBuider ensures that the appends it does can't fail. It also knows that what it what it appends can't alias the buffer of the string. Yet, on each append (including on a per-code unit basis!), the string implementation re-checks for these conditions. Also, the zero terminator gets written and rewritten all the time.

Additionally, StringBuider forgoes SIMD acceleration when appending text nodes that contain markup-significant characters.

This can be made more efficient using BulkWrite.
smaug, can you recommend a benchmark that tests the innerHTML getter?
Flags: needinfo?(bugs)
Look at the blame of StringBuilder. It was added because of performance.
Flags: needinfo?(bugs)
Priority: -- → P3
Blocks: 1489036
(In reply to Olli Pettay [:smaug] from comment #5)
> https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c10 has a link to at
> least some testcase.

Thanks. With this patch, I see a 13% execution time reduction for body.innerHTML on that benchmark. Seems like a worthwhile improvement. (Desktop-class Haswell i7, from 182.8 ms to 158.2 ms.)
* Avoid the string implementation's capacity checks, since we know they
   succeed.
 * Avoid the string implementation's aliasing checks, since we know there's no
   aliasing.
 * Avoid writing the zero terminator more than once or out of sequence.
 * Use u"" literals when appending literals to a UTF-16 string.
 * Write runs of non-escaped code units instead of writing code unit by code
   unit in order to benefit from SIMD (either via memcpy or
   ConvertLatin1toUTF16).

This results in a 13% execution time reduction on desktop Haswell i7 when
getting the innerHTML of the body of the Selectors spec. (The WebKit
optimization target from https://bugs.webkit.org/show_bug.cgi?id=81214 .)

MozReview-Commit-ID: LAg3gkGJnpQ
Comment on attachment 9007148 [details]
Bug 1488697 - Make StringBuilder::ToString avoid repeated capacity and aliasing checks as well as terminator writes.

Olli Pettay [:smaug] has approved the revision.
Attachment #9007148 - Flags: review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f00e66757346
Make StringBuilder::ToString avoid repeated capacity and aliasing checks as well as terminator writes. r=smaug
https://hg.mozilla.org/mozilla-central/rev/f00e66757346
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Consider adding the `perf` keyword here?
Keywords: perf
On Haswell desktop i7 on Windows, for the body.innerHTML case, this patch took Firefox from slightly slower than Chrome to slightly faster than Chrome. On a Core2 Duo Mac, Safari is still faster in the body.innerHTML case.

For the div cases, both WebKit and Blink are suspiciously slow compared to Gecko making me wonder if the div cases test a totally different thing in Gecko compared to WebKit/Blink.
Depends on: 1512758
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.