WantAllTraces should disable Skippable CC optimizations

RESOLVED FIXED in mozilla12

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: WantAllTraces should disable Skippable CC optimizations → WantAllTraces should disable synchronous Skippable CC optimizations
(Assignee)

Comment 1

5 years ago
Created attachment 592310 [details] [diff] [review]
skip skipping with wantalltraces

untested.
Assignee: nobody → continuation
(Assignee)

Comment 2

5 years ago
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.
Attachment #592310 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
I'm not sure how that will work with incremental GC, though.

Comment 4

5 years ago
You should be able to force non-incremental GC.
(Assignee)

Comment 5

5 years ago
Created attachment 592509 [details] [diff] [review]
disable CanSkips, force GC on WantAllTraces
Attachment #592315 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Summary: WantAllTraces should disable synchronous Skippable CC optimizations → WantAllTraces should disable Skippable CC optimizations
(Assignee)

Comment 6

5 years ago
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
Attachment #592509 - Flags: review?(bugs)

Comment 7

5 years ago
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.
Attachment #592509 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

5 years ago
(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
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/1725d83e893a
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.