764184 - GC_REASON telemetry might be wrong due to quirk in telemetry API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Attachment: Fix GC_REASON telemetry bucket count

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

(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 [inactive unless it's an emergency] (:billm)]

We are passing 0 in here (the API reason). Should that be fixed?

Comment 5

[Nathan Froyd [:froydnj]]

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

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 [:mccr8]]

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

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

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

Attachment: Fix GC_REASON telemetry bucket count

Ok, how about this?

Comment 11

[Nathan Froyd [:froydnj]]

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

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

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/fddffb3dc2ad

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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