There are a variety of reasons the browser might invoke the GC, but right now they all get shown as "Reason: API". It would be nice if there was another way to invoke the GC, perhaps in JSFriendAPI, that lets you pass along a reason. This would either involve just passing along a string, or creating some new enum for reasons external users might invoke the API. I envision that this would show up in the error console as something like "Reason: API (forced by CC)". Once this interface is in place, followup bugs can fix clients so they pass in reasons, or I could do them in a few places in this bug.

Note that the followup will probably require changing the signature of nsXPConnect::Collect() so clients of that function can pass in a reason.

This is pretty gross. I'm not sure it all compiles. At least JS and XPCOM do...

Taras thought an enum would be better, but I'm not sure how possible it will be to create a type that is visible to js/, xpcom/, dom/, xpconnect/ and layout/. I haven't really tried yet, though.

I used this to investigate GC behavior in bug 694883.

Could be split out into another bug, but I'll just put it here for now.

I also want to telemetry-ize the GC API and CC reasons, which should be doable browser-side using the enums.

My patch is slightly buggy because it doesn't save the reason for the CC when we start the CC, but it does do the telemetry ping thing, so we end up with different reasons in the ping and the error console. I guess I need to cache the result for display later or something? I think that's what the GC does.

This is the patch we talked about. It's pretty simple and it works with telemetry. All the reasons are consolidated in jsfriendapi.h. I also converted the shrinking parameter to some of the GCs to a gckind parameter, which is an enum defined in nsIXPConnect.idl. In the future, besides normal and shrinking GCs, I want to allow incremental GCs. So this patch sets up that possibility.

Comment on attachment 591364 [details] [diff] [review] GC reason patch that uses jsfriendapi.h for all reasons Review of attachment 591364 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for fixing this up. Passing ints through XPConnect is a little gross, but a lot less gross than all the string stuff I was trying to do. Have you done some basic sanity checking comparing what the error console prints compared to what telemetry shows and that what it prints generally makes sense (like if you click the GC button in about memory, it says it was from whatever)? If not, I can try it out tomorrow. My comments are mostly suggestions, but I would like some assertions in nsXPConnect after values come in over the wall. ::: js/src/gc/Statistics.cpp @@ +136,5 @@ > } > > Statistics::Statistics(JSRuntime *rt) > + : runtime(rt), > + triggerReason(gcreason::API) //dummy reason to satisfy makeTable You could use NO_REASON here. ::: js/src/jsfriendapi.h @@ +570,5 @@ > + D(LAST_DITCH) \ > + D(TOO_MUCH_MALLOC) \ > + D(ALLOC_TRIGGER) \ > + D(CHUNK) \ > + D(SHAPE) \ Is shape unused? Could you add a comment to that effect (saying you have to keep it for telemetry I guess?) or rename it to OBSOLETE or UNUSED? ::: js/src/jsgc.cpp @@ +2504,5 @@ > if (rt->gcNextFullGCTime && rt->gcNextFullGCTime <= now) { > if (rt->gcChunkAllocationSinceLastGC || > rt->gcNumArenasFreeCommitted > FreeCommittedArenasThreshold) > { > + js_GC(cx, NULL, GC_SHRINK, gcreason::MAYBEGC); Does it make any sense to split some of these 4 MAYBEGC invocations into different conditions? I don't know anything about these conditions, so maybe not. ::: js/xpconnect/src/nsXPConnect.cpp @@ +423,5 @@ > // cycle collection. So to compensate for JS_BeginRequest in > // XPCCallContext::Init we disable the conservative scanner if that call > // has started the request on this thread. > js::AutoSkipConservativeScan ascs(cx); > + js::gcreason::Reason gcreason = (js::gcreason::Reason)reason; I think you should add some kind of MAX_GC_REASON kind of rigamarole to jsfriendapi.h near where you define the different reasons, then assert here that it is within bounds. @@ +430,2 @@ > else > + js::GCForReason(cx, gcreason); should assert that gcreason == nsGCNormal. I think adding the assertions for both these args is important especially because they have the same type, which could lead more easily to errors.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64f284541eaf Cool, thanks. After testing the patch for a while, I added a few more reasons. It seems like we GC a lot because of nsGlobalWindow::SetNewDocument. I suspect the mergers are going to close this bug now that a patch has landed, so it might be better for file a separate bug for the CC changes.

(In reply to Bill McCloskey (:billm) from comment #14) > https://hg.mozilla.org/integration/mozilla-inbound/rev/64f284541eaf > > Cool, thanks. After testing the patch for a while, I added a few more > reasons. It seems like we GC a lot because of nsGlobalWindow::SetNewDocument. Ah, nice! I never managed to investigate the context GCs. > I suspect the mergers are going to close this bug now that a patch has > landed, so it might be better for file a separate bug for the CC changes. Yeah, that was my plan. I was just leaving the patch in here until I was ready to copy them.

(In reply to Bill McCloskey (:billm) from comment #14) > I suspect the mergers are going to close this bug now that a patch has > landed, so it might be better for file a separate bug for the CC changes. If it helps for the future, [leave open] in the whiteboard will mean we won't close it (particularly once I get some kind of automation sorted instead of the manual process) :-) https://hg.mozilla.org/mozilla-central/rev/64f284541eaf