Closed Bug 1488697 Opened 2 years ago Closed 2 years ago
Builder::To String should use Bulk Write
Bug 1488697 - Make StringBuilder::ToString avoid repeated capacity and aliasing checks as well as terminator writes.
46 bytes, text/x-phabricator-request
|Details | Review|
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?
Look at the blame of StringBuilder. It was added because of performance.
https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c10 has a link to at least some testcase.
Opt builds, now with more Windows wchar_t* trickery: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2be95e25ac4c31cafd848eef6188144757494a1
Opt builds, which #include for the Windows sadness: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7f2673b8c8dd01c8bd3910295102d089b5abf1
Opt builds, still trying to get Windows to behave: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e539b2ebc7a6ec6b312b47f0ec6e048f53eadb6
(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 email@example.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
Consider adding the `perf` keyword here?
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.
You need to log in before you can comment on or make changes to this bug.