Optimize various tree traversing in Range

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla26
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 789572 [details]
artificial test

Currently Range uses GetParentNode to find the subtree root.
nsINode::SubtreeRoot() should work just fine. That one is O(1)

nsRange::ContentRemoved may end up doing nsContentUtils::ContentIsDescendantOf
twice. Reasonably often mStartParent and mEndParent are the same, so we can
optimize out one of those possibly slow calls.

(Unfortunately SubtreeRoot() check inside nsContentUtils::ContentIsDescendantOf
wouldn't be too useful since the parent chain is actually changed *after*
nsIMutationObserver::ContentRemoved call.)
Created attachment 789581 [details] [diff] [review]
patch

Gives 40% boost in that artificial test.


Note, I decided to not change
   if (!mEnableGravitationOnElementRemoval) {
     // Do not gravitate.
     return;
in any way, since that is used only in some odd special cases.


https://tbpl.mozilla.org/?tree=Try&rev=d1de420f93c6
Attachment #789581 - Flags: review?(matspal)
Comment on attachment 789581 [details] [diff] [review]
patch

>+  } else if (didCheckStartParentDescendant &&
>+             mStartParent == mEndParent) {

Nit: the condition fits on one line

The changes to nsRange::ContentRemoved looks correct.

nsRange::IsValidBoundary also looks correct, but a bit risky since
there's a lot code reaching IsValidBoundary... I see that SubtreeRoot()
has an assertion checking for this though.

I note that nsRange::ContentRemoved calls DoSetRange which calls
IsValidBoundary although only as part of an assertion, but it might
trigger false positives?

nsRange::ParentChainChanged calls IsValidBoundary - is SubtreeRoot()
always valid when ParentChainChanged is called?

I think it would good if you manually check the mochitest full logs
for any "These should always be in sync!" assertions. Perhaps
crashtests too since they may be masked by "asserts(0-999)" and the
like that we have for some tests.

r=mats
Attachment #789581 - Flags: review?(matspal) → review+
SubtreeRoot() should return right thing during ParentChainChanged for aContent and its
descendants since UnbindFromTree has been called for those.
Also, note, GetParentNode() during ContentRemoved hasn't been updated either so SubtreeRoot
should return the same value as GetParentNode chain.
OK, then there's shouldn't be any false positives for ContentRemoved.
Might be worth checking the logs anyway, just in case :-)
Yup, will do. Good that I pushed with try: -a so that I get debug builds too :)
https://hg.mozilla.org/mozilla-central/rev/63635d50157f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.