Closed Bug 660329 Opened 11 years ago Closed 10 years ago

GC: add reason for GC to GCTimer

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gwagner, Assigned: gwagner)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

I want to add the type (global, compartment) and the reason (API call, too much malloc, MaybeGC....) to the GCTimer output
Assignee: general → anygregor
Attached patch patchSplinter Review
The actual output is too long for this textfield so the added columns are T for Global or Compartment Type (C or G) and the reason for the GC.
Destroy,    End, +Chu, -Chu, T, Reason
    0.0,    1.4,  100,    0, C, Maybe
    0.0,    1.1,   63,    0, C, Maybe
    0.0,   19.5,    0,    0, G,   API

Other reasons are:
typedef enum JSGCReason {
    PUBLIC_API,
    MAYBEGC,
    LASTCONTEXT,
    DESTROYCONTEXT,
    COMPARTMENT,
    LASTDITCH,
    TOOMUCHMALLOC,
    ALLOCTRIGGER,
    CHUNK,
    SHAPE,
    NOREASON
} JSGCReason;
Attached file sample output for FOTN
Attachment #535737 - Flags: review?(igor)
Does this track the reason for all GCs?  Would CC-invoked GCs show up as PUBLIC_API as they are invoked from JS_GC?
(In reply to comment #3)
> Does this track the reason for all GCs?  Would CC-invoked GCs show up as
> PUBLIC_API as they are invoked from JS_GC?

Yes JS_GC calls are shown as PUBLIC_API.
We can definitely change it if you think it makes sense.
No, that should be okay as far as I can think, I was just wondering.
Comment on attachment 535737 [details] [diff] [review]
patch

> 
>+typedef enum JSGCReason {

Nit: drop typedef - we do not need to worry about C  compatibility.

>+} JSGCReason;
>+
>+extern JSGCReason gcReason;

Hm, I do not see how this global variable can be used to account for TriggerGC calls. GCREASON for those will be overwritten later with the reason for the actual js_GC calls and only those will be recorded, right?
Attachment #535737 - Flags: review?(igor)
(In reply to comment #6)
> Comment on attachment 535737 [details] [diff] [review] [review]
> patch
> 
> > 
> >+typedef enum JSGCReason {
> 
> Nit: drop typedef - we do not need to worry about C  compatibility.
> 
> >+} JSGCReason;
> >+
> >+extern JSGCReason gcReason;
> 
> Hm, I do not see how this global variable can be used to account for
> TriggerGC calls. GCREASON for those will be overwritten later with the
> reason for the actual js_GC calls and only those will be recorded, right?

The idea is that gcReason is only set the first time and every other trigger doesn't change it. So we only set it if gcReason is NONE.
Comment on attachment 535737 [details] [diff] [review]
patch

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

I have missed the conditional logic in GCREASON. So r+ with the comments addressed.

::: js/src/jsgcstats.h
@@ +185,5 @@
>  
>  extern jsrefcount newChunkCount;
>  extern jsrefcount destroyChunkCount;
>  
> +typedef enum JSGCReason {

Remove the typedef for the enum.

@@ +186,5 @@
>  extern jsrefcount newChunkCount;
>  extern jsrefcount destroyChunkCount;
>  
> +typedef enum JSGCReason {
> +    PUBLIC_API,

The names here are very generic. So either put a enum into some namespace or into a struct like GCTimer as it is used there. IMO the latter is better.

@@ +199,5 @@
> +    SHAPE,
> +    NOREASON
> +} JSGCReason;
> +
> +extern JSGCReason gcReason;

Make this volatile and comment that we do not care about any races when collecting the stats.

@@ +201,5 @@
> +} JSGCReason;
> +
> +extern JSGCReason gcReason;
> +
> +#define GCREASON(x) (gcReason == NOREASON) ? gcReason = x : gcReason = gcReason;

Add () around the macro and x usage in it and remove ";" at the end. If the reason constants would be put into a class, then the macro can add the prefix here.
Attachment #535737 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/558cb6730fbd
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/558cb6730fbd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.