1063559 - Convert TelemetryStopwatch.jsm to use Components.utils.now()

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[:Irving Reid (No longer working on Firefox)]

Bug 969490 has given us a more reliable way of measuring time in chrome code. We should replace uses of "Date.now()" or "new Date()" with Components.utils.now(), anywhere we subtract two time measurements to calculate an interval.

Because Components.utils.now() returns a floating-point milliseconds with microsecond precision, we also want to Math.round() the result of the subtraction so that we report an integer number of milliseconds.

Comment 1

[Aman Singh]

I want to work on this bug. Please help me.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Aman, thanks for looking at this.

The code that needs to be changed is in toolkit/components/telemetry/TelemetryStopwatch.jsm, in the 'start' method where the value of Date.now() is recorded and in the 'finish' method where we call Date.now() again, calculate the delta and add that value to the histogram.

To test this, you can run the unit tests using the command

./mach test --sequential test_TelemetryStopwatch.js

You can also check the recorded values; navigate to the page "about:telemetry", expand the "Histograms" section, and look for a measurement that is recorded using TelemetryStopwatch; FX_PAGE_LOAD_MS is one example.

Comment 3

[Manu Jain]

Attachment: bug1063559.patch

Please check the patch of bug 1063559.

Comment 4

[Manu Jain]

Comment on attachment 8485763 [details] [diff] [review]
bug1063559.patch

># HG changeset patch
># Parent 5b71f646dc5f7a5976d2bc0e2cb2cf2bb0d2b68c
># Manu Jain <[e-mail]>
>Bug 1063559 - Convert TelemetryStopwatch.jsm to use Components.utils.now(); r=irvingreid
>diff --git a/toolkit/components/telemetry/TelemetryStopwatch.jsm b/toolkit/components/telemetry/TelemetryStopwatch.jsm
>--- a/toolkit/components/telemetry/TelemetryStopwatch.jsm
>+++ b/toolkit/components/telemetry/TelemetryStopwatch.jsm
>@@ -49,17 +49,17 @@ this.TelemetryStopwatch = {
> 
>     if (timers.hasOwnProperty(aHistogram)) {
>       delete timers[aHistogram];
>       Cu.reportError("TelemetryStopwatch: key \"" +
>                      aHistogram + "\" was already initialized");
>       return false;
>     }
> 
>-    timers[aHistogram] = Date.now();
>+    timers[aHistogram] = Components.utils.now();
>     return true;
>   },
> 
>   /**
>    * Deletes the timer associated with a telemetry histogram. The timer can be
>    * directly associated with a histogram, or with a pair of a histogram and
>    * an object. Important: Only use this method when a legitimate cancellation
>    * should be done.
>@@ -112,17 +112,18 @@ this.TelemetryStopwatch = {
>     let timers = aObj
>                  ? objectTimers.get(aObj) || {}
>                  : simpleTimers;
> 
>     let start = timers[aHistogram];
>     delete timers[aHistogram];
> 
>     if (start) {
>-      let delta = Date.now() - start;
>+      let delta = Components.utils.now() - start;
>+      delta = Math.round(delta);
>       let histogram = Telemetry.getHistogramById(aHistogram);
>       histogram.add(delta);
>       return true;
>     }
> 
>     return false;
>   }
> }

Comment 5

[Manu Jain]

Attachment: bug_1063559.patch

Sorry, a spelling mistake was there in previous attachment.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8485784 [details] [diff] [review]
bug_1063559.patch

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

Thanks for the patch. Are you interested in taking on another one? If so, I can suggest bug 553869 or bug 1007656.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Unfortunately, when I run the tests this causes a failure in the underlying implementation. I'm trying to debug it now, but we need to hold this patch back until I get that sorted out.

Comment 8

[Manu Jain]

Ok, after debugging tell me also what was the problem. Thanks!!

Comment 9

[:Irving Reid (No longer working on Firefox)]

See bug 1064424 for the problem and patch to fix it. Currently running tests on the Try servers: https://tbpl.mozilla.org/?tree=Try&rev=f9e54ceec78f&pusher=[e-mail]

Comment 10

[Manu Jain]

So, the tests have ran successfully. :-)

Comment 11

[:Irving Reid (No longer working on Firefox)]

Must land after/with bug 1064424.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59a8bee7b7a4

Thanks for the patch, Manu! One request, can you please double-check your hg configuration? The author metadata wasn't in the right format for proper import. The link below has some more info for you if you need it. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 13

[Manu Jain]

Ryan, can you tell what's wrong with the format? Do I need to submit the patch again after correcting the format? Thanks!!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59a8bee7b7a4

Comment 15

[:ProgramFOX]

(In reply to manu.jain13 from comment #13)
> Ryan, can you tell what's wrong with the format? Do I need to submit the
> patch again after correcting the format? Thanks!!

The author metadata was in the following format:
> # Manu Jain <email address>
But it has to be like this:
> # User Manu Jain <email address>
You don't have to submit the patch again, as it is now checked in, with the metadata edited. You'll see that the metadata of the patch that's checked in has some more data. To be sure that the patch metadata is in the correct format, add this to your hgrc file (stored in the .hg folder in the base directory):
>[diff]
>git = 1
>showfunc = 1
>unified = 8

>[ui]
>username = User name <email address>

Note that you shouldn't start the lines with the > sign, that's just to indicate a quote here on Bugzilla.