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

RESOLVED FIXED in mozilla35

Status

()

Toolkit
Telemetry
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Irving, Assigned: Manu Jain, Mentored)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

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

4 years ago
I want to work on this bug. Please help me.
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8485763 [details] [diff] [review]
bug1063559.patch

Please check the patch of bug 1063559.
Attachment #8485763 - Flags: review?(irving)
(Assignee)

Comment 4

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

># HG changeset patch
># Parent 5b71f646dc5f7a5976d2bc0e2cb2cf2bb0d2b68c
># Manu Jain <manu.jain13@gmail.com>
>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;
>   }
> }
(Assignee)

Comment 5

4 years ago
Created attachment 8485784 [details] [diff] [review]
bug_1063559.patch

Sorry, a spelling mistake was there in previous attachment.
Attachment #8485763 - Attachment is obsolete: true
Attachment #8485763 - Flags: review?(irving)
Attachment #8485784 - Flags: review?(irving)
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
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.
Attachment #8485784 - Flags: review?(irving) → review+
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.
(Assignee)

Comment 8

4 years ago
Ok, after debugging tell me also what was the problem. Thanks!!
Depends on: 1064424
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=ireid@mozilla.com
(Assignee)

Comment 10

4 years ago
So, the tests have ran successfully. :-)
Must land after/with bug 1064424.
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 13

4 years ago
Ryan, can you tell what's wrong with the format? Do I need to submit the patch again after correcting the format? Thanks!!
https://hg.mozilla.org/mozilla-central/rev/59a8bee7b7a4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 15

4 years ago
(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.
You need to log in before you can comment on or make changes to this bug.