1843946 - Improve attribute storage of class attributes with multiple class names

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Markus Stange [:mstange]]

We can improve innerHTML performance on content which contains elements with multiple class names, i.e. class="one two three four", by making the MiscContainer of eAtomArray attributes refcounted, and by caching these MiscContainers globally. This optimization mirrors Chrome's SpaceSplitString optimization with its global cache. This improves innerHTML times on Speedometer 3.

Here's a profile with current Firefox Nightly, of 33 iterations of the TodoMVC-jQuery subtest, focused on nsHtml5TreeOperation::SetHTMLElementAttributes: https://share.firefox.dev/3Dk2cvg

I noticed these things in the profile:

• Inside Element::SetAttrAndNotify, a lot of time is taken up by valueForAfterSetAttr, specifically by allocating and freeing the copy of the atom array (inside SetTo and ~nsAttrValue).
• We spend a fair amount of time in ParseAttribute allocating atom arrays and atomizing strings.

Making the MiscContainer for eAtomArray refcounted makes the copy basically free, so it eliminates the time in SetTo and ~nsAttrValue. And by having a global cache we can reduce the time we spend atomizing individual class names. We'll just be atomizing the full attribute value, in order to have a faster cache lookup and in order to avoid a string copy. And by sharing the atom array between elements, we reduce the number of atom array allocations.

If copying the class attribute is cheaper, this also speeds up ServoElementSnapshot::AddAttrs, which is called when somebody mutates the classList of an element.

Comment 1

[Markus Stange [:mstange]]

And we might even be able to speed up selector matching for class selectors, by storing the class attribute value next to the regular attributes, so that we don't have to walk the attribute array in order to find the class names. The fast-access copy will share the same MiscContainer with the nsAttrValue inside the regular attribute array.

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1843946 - Make the MiscContainer for eAtomArray refcounted. r=emilio

This makes it cheaper to copy the nsAttrValue, which improves innerHTML times on Speedometer
because it reduces the time spent creating and destroying valueForAfterSetAttr.

It will also make it possible to reuse eAtomArray values across elements.

Before: https://share.firefox.dev/3rxCO2G
After: https://share.firefox.dev/3PXSN43

Depends on D183809

Comment 3

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1843946 - Add an eAtomArrayPreserveDuplicates type, as a first step towards sharing of the ones that don't preserve duplicates.

Sharing the same AttrAtomArray among all elements with the same attribute value
is problematic because it has a mutating RemoveDuplicates() method which is
called when a DOMTokenList for that attribute is used. What we don't want to
have happen is that one element uses autocomplete="email email" and another
element uses class="email email", queries el.classList.length, and ends up
affecting the first element's .autocomplete property. So we need to have a
distinction between duplicate-preserving atom arrays and
opportunistically-deduplicating atom arrays. Alternatively, we could consider
having an eAtomArrayForClassAttr type. I'm not sure what the best solution is.

Depends on D183810

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1843946 - Cache atom arrays. r=emilio

This adds a global cache for eAtomArray MiscContainers. This avoids
allocations and atomization cost for the individual class names for
class attributes with multiple space-separated values.

It also makes all elements with the same class attribute value share
a single MiscContainer, which may reduce memory on some pages.

In this implementation, the cache lookup is done based on a string.
To avoid the string hashing + string comparison, a future patch will
change the cache key to be an nsAtom.

Depends on D183811

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1843946 - Use an atom for the full class attribute value, to reduce string hash key cost during the AtomArrayCache lookup. r=emilio

Depends on D183812

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1843946 - Make a new copy of the MiscContainer + AttrAtomArray when deduplicating. r=emilio

Depends on D183810

Comment 7

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=94fc222ef27ca5feb8811fc5a331478754cc96c2

Comment 8

[Markus Stange [:mstange]]

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

Comment 9

[Markus Stange [:mstange]]

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

Comment 10

[Markus Stange [:mstange]]

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

For future reference, these are good tests to run when making changes to this code:
dom/base/test/test_bug254337.html
dom/base/test/test_bug395915.html
devtools/client/inspector/rules/test/browser_rules_class_panel_add.js

Comment 11

[Markus Stange [:mstange]]

5% improvements on TodoMVC-JavaScript-ES5, TodoMVC-JavaScript-ES6-Webpack and TodoMVC-jQuery:

https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=d267f7e3eae657a4fc9b8dddfbcf96ad8094ce15&originalSignature=4569240&newSignature=4569240&framework=13&application=firefox&originalRevision=a56febf9f6112dfef2f25ef6b42e718f097b676d&page=1&replicates=1&showOnlyConfident=1

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2a667056201
Make a new copy of the MiscContainer + AttrAtomArray when deduplicating. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7157792180bf
Make the MiscContainer for eAtomArray refcounted. r=emilio
https://hg.mozilla.org/integration/autoland/rev/70572f7b179d
Cache atom arrays. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e03be7f97ff2
Use an atom for the full class attribute value, to reduce string hash key cost during the AtomArrayCache lookup. r=emilio

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/c2a667056201
https://hg.mozilla.org/mozilla-central/rev/7157792180bf
https://hg.mozilla.org/mozilla-central/rev/70572f7b179d
https://hg.mozilla.org/mozilla-central/rev/e03be7f97ff2

Comment 14

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #12)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2a667056201
Make a new copy of the MiscContainer + AttrAtomArray when deduplicating.
r=emilio
https://hg.mozilla.org/integration/autoland/rev/7157792180bf
Make the MiscContainer for eAtomArray refcounted. r=emilio
https://hg.mozilla.org/integration/autoland/rev/70572f7b179d
Cache atom arrays. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e03be7f97ff2
Use an atom for the full class attribute value, to reduce string hash key
cost during the AtomArrayCache lookup. r=emilio

== Change summary for alert #39383 (as of Fri, 25 Aug 2023 10:06:00 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
2%	speedometer3	windows10-64-nightlyasrelease-qr	fission webrender	12.46 -> 12.75	Before/After
1%	speedometer3	windows10-64-nightlyasrelease-qr	fission webrender	12.57 -> 12.68	Before/After

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