1880044 - Support allocating flattened rope characters in the nursery

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jan de Mooij [:jandem]]

On a full Speedometer 3 run we call malloc about 200,000 times for rope flattening. Most of these (> 93% according to some quick-and-dirty logging) could instead allocate the buffer in the nursery.

This should be doable now after bug 1853907. I have a WIP patch that passes most tests, but it needs bug 1879918 to be fixed first.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1880044 part 1 - Remove special case for extensible strings in JSString::sizeOfExcludingThis. r?sfink!

This was added when we calculated the size manually, but with mallocSizeOf
we don't need special handling for extensible strings.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1880044 part 2 - Support extensible strings with a nursery buffer in maybeMallocCharsOnPromotion. r?sfink!

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1880044 part 3 - Fix some stale comments. r?sfink!

Also fixes/deletes some outdated comments.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1880044 part 4 - Support allocating flattened rope characters in the nursery. r?sfink!

Now that strings can have nursery-allocated chars, we can also use this when
flattening nursery-allocated ropes.

Comment 6

[Jan de Mooij [:jandem]]

Perf comparison: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=9176283ad4327fba0c64cf49d2890f7109bed210&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=355e7ae29a9ac994224f0fd963af5de86ebfdda6&page=1

Improves TodoMVC-JavaScript-ES5/Adding100Items/Sync by about 3.6%. Looking at the latest profile from Markus for this test, I do see some rope flattening time (under DOMParser.parseFromString and Template.prototype.show).

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jandem, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 8

[Jan de Mooij [:jandem]]

Waiting for some other bugs to land first.

Comment 9

[Iain Ireland [:iain]]

Can this land now?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #9)

Can this land now?

I have to rebase it on top of bug 1914378 which changes the same code.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/971125148077
part 1 - Remove special case for extensible strings in JSString::sizeOfExcludingThis. r=sfink

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d9e868b2be1
part 2 - Support extensible strings with a nursery buffer in maybeMallocCharsOnPromotion. r=sfink
https://hg.mozilla.org/integration/autoland/rev/fe9089be9484
part 3 - Fix some stale comments. r=sfink
https://hg.mozilla.org/integration/autoland/rev/ca8948ee1956
part 4 - Support allocating flattened rope characters in the nursery. r=sfink

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8d9e868b2be1
https://hg.mozilla.org/mozilla-central/rev/fe9089be9484
https://hg.mozilla.org/mozilla-central/rev/ca8948ee1956

Comment 15

[Mayank Bansal]

4.4% improvement on Jetstream2-offlineassembler

Comment 16

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #13)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d9e868b2be1
part 2 - Support extensible strings with a nursery buffer in
maybeMallocCharsOnPromotion. r=sfink
https://hg.mozilla.org/integration/autoland/rev/fe9089be9484
part 3 - Fix some stale comments. r=sfink
https://hg.mozilla.org/integration/autoland/rev/ca8948ee1956
part 4 - Support allocating flattened rope characters in the nursery. r=sfink

Perfherder has detected a browsertime performance change from push ca8948ee1956f9f88294c6e0b815a2c5ba8f861e.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
10%	speedometer Flight-TodoMVC/CompletingAllItems/Sync	android-hw-a55-14-0-aarch64-shippable	webrender	55.79 -> 50.18
10%	speedometer Flight-TodoMVC/CompletingAllItems/Sync	windows11-64-shippable-qr	fission webrender	14.90 -> 13.42
10%	speedometer Flight-TodoMVC/CompletingAllItems	windows11-64-shippable-qr	fission webrender	16.74 -> 15.13
9%	speedometer Flight-TodoMVC/CompletingAllItems/Sync	android-hw-a55-14-0-aarch64-shippable	fission webrender	55.96 -> 50.90
9%	speedometer Flight-TodoMVC/CompletingAllItems	android-hw-a55-14-0-aarch64-shippable	webrender	61.84 -> 56.26
...	...	...	...	...	...
2%	speedometer EmberJS-Debug-TodoMVC/DeletingItems/Sync	windows11-64-shippable-qr	fission webrender	101.20 -> 99.16

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43338

For more information on performance sheriffing please see our FAQ.

Comment 17

[Florin Bilt]

Perfherder has detected a devtools performance change from push ca8948ee1956f9f88294c6e0b815a2c5ba8f861e.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	damp console.log-in-loop-content-process-longstring	windows11-64-shippable-qr	e10s fission stylo webrender	31.26 -> 28.68
5%	damp console.log-in-loop-content-process-node	macosx1470-64-shippable	e10s fission stylo webrender	235.87 -> 224.42

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43334

For more information on performance sheriffing please see our FAQ.

Comment 18

[Alex Finder|PTO, back on March 31st]

(In reply to Pulsebot from comment #13)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d9e868b2be1
part 2 - Support extensible strings with a nursery buffer in
maybeMallocCharsOnPromotion. r=sfink
https://hg.mozilla.org/integration/autoland/rev/fe9089be9484
part 3 - Fix some stale comments. r=sfink
https://hg.mozilla.org/integration/autoland/rev/ca8948ee1956
part 4 - Support allocating flattened rope characters in the nursery. r=sfink

Perfherder has detected a talos performance change from push ca8948ee1956f9f88294c6e0b815a2c5ba8f861e.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	perf_reftest_singletons many-custom-props.html	macosx1470-64-shippable	e10s fission stylo webrender	262.29 -> 244.55
5%	perf_reftest_singletons many-custom-props.html	windows11-64-shippable-qr	e10s fission stylo webrender	148.02 -> 140.06
5%	perf_reftest_singletons many-custom-props.html	linux1804-64-shippable-qr	e10s fission stylo webrender	320.50 -> 303.86
5%	perf_reftest_singletons many-custom-props.html	linux1804-64-shippable-qr	e10s fission stylo webrender	321.54 -> 305.10

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43341

For more information on performance sheriffing please see our FAQ.