1879143 - Improve atom caches in zones

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Justin Link]

When atomizing a string, before looking in the permanent atoms or checking the master list of atoms to see if that string already has an atom, we first check a cache in the zone.

The purpose of a cache is to be faster by working with a reduced set that is going to contain most of what you need. In this case, each zone keeps a cache of all of the string (with Atoms) that is has seen up to that point (and since the last GC). The data structure currently used by the Zone for this cache, the AtomSet, is a general-purpose data structure that can grow arbitrarily large and is, in fact, the same as the data structure used for the master list. In some reasonable cases, these caches are not adding much value but are just introducing additional cost to maintain.

As an example, during browser start-up, the master list grows to over 200,000 entries and one zone has a cache that is likewise growing to that size and is essentially just a duplicate of the master list. Having a cache that is a duplicate of the master list is not adding any value but is adding overhead to maintain.

The purpose of this bug is to replace the atom cache used by zones with something more appropriate.

Comment 1

[Justin Link]

Attachment: Bug 1879143: Implemented new atom cache in zones. r=arai,jonco

Comment 2

[Justin Link]

I have seen some pretty good improvements with this change. Here is a comparison report that shows pretty strong results on a good number of subtests.

Comment 3

[Justin Link]

Attachment: Zone-cache statistics from opening browser and leaving on empty tab

Comment 4

[Justin Link]

Attachment: Zone-cache statistics from when SP3 test suite is running

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c0de707e2a4
Implemented new atom cache in zones. r=arai,jonco

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7c0de707e2a4

Comment 7

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c0de707e2a4
Implemented new atom cache in zones. r=arai,jonco

== Change summary for alert #41941 (as of Thu, 21 Mar 2024 20:20:20 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	perf_reftest_singletons display-none-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	0.54 -> 0.49
8%	perf_reftest_singletons only-children-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	0.96 -> 0.88
7%	perf_reftest_singletons only-children-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	0.61 -> 0.56
7%	perf_reftest_singletons slow-selector-1.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	0.60 -> 0.56
7%	perf_reftest_singletons display-none-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	0.52 -> 0.49
...	...	...	...	...
5%	perf_reftest_singletons only-children-1.html	windows10-64-shippable-qr	e10s fission stylo webrender	0.59 -> 0.56

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41941

Comment 8

[Mayank Bansal]

Hi Justin,

Curiois, Are these improvements expected? These tests measure layout/style type tests and naively it appears this patch is for js improvements. (All improvements are very welcome, of course :))
Cc :emilio in case he has any opinions.

Comment 9

[Justin Link]

Hi Mayank,

I wouldn't say that they were expected to me, but there are a lot more interactions between pieces than I am aware of so I also expect to be surprised. :)

I brought this up in a meeting with the SpiderMonkey folks this morning and the expectation was that there was probably still be some JavaScript running in these tests. There is also some precedent for a JS/string-related change to affect these tests (see bug 1858040 for example) so it's probably not totally crazy for these tests to have been affected.

Comment 10

[Markus Stange [:mstange]]

I'd like to get to a point where our answer to this type of question is "let's look at the before/after profiles and find out!" However, the profiles we collect for this type of test in CI don't seem very useful; this "before" profile has only 36 samples: https://share.firefox.dev/3TZ0UPK

Comment 11

[Mayank Bansal]

This bug improved bug 1842427 by 28% and bug 1830831 by 30%

Comment 12

[Justin Link]

Thanks for noticing this Mayank! This is great to hear, and exactly the kind of thing that I was hoping that we'd find over time.

I have some further atomization improvements in progress (see bug 1893690 and I'll take a look at these tests to see how they're impacted by them as well.