1953167 - Experiment with changing the JS holders map to make it faster to add and drop holders

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Jon Coppeard (:jonco)]

The patch in bug 1903034 gave some significant improvements for promise-heavy workloads (e.g. it improved our doxbee-async score by 30% and our doxbee-promise score by 50%). However it was backed out due to causing some speedometer3 regressions (see bug 1904810), likely due to the overhead of removing entries from the JS holders map.

This bug is about exploring possible changes to that JS holders map data structure to make adding and removing entries more efficient. This would also hopefully allow the original optimization to land.

The idea is to store a the holders in a vector and store the holder's location in the vector in the holder itself, removing the need for hash table lookup on add and remove.

Comment 1

[Jan de Mooij [:jandem]]

Bug 1950952 comment 7 has a similar idea for improving the JSHolderMap data structure. I started looking into that when Jon told me he had done some work on that already :)

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1953167 - Part 1: Add mTraceState to make incremental marking state more explicit r?mccr8

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1953167 - Part 2: Move WhichHolders out of JSHolderMap r?mccr8

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1953167 - Part 3: Add JSHolderList container that doesn't use a hash table r?mccr8

Comment 5

[Jon Coppeard (:jonco)]

Attachment: WIP: Bug 1953167 - Part 4: Implement JSHolderList and remove hash tables

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1953167 - Part 4: Use new holder APIs for CallbackObject r?mccr8

Comment 7

[Jan de Mooij [:jandem]]

Mayank tested a build with these patches and it's a significant speedup for some promise micro-benchmarks (eg bug 1949221 comment 8).

Comment 8

[Jon Coppeard (:jonco)]

Performance comparison here: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=e66e597bbcc792a7f3dae139a188803f7a59d052&newProject=try&newRevision=acfd8cc679018be1b695f0778cea9c8822ba33af&framework=13&page=1

This shows significant improvement in the jetstream2 async-fs benchmark and maybe some ~2% improvements on a couple of sp3 subtests.

Comment 9

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

This is super interesting. And something I've been pondering quite a bit recently.
The idea here is that some objects could be holders themselves, and some still use the old mechanism?
(like, Nodes likely should use the old thingie, so that their sizeof doesn't increase.)

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #9)

The idea here is that some objects could be holders themselves, and some still use the old mechanism?
(like, Nodes likely should use the old thingie, so that their sizeof doesn't increase.)

Yes. I wrote this specifically for this case but with the idea that it could be applied to other kinds of object too. It does increases storage size so it's not good for small objects that are unlikely to be in the holders map. It should be a win for objects that we always have to put in the map. What do you think, are there other classes of object that this would be useful for?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea7ce945b9a3
Part 1: Add mTraceState to make incremental marking state more explicit r=mccr8
https://hg.mozilla.org/integration/autoland/rev/c898b4373f41
Part 2: Move WhichHolders out of JSHolderMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/553a0a553740
Part 3: Add JSHolderList container that doesn't use a hash table r=mccr8
https://hg.mozilla.org/integration/autoland/rev/22622b9ad46c
Part 4: Use new holder APIs for CallbackObject r=mccr8

Comment 12

[Serban Stanca [:SerbanS]]

Backed out as requested.

• Backout link

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/859da4ff3831
Part 1: Add mTraceState to make incremental marking state more explicit r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4017f4c1bf2d
Part 2: Move WhichHolders out of JSHolderMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/d930a43e130e
Part 3: Add JSHolderList container that doesn't use a hash table r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6f6ae326dde1
Part 4: Use new holder APIs for CallbackObject r=mccr8

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/859da4ff3831
https://hg.mozilla.org/mozilla-central/rev/4017f4c1bf2d
https://hg.mozilla.org/mozilla-central/rev/d930a43e130e
https://hg.mozilla.org/mozilla-central/rev/6f6ae326dde1

Comment 15

[Mayank Bansal]

11% improvement on Jetstream2-async-fs-average
1%-2% improvement on Jetstream2-offline assembler

Comment 17

[Florin Bilt]

(In reply to Pulsebot from comment #13)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/859da4ff3831
Part 1: Add mTraceState to make incremental marking state more explicit
r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4017f4c1bf2d
Part 2: Move WhichHolders out of JSHolderMap r=mccr8
https://hg.mozilla.org/integration/autoland/rev/d930a43e130e
Part 3: Add JSHolderList container that doesn't use a hash table r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6f6ae326dde1
Part 4: Use new holder APIs for CallbackObject r=mccr8

Perfherder has detected a browsertime performance change from push 6f6ae326dde1952dcaaee9679142bdc31060f5df.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
9%	speedometer Inferno-TodoMVC/DeletingItems/Async	windows11-64-24h2-nightlyasrelease	fission webrender	0.43 -> 0.40
6%	speedometer Inferno-TodoMVC/DeletingItems/Sync	windows11-64-24h2-nightlyasrelease	fission webrender	33.32 -> 31.25
6%	speedometer Inferno-TodoMVC/DeletingItems	windows11-64-24h2-nightlyasrelease	fission webrender	33.81 -> 31.74
5%	speedometer Inferno-TodoMVC/DeletingItems/Sync	windows11-64-24h2-shippable	fission webrender	33.87 -> 32.03	Before/After
5%	speedometer Inferno-TodoMVC/DeletingItems	windows11-64-24h2-shippable	fission webrender	34.33 -> 32.59	Before/After
...	...	...	...	...	...
3%	speedometer Inferno-TodoMVC	macosx1400-64-shippable-qr	fission webrender	39.42 -> 38.36	Before/After

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 performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 44580

The following documentation link provides more information about this command.

Comment 18

[Florin Bilt]

(In reply to Cristian Tuns from comment #14)

https://hg.mozilla.org/mozilla-central/rev/859da4ff3831
https://hg.mozilla.org/mozilla-central/rev/4017f4c1bf2d
https://hg.mozilla.org/mozilla-central/rev/d930a43e130e
https://hg.mozilla.org/mozilla-central/rev/6f6ae326dde1

Perfherder has detected a devtools performance change from push 6f6ae326dde1952dcaaee9679142bdc31060f5df.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
2%	damp custom.jsdebugger.stepInNewSource.DAMP	windows11-64-24h2-shippable	e10s fission stylo webrender	879.60 -> 858.07
2%	damp custom.jsdebugger.stepIn.DAMP	windows11-64-24h2-shippable	e10s fission stylo webrender	1,417.62 -> 1,385.82
2%	damp custom.jsdebugger.stepIn.big-bundle.DAMP	windows11-64-24h2-shippable	e10s fission stylo webrender	1,493.83 -> 1,462.22

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 performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 44599

The following documentation link provides more information about this command.