WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Summary: WantAllTraces does not disable nsINode::Traverse optimizations → WantAllTraces should disable nsINode::Traverse optimizations
(Assignee)

Comment 1

5 years ago
Really, the logic for skipping non-JS edges out of nsINode should probably be moved entirely into CanSkipThis, and Traverse should always succeed.
(Assignee)

Updated

5 years ago
Summary: WantAllTraces should disable nsINode::Traverse optimizations → WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations
(Assignee)

Comment 2

5 years ago
Created attachment 607365 [details] [diff] [review]
disable want all traces

I did an audit of every place where NS_SUCCESS_INTERRUPTED is returned to make sure that it was being properly handled.  It may make more sense in the long term to rip out all of that logic and move it into CanSkipThis, and make Traverse always succeed, but this will do for now.
(Assignee)

Comment 3

5 years ago
Created attachment 607366 [details] [diff] [review]
disable want all traces, for reals
Attachment #607365 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
This patch is hard to read, but what I did was take the entire block of code at the start of nsINode::Traverse and wrap it in a !cb.WantAllTraces() test.  I also dropped the cb argument to InGeneration, because we don't have to check cb any more if we're in that block.  The code should be otherwise unchanged.
(Assignee)

Updated

5 years ago
Attachment #607366 - Flags: review?(bugs)

Updated

5 years ago
Attachment #607366 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca353538c7f9
Target Milestone: --- → mozilla14
(Assignee)

Comment 6

5 years ago
According to tree management, this caused some DOM-y regressions on Linux64.  I'm skeptical that an additional highly predictable branch (these tests are always true except when dumping a CC log) per DOM node visited by the cycle collector could cause a 3% regression, but I guess it is possible.  Maybe I need to sprinkle some NS_LIKELY pixie dust around.

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/VTerA5eCc6M
(Assignee)

Comment 7

5 years ago
Created attachment 608197 [details] [diff] [review]
add NS_LIKELY for new !WantAllTraces checks

Can't hurt, I guess.
(Assignee)

Comment 8

5 years ago
Backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e530a78a527a
Target Milestone: mozilla14 → ---
(Assignee)

Comment 9

5 years ago
Comment on attachment 608197 [details] [diff] [review]
add NS_LIKELY for new !WantAllTraces checks

The backout didn't cause a corresponding increase, so I guess the regression was bogus, but it can't hurt to help GCC out a little.
Attachment #608197 - Flags: review?(bugs)

Updated

5 years ago
Attachment #608197 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

5 years ago
Thanks for the quick review.  Relanded, with the additional patch folded in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72bcaa8ab10d
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/72bcaa8ab10d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.