Closed
Bug 660329
Opened 13 years ago
Closed 13 years ago
GC: add reason for GC to GCTimer
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: gwagner, Assigned: gwagner)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
9.13 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
10.75 KB,
text/plain
|
Details |
I want to add the type (global, compartment) and the reason (API call, too much malloc, MaybeGC....) to the GCTimer output
Assignee | ||
Updated•13 years ago
|
Assignee: general → anygregor
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #535737 -
Flags: review?(igor)
Comment 3•13 years ago
|
||
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•13 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.
Comment 5•13 years ago
|
||
No, that should be okay as far as I can think, I was just wondering.
Comment 6•13 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•13 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•13 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•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/558cb6730fbd
Whiteboard: fixed-in-tracemonkey
Comment 10•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/558cb6730fbd
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•