Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Description • 6 years ago

When we had child array, even it had a cache for indexing.
https://hg.mozilla.org/mozilla-central/file/ca936adbc565/dom/base/nsAttrAndChildArray.cpp#l41
https://hg.mozilla.org/mozilla-central/file/ca936adbc565/dom/base/nsAttrAndChildArray.cpp#l221

It is based on the educated guess that one accesses usually child nodes close to each others. 
CACHE_POINTER_SHIFT is based on some very old data, and should be inreased.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 6 years ago

So, in case element has more than couple of child nodes, cache the last result of ComputeIndexOf. This is basically the same what we use to do until recently.

	(no longer active)
	Comment 3 • 6 years ago

The cache seems to have been removed in bug 1481501.

	(no longer active)
	Comment 4 • 6 years ago

Does this need to get backported to 63?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 6 years ago

I think so. Will ask approvals.

	Emilio Cobos Álvarez (:emilio)
	Comment 6 • 6 years ago

I removed the remaining code from that in bug 1481501, but it was dead at that point. Bug 651120 is the actual culprit.

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 6 years ago

errr...

	Pulsebot
	Comment 8 • 6 years ago

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a101e633e0
Add back a cache for IndexOf(childnode), r=ehsan

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/44a101e633e0

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 10 • 6 years ago

Comment on attachment 9006094 [details] [diff] [review]
compute_index_of_cache.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 651120
[User impact if declined]: jank in worst cases, needed for bug 1483963
[Is this code covered by automated tests?]: This is often used code
[Has the fix been verified in Nightly?]: landed to m-i yesterday
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: bug 1483963 and bug 1488279
[Is the change risky?]: This is adding back similar cache we used to have, but the old cache dealt with an array and this one a list of child nodes, so they are a bit different and different performance characteristics. So, very tiny bit risky from that point of view
[Why is the change risky/not risky?]: see above
[String changes made/needed]: NA

	(No longer employed by Mozilla) Aaron Klotz
	Comment 11 • 6 years ago

This is significantly better than before, but still ~400ms.

Is this just something that we have to live with when there are high numbers of open tabs?
https://perfht.ml/2oGKWtU

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 6 years ago

I have no idea what that comment is about.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 13 • 6 years ago

Sorry, wrong bug.

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 14 • 6 years ago

Comment on attachment 9006094 [details] [diff] [review]
compute_index_of_cache.diff

Needed to uplift the fix for the regression in bug 1483963, approved for 63 beta

	Cristina Coroiu [:ccoroiu]
	Comment 15 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e23bd4f15e1c