The call in this profile is coming from http://searchfox.org/mozilla-central/rev/990055a4902952e038cc350c9ffe1ac426d1c943/dom/base/Element.cpp#1679-1688 (100% is HTMLInputElement::BindToTree) | | | -nsAttrAndChildArray::DoSetMappedAttrStyleSheet(nsHTMLStyleSheet*) 43.2% 0.0% 144B libxul.so | | | | -nsAttrAndChildArray::MakeMappedUnique(nsMappedAttributes*) 27.3% 0.0% 176B libxul.so | | | | | __GI___pthread_mutex_unlock 2.3% 2.3% 192B libpthread-2.23.so | | | | | -nsHTMLStyleSheet::UniqueMappedAttributes(nsMappedAttributes*) 11.4% 2.3% 96B libxul.so | | | | | | PLDHashEntryHdr* PLDHashTable::SearchTable<(PLDHashTable::SearchReason)1>(void const*, unsigned int) 2.3% 2.3% 304B libxul.so | | | | | | +PLDHashTable::Add(void const*, mozilla::fallible_t const&) 6.8% 0.0% 352B libxul.so | | | | | +nsMappedAttributes::Release() 13.6% 0.0% 64B libxul.so | | | | +nsMappedAttributes::Clone(bool) 13.6% 0.0% 80B libxul.so | | | | +nsMappedAttributes::Release() 2.3% 0.0% 64B libxul.so

bz said he will take a look. We do create a temporary nsMappedAttributes object for each element which has nsMappedAttributes already and which is being added to document. I don't know why we aren't reusing the existing nsMappedAttributes, or could we perhaps not create nsMappedAttributes at all for elements not in document, and only create (or reuse from the stylesheet) when adding to document?

OK, so I looked up how this stuff works now. The basic idea is that the nsMappedAttributes is keyed on the GetComposedDoc() of the element. While that's null, setting a mapped attributes will either allocate an nsMappedAttributes or clone the existing one with one extra slot in it, then add the new attr. When the element has a composed doc, we want to ensure that identical mapped attributes are shared, to reduce the size of the ruletree. This means that attr sets when GetComposedDoc is _not_ null follow their Clone() call or creation, and the ensuing attr set, with a lookup in the nsHTMLStyleSheet hashtable. If that hashtable lookup succeeds, we use the thing in that hashtable an drop our existing nsMappedAttributes. This means that when GetComposedDoc() goes from null to non-null we need to go ahead and look up our thing in the hashtable. If found, we use it. I do see that we do this in a pretty dumb way: we clone it first and _then_ do the hashtable lookup. So one thing worth trying is changing nsAttrAndChildArray::DoSetMappedAttrStyleSheet from doing: RefPtr<nsMappedAttributes> mapped = GetModifiableMapped(nullptr, nullptr, false); to doing: RefPtr<nsMappedAttributes> mapped; if (mImpl->mMappedAttrs->GetStyleSheet()) { mapped = GetModifiableMapped(nullptr, nullptr, false); } else { mapped = mImpl->mMappedAttrs; } (or simular but with less refcounting if we want I guess).

In terms of changing the entire setup around... We _could_ switch to the owner doc, not the composed doc, for getting the nsHTMLStyleSheet. Arguably this would be more correct anyway; right now I expect getComputedStyle doesn't work right with stylo for non-bound-to-tree elements... I'll check that. Anyway, the drawback there would be doing the "make it unique" lookup on every attr set for things that are not in the document. Which may not be a big deal in practice if the common parser case is already doing them on each set anyway. Then we'd just need to reset the nsHTMLStyleSheet pointer on adopt. That may or may not help in this case, though, since presumably in the testcase we'd end up doing the same work but just before the BindToTree() call.

We could add yet another cache on top of the hashtable StyleSheet has, that might help a bit. And I think that setup would help here, since there wasn't the extra malloc/free for the nsMappedAttributes object (the one created for un-bound element and released when bound to document, if using existing nsMappedAttributes)

Would that setup help compared to the null-check suggestion from comment 2, though?

This would probably benefit from similar microbenchmarks to those added at bug 1353900, I guess.

That approach doesn't seem to make that particular worst case scenario any worse on linux. OSX might show different results since malloc seems to be slower there. This is really changing some malloc/free usage to hashtable usage, and then not doing any work during BindToTree

Maybe this even compiles on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b09b03eea399b0f6c2e11c9ddff99b7f8a3b19f

Comment on attachment 8855576 [details] [diff] [review] faster_mapped_attributes_2.diff It would be nice to get rid of some of the hashtable usage too, but haven't figured out any good cache. Commit message for this could be -m "Bug 1352898, add a cache for nsMappedAttributes to reduce malloc/free and bind nsMappedAttributes always to nsHTMLStyleSheet if owner document has such, r=bz"

Comment on attachment 8855576 [details] [diff] [review] faster_mapped_attributes_2.diff >+++ b/dom/base/nsMappedAttributeElement.cpp >\ No newline at end of file Should be a newline. >+++ b/dom/base/nsMappedAttributes.cpp >+nsMappedAttributes::sCachedMappedAttributes = nullptr; Document that this cache is to avoid thrashing the allocator as we go from 0 mapped attributes to our final number? And maybe call it sCachedMappedAttributeAllocations for the same reason? And document that the thing stored at index N can hold N attribute values. >+ // Ensure the cache array is at least mAttrCount + 1 long and I guess this is reasonably safe because we have a hard cap on how many mapped attributes an element can have, so this growth is very much bounded? Might be worth clearly documenting that. Also, if we could hang this cache off document or tabgroup or something or would play much nicer with quantum DOM stuff... Followup is OK here, I guess. r=me

From Quantum DOM point of view it shouldn't actually matter where we keep the cache. Only one fiber is running at a time, so accessing the cache should be safe. And the cache doesn't keep any expect some random memory alive.

I was thinking of the next step, where we start running fibers in parallel...

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/828bcefe18f2 add a cache for nsMappedAttributes to reduce malloc/free and bind nsMappedAttributes always to nsHTMLStyleSheet if owner document has such, r=bz