1488697 - StringBuilder::ToString should use BulkWrite

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

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.

Comment 1

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b38e73af08187a45386372866619814d66bcf6f8

Comment 2

[Henri Sivonen (:hsivonen)]

smaug, can you recommend a benchmark that tests the innerHTML getter?

Comment 3

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a4070fdb7a770ac3eef95b40be9c1a2d90cc62

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Look at the blame of StringBuilder. It was added because of performance.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c10 has a link to at least some testcase.

Comment 6

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe5039fc70c4cd69a9a3d9be14852422d60b327

Comment 7

[Henri Sivonen (:hsivonen)]

Opt builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99471e33c9e6fa97e63e9eabfb947d411ad91b09

Comment 8

[Henri Sivonen (:hsivonen)]

Opt builds, now with more Windows wchar_t* trickery:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2be95e25ac4c31cafd848eef6188144757494a1

Comment 9

[Henri Sivonen (:hsivonen)]

Opt builds, which #include for the Windows sadness:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7f2673b8c8dd01c8bd3910295102d089b5abf1

Comment 10

[Henri Sivonen (:hsivonen)]

Opt builds, still trying to get Windows to behave:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e539b2ebc7a6ec6b312b47f0ec6e048f53eadb6

Comment 11

[Henri Sivonen (:hsivonen)]

(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.)

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1488697 - Make StringBuilder::ToString avoid repeated capacity and aliasing checks as well as terminator writes.

* 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 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f00e66757346
Make StringBuilder::ToString avoid repeated capacity and aliasing checks as well as terminator writes. r=smaug

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f00e66757346

Comment 16

[Jens Hausdorf :jens1o [ni? me]]

Consider adding the `perf` keyword here?

Comment 17

[Henri Sivonen (:hsivonen)]

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.