Closed
Bug 904602
Opened 11 years ago
Closed 11 years ago
Optimize various tree traversing in Range
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: smaug, Assigned: smaug)
Details
Attachments
(3 files)
1.29 KB,
text/html
|
Details | |
2.66 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
2.64 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
Patch seems to also save over 9000000 method calls when running http://mxr.mozilla.org/mozilla-central/source/dom/imptests/editing/conformancetest/test_runtest.html?force=1
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
SubtreeRoot() should return right thing during ParentChainChanged for aContent and its descendants since UnbindFromTree has been called for those.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Also, note, GetParentNode() during ContentRemoved hasn't been updated either so SubtreeRoot should return the same value as GetParentNode chain.
Comment 7•11 years ago
|
||
OK, then there's shouldn't be any false positives for ContentRemoved. Might be worth checking the logs anyway, just in case :-)
Assignee | ||
Comment 8•11 years ago
|
||
Yup, will do. Good that I pushed with try: -a so that I get debug builds too :)
Assignee | ||
Comment 9•11 years ago
|
||
Couldn't see the assertions in try logs https://hg.mozilla.org/integration/mozilla-inbound/rev/63635d50157f
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63635d50157f
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•