Closed Bug 1202923 Opened 9 years ago Closed 9 years ago

Encapsulate GC begin and end notification callbacks in an RAII guard

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- affected
firefox45 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

It has always seemed exceedingly weird to me that multiple gcCycles can get explicitly aggregated into a single stats entry. I don't think anything other than EnqueuePendingParseTasksAfterGC needs to actually run here and not in gcCycle, so let's see if try agrees:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3f2860c735
Attachment #8658450 - Flags: review?(jcoppeard)
Comment on attachment 8658450 [details] [diff] [review]
55_move_all_state_into_gcCycle-v0.diff

Review of attachment 8658450 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsgc.cpp
@@ +1638,1 @@
>      }

Nit: can get rid of braces here too.

@@ +1665,1 @@
>      }

Ditto braces.

@@ +6099,5 @@
>  GCRuntime::gcCycle(SliceBudget& budget, JS::gcreason::Reason reason)
>  {
> +    AutoTraceLog logGC(TraceLoggerForMainThread(rt), TraceLogger_GC);
> +    AutoStopVerifyingBarriers av(rt, IsShutdownGC(reason));
> +    AutoScheduleZonesForGC asz(rt);

I'm not sure about moving AutoScheduleZonesForGC here.   Don't we want to just do this once?
Attachment #8658450 - Flags: review?(jcoppeard) → review+
Sorry it took me so long to get back to this!

(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8658450 [details] [diff] [review]
> 55_move_all_state_into_gcCycle-v0.diff
> 
> Review of attachment 8658450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsgc.cpp
> @@ +1638,1 @@
> >      }
> 
> Nit: can get rid of braces here too.
> 
> @@ +1665,1 @@
> >      }
> 
> Ditto braces.

This is the MSVC 2013 ranged-for braces bug. Not much we can do in the short term.

> @@ +6099,5 @@
> >  GCRuntime::gcCycle(SliceBudget& budget, JS::gcreason::Reason reason)
> >  {
> > +    AutoTraceLog logGC(TraceLoggerForMainThread(rt), TraceLogger_GC);
> > +    AutoStopVerifyingBarriers av(rt, IsShutdownGC(reason));
> > +    AutoScheduleZonesForGC asz(rt);
> 
> I'm not sure about moving AutoScheduleZonesForGC here.   Don't we want to
> just do this once?

I certainly *hope* that deciding what zones to GC would be idempotent if we do not run the mutator! But I don't know for sure and you are right to urge caution. I overreached a bit here. I've split this patch up so that it only includes the AutoNotify changes and will file separate bugs for moving the dangerous-looking stuff individually.

Carrying the r+ as this is a pure subset.
Attachment #8658450 - Attachment is obsolete: true
Attachment #8680320 - Flags: review+
Summary: Move all GC state below the reset loop → Encapsulate GC begin and end notification callbacks in an RAII guard
https://hg.mozilla.org/integration/mozilla-inbound/rev/b526e349c77fd781319ba2fe95f78233b5670767
Bug 1202923 - Encapsulate GC begin and end notification callbacks in an RAII guard; r=jonco
https://hg.mozilla.org/mozilla-central/rev/b526e349c77f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.