1484938 - Implementing Append() via Replace() leads to cache-unfriendly access pattern

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Currently, Append() (and AppendASCII(), AppendLiteral(), etc.) are this wrappers around Replace() (and ReplaceASCII(), ReplaceLiteral(), etc.) with a zero-length cut that starts at the end of the string.

While this leads to the correct observable result, it is done with writes ordered in a cache-unfriendly way: The zero terminator is written first and then the newly-appended data is written between the old end of the string and the zero terminator.

Implementing Append() (etc.) directly in terms of StartBulkWriteImpl() (or special-casing zero-length cut at the end of the string in Replace() to use StartBulkWriteImpl() directly and not via MutatePrep()) would result in a linear write pattern, which should be more cache-friendly.

(I expect the difference to be small enough not to be easily quantifiable but to add up still.)

Comment 1

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> MutatePrep()

s/MutatePrep/ReplacePrep/

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1484938 - Make XPCOM string appends and copying assignments write the terminator after writing content.

MozReview-Commit-ID: DOmAitH5YDh

Comment 3

[Henri Sivonen (:hsivonen)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8111dfc75c09d3b1adcb0162de0cd6c6c25a8fd

(The riskiest part is removing the "-1 means 'use strlen()'" behavior.)

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #3)
> (The riskiest part is removing the "-1 means 'use strlen()'" behavior.)

Removing that behavior is futile.

Another try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74dec051de3a2d5c1ca39bfc24c52da098d77743

Comment 5

[Henri Sivonen (:hsivonen)]

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

Comment 6

[Henri Sivonen (:hsivonen)]

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

Comment 7

[Henri Sivonen (:hsivonen)]

Microbenchmarks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=083dbd7e9a0972f6947457d85bfa1ead973bc8b2

Comment 8

[Henri Sivonen (:hsivonen)]

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

Comment 9

[Henri Sivonen (:hsivonen)]

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

Comment 10

[Henri Sivonen (:hsivonen)]

Try run with the Oculus loader patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a4c319ee749ab9b3d88eae5520d9052f56cfa7

Comment 11

[Henri Sivonen (:hsivonen)]

Cross-platform crash around layout/style/test/test_value_cloning.html on try. :-(

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #11)
> Cross-platform crash around layout/style/test/test_value_cloning.html on
> try. :-(

On closer look, it seems it's a timeout and the crash comes from the harness forcibly stopping the browser.

Let's see if the timeout goes away by rebasing, i.e. if I had an unlucky base revision:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4de43504dcd4a93551735f77d742c05b3157e3f

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Let's see if the timeout goes away by rebasing

It doesn't.

Comment 14

[Henri Sivonen (:hsivonen)]

The timeout is due to bad interaction of bug 1486711 and NS_UnescapeURL.

The test case uses exceptionally large percent-escaped data: URLs. NS_UnescapeURL first calls SetCapacity() with a large capacity and then calls single-char Append() very often. This results in a huge amount of time getting spent in memset() poisoning the buffer from after the current length to the capacity.

Probably the number of bytes to poison should be capped. Also, NS_UnescapeURL should probably use lower-level writing than using single-char Append() often.

Comment 15

[Henri Sivonen (:hsivonen)]

With fix for bug 1490973:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=176c1ab6a64dd5a3917f1624b71f213b58f02801

(Also filed bug 1490972.)

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> With fix for bug 1490973:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=176c1ab6a64dd5a3917f1624b71f213b58f02801

Still times out. :-(

Comment 17

[Henri Sivonen (:hsivonen)]

With fix for bug 1490973 *and* bug 1490972:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62207baefc3135dd3a8c6b15320cb800a8db5a19

Comment 18

[Henri Sivonen (:hsivonen)]

Microbenchmarks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d96145e4ee52186d1dc0c6323ed2b5e1d11c889

Comment 19

[Henri Sivonen (:hsivonen)]

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

Comment 20

[Nathan Froyd [:froydnj]]

Comment on attachment 9005982 [details]
Bug 1484938 - Make XPCOM string appends and copying assignments write the terminator after writing content.

Nathan Froyd [:froydnj] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00df8305ed36
Make XPCOM string appends and copying assignments write the terminator after writing content. r=froydnj

Comment 22

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/00df8305ed36