WantAllTraces should disable Skippable CC optimizations

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

8 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

8 years ago
Summary: WantAllTraces should disable Skippable CC optimizations → WantAllTraces should disable synchronous Skippable CC optimizations
Assignee

Comment 1

8 years ago
untested.
Assignee: nobody → continuation
Assignee

Comment 2

8 years ago
Posted patch null checks are your friend (obsolete) — Splinter Review
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

8 years ago
I'm not sure how that will work with incremental GC, though.
You should be able to force non-incremental GC.
Assignee

Comment 5

8 years ago
Attachment #592315 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Summary: WantAllTraces should disable synchronous Skippable CC optimizations → WantAllTraces should disable Skippable CC optimizations
Assignee

Comment 6

8 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 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

8 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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.