Last Comment Bug 721420 - WantAllTraces should disable Skippable CC optimizations
: WantAllTraces should disable Skippable CC optimizations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on:
Blocks: 705582
  Show dependency treegraph
 
Reported: 2012-01-26 09:07 PST by Andrew McCreight [:mccr8]
Modified: 2012-01-30 02:50 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
skip skipping with wantalltraces (2.88 KB, patch)
2012-01-27 15:56 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
null checks are your friend (2.91 KB, patch)
2012-01-27 16:14 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
disable CanSkips, force GC on WantAllTraces (2.96 KB, patch)
2012-01-29 09:01 PST, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-01-26 09:07:55 PST
The following should be disabled when we're under WantAllTraces in the CC:
- CanSkipThis in NoteXPComChild, NoteRoot
- CanSkipInCC in AddPurpleRoot
- mBeforeUnlinkCB

We could cache WantAllTraces in the graph builder to avoid virtual calls everywhere, but we do those calls all over the place anyways.

In the future, when the async purple buffer clearing phase lands, we may have to do something more complex to disable that, like you do a normal CC, then turn off the async purple buffer until the next CC, which is then a WantAllTraces CC.  That could be more complex and would probably mess up the scheduling.
Comment 1 Andrew McCreight [:mccr8] 2012-01-27 15:56:40 PST
Created attachment 592310 [details] [diff] [review]
skip skipping with wantalltraces

untested.
Comment 2 Andrew McCreight [:mccr8] 2012-01-27 16:14:51 PST
Created attachment 592315 [details] [diff] [review]
null checks are your friend

I think the way to deal with the asynchronous cleanup phase is to just force a global GC on a WantAllTraces CC.  This will also avoid issues with losing things due to the compartmental GC.  Just have to make sure this doesn't trigger a FORCE_GC telemetry ping.
Comment 3 Andrew McCreight [:mccr8] 2012-01-27 16:15:11 PST
I'm not sure how that will work with incremental GC, though.
Comment 4 Olli Pettay [:smaug] (reviewing overload) 2012-01-27 16:19:11 PST
You should be able to force non-incremental GC.
Comment 5 Andrew McCreight [:mccr8] 2012-01-29 09:01:52 PST
Created attachment 592509 [details] [diff] [review]
disable CanSkips, force GC on WantAllTraces
Comment 6 Andrew McCreight [:mccr8] 2012-01-29 11:23:45 PST
Comment on attachment 592509 [details] [diff] [review]
disable CanSkips, force GC on WantAllTraces

Seems to work okay with regular CC dumps and WantAllTraces dumps.  I don't disable the unlink callback as that seems to just be used for cleanups, not actually doing anything.

https://tbpl.mozilla.org/?tree=Try&rev=610c52805848
Comment 7 Olli Pettay [:smaug] (reviewing overload) 2012-01-29 11:45:00 PST
Comment on attachment 592509 [details] [diff] [review]
disable CanSkips, force GC on WantAllTraces


>+    if (!cp->CanSkipInCC(root) || builder.WantAllTraces()) {
You should check WantAllTraces() first. CanSkipInCC can remove stuff from purple buffer.
Comment 8 Andrew McCreight [:mccr8] 2012-01-29 16:46:45 PST
(In reply to Olli Pettay [:smaug] from comment #7)
> You should check WantAllTraces() first. CanSkipInCC can remove stuff from
> purple buffer.

Good point.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1725d83e893a
Comment 9 Marco Bonardo [::mak] 2012-01-30 02:50:30 PST
https://hg.mozilla.org/mozilla-central/rev/1725d83e893a

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