706164 - [CC] Add telemetry for CC forcing GC

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

(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 [:bzbarsky]]

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]]

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)]

(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]]

(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]]

Filed bug 706227 for adding an interface for API users to communicate a reason for the GC.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: 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!)]

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]]

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)]

https://hg.mozilla.org/mozilla-central/rev/ad9fce0aed78

Comment 10

[Andrew McCreight [:mccr8]]

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...