1488278 - Add back a cache for IndexOf(childnode)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

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

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.

Comment 1

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

Attachment: compute_index_of_cache.diff

Comment 2

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

Attachment: compute_index_of_cache.diff

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.

Comment 3

[(no longer active)]

The cache seems to have been removed in bug 1481501.

Comment 4

[(no longer active)]

Does this need to get backported to 63?

Comment 5

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

I think so. Will ask approvals.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

errr...

Comment 8

[Pulsebot]

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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

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

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

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 12

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

I have no idea what that comment is about.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Sorry, wrong bug.

Comment 14

[Pascal Chevrel:pascalc]

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

Comment 15

[Cristina Coroiu [:ccoroiu]]

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