1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Dão Gottwald [:dao]]

We can explicitly check whether the stopwatches are already running and avoid the re-entry logic (and drop the accompanying comment). The comment on why we're starting both stopwatches can be much briefer without losing crucial information, I think. I'd also like to group this with the window.maybeRecordAbandonmentTelemetry call.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

Review commit: https://reviewboard.mozilla.org/r/129946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129946/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review132654

Thanks for cleaning this up. It looks good to me, but I think you'll likely want a review of the TelemetryStopwatch.jsm change from Dexter.

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review132906

Thanks for adding this new feature to TelemetryStopwatch, it looks good! Could you please also add docs for this at: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/measuring-time.rst ?

Make sure they read ok by testing them with ./mach doc :)

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129946/diff/1-2/

Comment 5

[Georg Fritzsche [:gfritzsche]]

Let's please add test coverage for the functions to test_TelemetryStopwatch.js.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review133656

::: toolkit/components/telemetry/docs/collection/measuring-time.rst:53
(Diff revision 2)
>      // ... do more work.
>      TelemetryStopwatch.finish("SAMPLE_FILE_LOAD_TIME_MS");
>  
> +    // Another loading attempt? Start stopwatch again if
> +    // not already running.
> +    if (!TelemetryStopwatch.running("SAMPLE_FILE_LOAD_TIME_MS")) {

Sorry for not catching this earlier, but would you please add test coverage in [test_TelemetryStopwatch.js](http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js) too?

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129946/diff/2-3/

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review134384

::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
(Diff revision 3)
>  
>    // Starting & finishing a keyed stopwatch for an existing histogram should work.
>    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
>    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
>    // Verify that TS.finish deleted the timers
> -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));

Please leave the test for |finishKeyed| too, just add a new do_check_false(...) for runningKeyed as well here.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Alessio Placitelli [:Dexter] from comment #8)
> Comment on attachment 8857912 [details]
> Bug 1356210 - Clean up TelemetryStopwatch.start calls for
> FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.
> 
> https://reviewboard.mozilla.org/r/129946/#review134384
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
> (Diff revision 3)
> >  
> >    // Starting & finishing a keyed stopwatch for an existing histogram should work.
> >    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
> >    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> >    // Verify that TS.finish deleted the timers
> > -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));
> 
> Please leave the test for |finishKeyed| too, just add a new
> do_check_false(...) for runningKeyed as well here.

That's not useful as far as I can tell. That aspect of finishKeyed is already tested earlier in the test.

Comment 10

[Dão Gottwald [:dao]]

In other words, the line that I modified isn't meant to test how finishKeyed behaves when called twice; it's a test for the behavior of the previous finishKeyed call.

Comment 11

[Alessio Placitelli [:Dexter]]

(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to Alessio Placitelli [:Dexter] from comment #8)
> > Comment on attachment 8857912 [details]
> > Bug 1356210 - Clean up TelemetryStopwatch.start calls for
> > FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.
> > 
> > https://reviewboard.mozilla.org/r/129946/#review134384
> > 
> > ::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
> > (Diff revision 3)
> > >  
> > >    // Starting & finishing a keyed stopwatch for an existing histogram should work.
> > >    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > >    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > >    // Verify that TS.finish deleted the timers
> > > -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > > +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > 
> > Please leave the test for |finishKeyed| too, just add a new
> > do_check_false(...) for runningKeyed as well here.
> 
> That's not useful as far as I can tell. That aspect of finishKeyed is
> already tested earlier in the test.

Good point, I've dropped the issue on mozreview.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b4aac572731
Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes. r=Dexter,mconley

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b4aac572731