Closed
Bug 736459
Opened 12 years ago
Closed 12 years ago
WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
3.54 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Summary: WantAllTraces does not disable nsINode::Traverse optimizations → WantAllTraces should disable nsINode::Traverse optimizations
Assignee | ||
Comment 1•12 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•12 years ago
|
Summary: WantAllTraces should disable nsINode::Traverse optimizations → WantAllTraces should disable nsINode::Traverse, nsBaseContentList optimizations
Assignee | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Attachment #607365 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 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•12 years ago
|
Attachment #607366 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #607366 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca353538c7f9
Target Milestone: --- → mozilla14
Assignee | ||
Comment 6•12 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•12 years ago
|
||
Can't hurt, I guess.
Assignee | ||
Comment 8•12 years ago
|
||
Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/e530a78a527a
Target Milestone: mozilla14 → ---
Assignee | ||
Comment 9•12 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•12 years ago
|
Attachment #608197 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•12 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
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72bcaa8ab10d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•