Add way for JS_GC API users to give detailed reason for invocation

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: billm)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

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

Comment 2

6 years ago
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...
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Created attachment 579190 [details] [diff] [review]
improved patch that doesn't crash immediately.  WIP.
Attachment #578899 - Attachment is obsolete: true
(Reporter)

Comment 5

6 years ago
I used this to investigate GC behavior in bug 694883.
Blocks: 694883
(Reporter)

Updated

6 years ago
Blocks: 711900
(Reporter)

Comment 6

6 years ago
Created attachment 583221 [details] [diff] [review]
part 1: add way for JS GC API users to give a reason
Assignee: general → continuation
Attachment #579190 - Attachment is obsolete: true
(Reporter)

Comment 7

6 years ago
Created attachment 583222 [details] [diff] [review]
part 2: pass GC reasons to JS
(Reporter)

Comment 8

6 years ago
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.
(Reporter)

Comment 9

6 years ago
I also want to telemetry-ize the GC API and CC reasons, which should be doable browser-side using the enums.
(Reporter)

Comment 10

6 years ago
Created attachment 583247 [details] [diff] [review]
part 4: telemetry for CC, GC API reasons.  Completely untested.
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

5 years ago
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.
Attachment #591364 - Flags: review?(continuation)
(Reporter)

Updated

5 years ago
Assignee: continuation → wmccloskey
(Reporter)

Comment 13

5 years ago
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.
Attachment #591364 - Flags: review?(continuation) → review+
(Reporter)

Updated

5 years ago
Attachment #583221 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #583222 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #583247 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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.
Target Milestone: --- → mozilla12
(Reporter)

Comment 15

5 years ago
(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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 721302
(Reporter)

Updated

5 years ago
Duplicate of this bug: 711209
You need to log in before you can comment on or make changes to this bug.