The default bug view has changed. See this FAQ.

[CC] Add telemetry for CC forcing GC

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment)

2.50 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 2

5 years ago
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

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

Comment 4

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

Comment 5

5 years ago
Filed bug 706227 for adding an interface for API users to communicate a reason for the GC.
(Assignee)

Comment 6

5 years ago
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.
Attachment #579107 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Whiteboard: [Snappy]
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.
Attachment #579107 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 8

5 years ago
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
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/ad9fce0aed78
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

5 years ago
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...
No longer blocks: 698919
Blocks: 698919
You need to log in before you can comment on or make changes to this bug.