Last Comment Bug 706164 - [CC] Add telemetry for CC forcing GC
: [CC] Add telemetry for CC forcing GC
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Andrew McCreight [:mccr8]
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks: 698919 638299
  Show dependency treegraph
 
Reported: 2011-11-29 10:46 PST by Andrew McCreight [:mccr8]
Modified: 2012-05-20 12:35 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add telemtry for GC forcing CC (2.50 KB, patch)
2011-12-05 10:01 PST, Andrew McCreight [:mccr8]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-11-29 10:46:16 PST
(Splitting this part of the telemetry out from the timing stuff.)

How often does a CC force a GC?  This would have a nasty impact on responsiveness if it is common.  It happens when there's an overflow while ungraying.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-11-29 11:45:08 PST
Hmm.  _This_ may be what's been biting me on m-c.  I was certainly seeing a slew of CCs and GCs all in a row.  Could we also add this info the to the CC/GC logging we do to the console?
Comment 2 Andrew McCreight [:mccr8] 2011-11-29 11:58:46 PST
Ideally, this won't happen much, so if it shows up in about:telemetry that would be a sign that something is wrong.  Though those overflows seemed to happen a lot in Thunderbird.  But yes, that's a good idea.  Another approach would be to enrich the GC's interface a bit, so you could pass in a string that tells it why you are invoking it, and it could print that out in the reason.
Comment 3 (dormant account) 2011-11-29 12:02:34 PST
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Ideally, this won't happen much, so if it shows up in about:telemetry that
> would be a sign that something is wrong.  Though those overflows seemed to
> happen a lot in Thunderbird.  But yes, that's a good idea.  Another approach
> would be to enrich the GC's interface a bit, so you could pass in a string
> that tells it why you are invoking it, and it could print that out in the
> reason.

That sounds like a great idea, but I vote for an enum.
Comment 4 Andrew McCreight [:mccr8] 2011-11-29 12:15:44 PST
(In reply to Taras Glek (:taras) from comment #3)
> That sounds like a great idea, but I vote for an enum.
The only reason I was suggesting a string is that having a type defined that works in both JS land and non-JS land can be a pain, but I guess I can just put it wherever the signature for the external JS hook lives and it shouldn't be a problem.  I'll look into filing a new bug for that.
Comment 5 Andrew McCreight [:mccr8] 2011-11-29 12:49:23 PST
Filed bug 706227 for adding an interface for API users to communicate a reason for the GC.
Comment 6 Andrew McCreight [:mccr8] 2011-12-05 10:01:46 PST
Created attachment 579107 [details] [diff] [review]
add telemtry for GC forcing CC

The control flow here is a little weird.  I added NS_LIKELY mostly as a way to indicate to the reader what the hot path is.  We only want to ping telemetry on non-shutdown CCs, as those are always going to force a GC, so we don't want to skew the stats one way or the other by pinging in those cases.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-16 21:50:39 PST
Comment on attachment 579107 [details] [diff] [review]
add telemtry for GC forcing CC

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2685,5 @@
>  
>      nsCycleCollectionJSRuntime* rt =
>          static_cast<nsCycleCollectionJSRuntime*>
>              (mRuntimes[nsIProgrammingLanguage::JAVASCRIPT]);
> +    if (NS_LIKELY(!aForceGC)) {

These NS_LIKELY are probably not really important... I think evidence that they help is scant.
Comment 8 Andrew McCreight [:mccr8] 2011-12-17 11:52:10 PST
Okay, I just removed the NS_LIKELYs.  The comment is probably enough to figure out what is happening.  Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9fce0aed78
Comment 9 Matt Brubeck (:mbrubeck) 2011-12-18 15:31:59 PST
https://hg.mozilla.org/mozilla-central/rev/ad9fce0aed78
Comment 10 Andrew McCreight [:mccr8] 2012-01-20 11:16:06 PST
Taras pointed out to me that this measure has gone up 257.71% recently.  Of course, this is hit so rarely that could just mean that 3 people are hitting it instead of 1...

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