Last Comment Bug 764184 - GC_REASON telemetry might be wrong due to quirk in telemetry API
: GC_REASON telemetry might be wrong due to quirk in telemetry API
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 15:29 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-06-23 05:44 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix GC_REASON telemetry bucket count (3.44 KB, patch)
2012-06-15 15:23 PDT, Steve Fink [:sfink] [:s:]
wmccloskey: review+
nfroyd: review-
Details | Diff | Review
Fix GC_REASON telemetry bucket count (5.26 KB, patch)
2012-06-18 11:33 PDT, Steve Fink [:sfink] [:s:]
nfroyd: review-
Details | Diff | Review
Fix GC_REASON telemetry bucket count (4.98 KB, patch)
2012-06-21 16:51 PDT, Steve Fink [:sfink] [:s:]
nfroyd: review+
Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 15:29:41 PDT
See bug 763359, and bug 763351 which triggered me to file it.

I see that the JS engine seems to be using the same pattern that can give misleading (wrong) results as we were doing in bug 763351, so you might be getting wrong results for this telemetry.
Comment 1 Steve Fink [:sfink] [:s:] 2012-06-15 15:23:57 PDT
Created attachment 633700 [details] [diff] [review]
Fix GC_REASON telemetry bucket count
Comment 2 Bill McCloskey (:billm) 2012-06-15 15:53:39 PDT
Comment on attachment 633700 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

Wow, getting this telemetry stuff right is so much harder than it should be. I still don't understand whether 0 is really a valid telemetry value.
Comment 3 Steve Fink [:sfink] [:s:] 2012-06-15 16:24:52 PDT
(In reply to Bill McCloskey (:billm) from comment #2)
> I still don't understand whether 0 is really a valid telemetry value.

Only for booleans, iiuc. The zero thing still seems bizarre to me.
Comment 4 Bill McCloskey (:billm) 2012-06-15 16:28:22 PDT
We are passing 0 in here (the API reason). Should that be fixed?
Comment 5 Nathan Froyd [:froydnj] 2012-06-18 06:42:33 PDT
(In reply to Steve Fink [:sfink] from comment #3)
> (In reply to Bill McCloskey (:billm) from comment #2)
> > I still don't understand whether 0 is really a valid telemetry value.
> 
> Only for booleans, iiuc. The zero thing still seems bizarre to me.

A zero bucket does make sense; as a contrived example, let's say you allocated frames on the heap and you want to measure how many frames you collect per GC, since you're not completely convinced by the ML guys.  Sure, you define your histogram as 1-1000 with whatever scale you like, but you still need to account for not GC'ing any frames each time.  Whether the manner in which you define histograms is a good one or not is an open question. :)
Comment 6 Nathan Froyd [:froydnj] 2012-06-18 06:44:28 PDT
Comment on attachment 633700 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

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

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +47,5 @@
>  
>  /**
>   * GC telemetry
>   */
> +HISTOGRAM_ENUMERATED_VALUES(GC_REASON_2, 20, "Reason (enum value) for initiating a GC")

It looks like there are more than 20 reasons in jsfriendapi.h nowadays.  Could you please fix that?  (Can we use gcreason::NUM_REASONS here?)
Comment 7 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-06-18 08:17:01 PDT
Is it okay to just randomly change the number of buckets, or does the bucket have to be changed every time?
Comment 8 Nathan Froyd [:froydnj] 2012-06-18 10:12:23 PDT
(In reply to Andrew McCreight [:mccr8] from comment #7)
> Is it okay to just randomly change the number of buckets, or does the bucket
> have to be changed every time?

If any of the parameters to a HISTOGRAM* macro change, the histogram should be renamed.  Weird things will happen on the server side otherwise.

You could insulate yourself from future gcreason changes by selecting some high upper bound (40?) and using that.

Regardless, in the worst case where you have more enums than buckets, you'd be compressing your upper enums into a single bucket; maybe that's not so bad.  (I think the current GC_REASON histogram summaries only show the buckets at the low end being used anyway.)
Comment 9 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-06-18 10:21:35 PDT
A static assert somewhere that NUM_REASONS is less than the number of GC buckets might be good, with a comment that when the number of buckets is exceeded that GC_REASONS_3 has to happen.
Comment 10 Steve Fink [:sfink] [:s:] 2012-06-18 11:33:35 PDT
Created attachment 634123 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

Ok, how about this?
Comment 11 Nathan Froyd [:froydnj] 2012-06-18 11:51:51 PDT
Comment on attachment 634123 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

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

::: js/src/jsfriendapi.h
@@ +62,5 @@
>  extern JS_FRIEND_API(void)
>  JS_TraceShapeCycleCollectorChildren(JSTracer *trc, void *shape);
>  
>  enum {
> +    JS_TELEMETRY_GC_REASON_2,

I think you want to be changing the call to Telemetry::Accumulate in XPCJSRuntime.cpp::AccumulateTelemetryCallback instead of this value.

@@ +603,5 @@
>      GCREASONS(MAKE_REASON)
>  #undef MAKE_REASON
>      NO_REASON,
> +    NUM_REASONS,
> +    NUM_TELEMETRY_REASONS = 100

Quite a future-proof value there.  Do we really need so many, or would 50 do just as well?  How often do we add new GC reasons?

I think a small explanatory comment here would be good.
Comment 12 Steve Fink [:sfink] [:s:] 2012-06-21 16:51:16 PDT
Created attachment 635531 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

Heh. Yes, that was totally the wrong place. I should've mxr'ed it instead of just grepping the JS tree.

I picked 100 just because my initial estimate was 4 or 5, and there were actually 27ish. So I bumped it by an order of magnitude since (1) the cost seems to be low, (2) I really don't to revisit this anytime soon, and (3) I can imagine lots more reasons why we might want to trigger a GC (or a particular type of GC, eg compacting).
Comment 13 Nathan Froyd [:froydnj] 2012-06-22 06:43:33 PDT
Comment on attachment 635531 [details] [diff] [review]
Fix GC_REASON telemetry bucket count

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

Looks good.
Comment 14 Steve Fink [:sfink] [:s:] 2012-06-22 15:36:55 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/fddffb3dc2ad
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:44:58 PDT
https://hg.mozilla.org/mozilla-central/rev/fddffb3dc2ad

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