Closed Bug 1488278 Opened 6 years ago Closed 6 years ago

Add back a cache for IndexOf(childnode)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch compute_index_of_cache.diff (obsolete) — Splinter Review
Attachment #9006093 - Flags: review?(ehsan)
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.
Attachment #9006094 - Flags: review?(ehsan)
Attachment #9006093 - Flags: review?(ehsan)
Attachment #9006093 - Attachment is obsolete: true
The cache seems to have been removed in bug 1481501.
Blocks: 1481501
Attachment #9006094 - Flags: review?(ehsan) → review+
Does this need to get backported to 63?
Flags: needinfo?(bugs)
I think so. Will ask approvals.
Flags: needinfo?(bugs)
I removed the remaining code from that in bug 1481501, but it was dead at that point. Bug 651120 is the actual culprit.
Blocks: 651120
No longer blocks: 1483963
errr...
Blocks: 1483963
No longer blocks: 1481501
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a101e633e0
Add back a cache for IndexOf(childnode), r=ehsan
Keywords: regression
Priority: -- → P2
No longer blocks: 1423367
https://hg.mozilla.org/mozilla-central/rev/44a101e633e0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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
Attachment #9006094 - Flags: approval-mozilla-beta?
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
I have no idea what that comment is about.
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
Attachment #9006094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: