Closed Bug 1283256 Opened 4 years ago Closed 4 years ago

Move slice stats collection inside of the auto-cycle


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: terrence, Assigned: terrence)



(1 file)

Currently, the JSGC_END callback happens inside of the slice's scope. It looks like if this triggers a GC, then we'll append a new slice without doing end-slice on the current slice and then do end-slice twice on the inner slice later. Which is probably not going to work very well. The attached patch moves the AutoGCSlice after AutoNotifyGCActivity so that we at least don't have to worry about this weird nesting.

Note that this is independent of the cycle repeat mechanism that we have for poke and zombie compartments. Moving this inside that logic means that we can strip the confusing cycle count from the slice and have the repeated gc slice have its own stats slice.

This patch appears to be as green as tip, so let's move forward with this.
Attachment #8766490 - Flags: review?(sphink)
Comment on attachment 8766490 [details] [diff] [review]

Review of attachment 8766490 [details] [diff] [review]:

Ugh. You know that we still have two different AutoGCSlice classes? :-(

I'm fine with this, since I've never paid any attention to the cycle counts, but given that jonco added that stuff in bug 1257496 I think I'll give him the review.

::: js/src/jsgc.cpp
@@ -6306,5 @@
>      AutoTraceLog logGC(TraceLoggerForMainThread(rt), TraceLogger_GC);
>      AutoStopVerifyingBarriers av(rt, IsShutdownGC(reason));
>      AutoEnqueuePendingParseTasksAfterGC aept(*this);
>      AutoScheduleZonesForGC asz(rt);
> -    gcstats::AutoGCSlice agc(stats, scanZonesBeforeGC(), invocationKind, budget, reason);

This seems like a very strange place to have the AutoGCSlice, but maybe it made sense when tracking cycle counts.
Attachment #8766490 - Flags: review?(sphink) → review?(jcoppeard)
Comment on attachment 8766490 [details] [diff] [review]

Review of attachment 8766490 [details] [diff] [review]:

Yeah, this makes sense to me.

The addition of the cycle count was just to make visible what was happening without changing the way things worked.  This actually gives us better visibility as the cycles now have their own separate slices.
Attachment #8766490 - Flags: review?(jcoppeard) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.