GC: add reason for GC to GCTimer

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
I want to add the type (global, compartment) and the reason (API call, too much malloc, MaybeGC....) to the GCTimer output
(Assignee)

Updated

7 years ago
Assignee: general → anygregor
(Assignee)

Comment 1

7 years ago
Created attachment 535737 [details] [diff] [review]
patch

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;
(Assignee)

Comment 2

7 years ago
Created attachment 535738 [details]
sample output for FOTN
(Assignee)

Updated

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

Comment 4

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

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

Comment 7

7 years ago
(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 8

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

Comment 9

7 years ago
http://hg.mozilla.org/tracemonkey/rev/558cb6730fbd
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/558cb6730fbd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.