736459 - WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations

Status: RESOLVED FIXED

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

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: disable want all traces, for reals

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca353538c7f9

Comment 6

[Andrew McCreight [:mccr8]]

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

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: add NS_LIKELY for new !WantAllTraces checks

Can't hurt, I guess.

Comment 8

[Andrew McCreight [:mccr8]]

Backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e530a78a527a

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Andrew McCreight [:mccr8]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/72bcaa8ab10d

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/72bcaa8ab10d