Last Comment Bug 736459 - WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations
: WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 723783
  Show dependency treegraph
 
Reported: 2012-03-16 07:28 PDT by Andrew McCreight [:mccr8]
Modified: 2012-03-22 18:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
disable want all traces (215 bytes, patch)
2012-03-19 16:25 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
disable want all traces, for reals (3.54 KB, patch)
2012-03-19 16:28 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review
add NS_LIKELY for new !WantAllTraces checks (2.04 KB, patch)
2012-03-21 19:28 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-03-16 07:28:49 PDT

    
Comment 1 Andrew McCreight [:mccr8] 2012-03-16 07:38:04 PDT
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] 2012-03-19 16:25:34 PDT
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.
Comment 3 Andrew McCreight [:mccr8] 2012-03-19 16:28:13 PDT
Created attachment 607366 [details] [diff] [review]
disable want all traces, for reals
Comment 4 Andrew McCreight [:mccr8] 2012-03-19 16:31:11 PDT
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] 2012-03-21 15:41:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca353538c7f9
Comment 6 Andrew McCreight [:mccr8] 2012-03-21 18:32:58 PDT
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] 2012-03-21 19:28:48 PDT
Created attachment 608197 [details] [diff] [review]
add NS_LIKELY for new !WantAllTraces checks

Can't hurt, I guess.
Comment 8 Andrew McCreight [:mccr8] 2012-03-21 19:38:00 PDT
Backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e530a78a527a
Comment 9 Andrew McCreight [:mccr8] 2012-03-22 07:43:59 PDT
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] 2012-03-22 08:40:33 PDT
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] 2012-03-22 18:14:00 PDT
https://hg.mozilla.org/mozilla-central/rev/72bcaa8ab10d

Note You need to log in before you can comment on or make changes to this bug.