Last Comment Bug 706227 - Add way for JS_GC API users to give detailed reason for invocation
: Add way for JS_GC API users to give detailed reason for invocation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
: 711209 (view as bug list)
Depends on: 721302
Blocks: 694883 711900
  Show dependency treegraph
 
Reported: 2011-11-29 12:48 PST by Andrew McCreight [:mccr8]
Modified: 2012-01-29 08:18 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add string args to GC invocations. WIP. (21.33 KB, patch)
2011-12-04 06:54 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
improved patch that doesn't crash immediately. WIP. (23.99 KB, patch)
2011-12-05 18:47 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: add way for JS GC API users to give a reason (14.40 KB, patch)
2011-12-20 11:07 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: pass GC reasons to JS (10.99 KB, patch)
2011-12-20 11:08 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 3: print CC reasons to the error console (9.22 KB, patch)
2011-12-20 11:09 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 4: telemetry for CC, GC API reasons. Completely untested. (3.14 KB, patch)
2011-12-20 12:16 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
GC reason patch that uses jsfriendapi.h for all reasons (37.83 KB, patch)
2012-01-24 21:24 PST, [PTO to Dec5] Bill McCloskey (:billm)
continuation: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-11-29 12:48:07 PST
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.
Comment 1 Andrew McCreight [:mccr8] 2011-11-29 13:14:03 PST
Note that the followup will probably require changing the signature of nsXPConnect::Collect() so clients of that function can pass in a reason.
Comment 2 Andrew McCreight [:mccr8] 2011-12-04 06:54:54 PST
Created attachment 578899 [details] [diff] [review]
Add string args to GC invocations. WIP.

This is pretty gross.  I'm not sure it all compiles.  At least JS and XPCOM do...
Comment 3 Andrew McCreight [:mccr8] 2011-12-04 06:57:58 PST
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.
Comment 4 Andrew McCreight [:mccr8] 2011-12-05 18:47:21 PST
Created attachment 579190 [details] [diff] [review]
improved patch that doesn't crash immediately.  WIP.
Comment 5 Andrew McCreight [:mccr8] 2011-12-05 19:05:26 PST
I used this to investigate GC behavior in bug 694883.
Comment 6 Andrew McCreight [:mccr8] 2011-12-20 11:07:01 PST
Created attachment 583221 [details] [diff] [review]
part 1: add way for JS GC API users to give a reason
Comment 7 Andrew McCreight [:mccr8] 2011-12-20 11:08:03 PST
Created attachment 583222 [details] [diff] [review]
part 2: pass GC reasons to JS
Comment 8 Andrew McCreight [:mccr8] 2011-12-20 11:09:18 PST
Created attachment 583224 [details] [diff] [review]
part 3: print CC reasons to the error console

Could be split out into another bug, but I'll just put it here for now.
Comment 9 Andrew McCreight [:mccr8] 2011-12-20 11:39:52 PST
I also want to telemetry-ize the GC API and CC reasons, which should be doable browser-side using the enums.
Comment 10 Andrew McCreight [:mccr8] 2011-12-20 12:16:06 PST
Created attachment 583247 [details] [diff] [review]
part 4: telemetry for CC, GC API reasons.  Completely untested.
Comment 11 Andrew McCreight [:mccr8] 2011-12-20 17:53:16 PST
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.
Comment 12 [PTO to Dec5] Bill McCloskey (:billm) 2012-01-24 21:24:32 PST
Created attachment 591364 [details] [diff] [review]
GC reason patch that uses jsfriendapi.h for all reasons

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 13 Andrew McCreight [:mccr8] 2012-01-24 23:47:14 PST
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.
Comment 14 [PTO to Dec5] Bill McCloskey (:billm) 2012-01-25 11:03:27 PST
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.
Comment 15 Andrew McCreight [:mccr8] 2012-01-25 11:09:36 PST
(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.
Comment 16 Ed Morley [:emorley] 2012-01-25 18:07:29 PST
(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
Comment 17 Andrew McCreight [:mccr8] 2012-01-29 08:18:02 PST
*** Bug 711209 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.