All users were logged out of Bugzilla on October 13th, 2018

Improve reliability of the CPOW clock

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

CPOW monitoring uses `PR_IntervalNow()`, which returns milliseconds. That's way to inaccurate for our use.

If I read the code and comments correctly, `PR_Now()` should have µs resolution under Unix, but only milliseconds under Windows. Under Windows, we may need QueryPerformanceCounter / QueryPerformanceFrequency or GetSystemTimePreciseAsFileTime .
In bug 1152829 it was agreed to use PRMJ_Now() (defined in js/src/prmjtime.cpp) for everything involving the profiler. I'm not sure what that bug is waiting on, but perhaps you could use it too? PRMJ_Now() gives a microsecond resolution; it uses QPC under the hood on Windows (and GetSystemTimePreciseAsFileTime from Windows 8).
Good idea.
No longer blocks: 1188250
Depends on: 1188250
Blocks: 1188250
No longer depends on: 1188250
Depends on: 1189066
Blocks: 1189066
No longer depends on: 1189066
Summary: Improve reliability of CPOW monitoring → Improve reliability of the CPOW clock
Created attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

Bug 1188616 - CPOW monitoring should use PRMJ_Now instead of PR_IntervalNow;r?jandem
Attachment #8641038 - Flags: review?(jdemooij)
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

https://reviewboard.mozilla.org/r/14413/#review13129

::: js/ipc/CPOWTimer.cpp:12
(Diff revision 1)
> +#include "vm/Time.h"

Why do you need this include? Does this file exist?

::: js/ipc/CPOWTimer.cpp:23
(Diff revision 1)
> -    startInterval_ = PR_IntervalNow();
> +    startInterval_ = PRMJ_Now();

I think PRMJ_Now is an internal JS function, it seems nicer to use JS_Now. It just forwards to PRMJ_Now but it's the offical API.

::: js/ipc/CPOWTimer.cpp:38
(Diff revision 1)
> -    PRIntervalTime endInterval = PR_IntervalNow();
> +    int64_t endInterval = PRMJ_Now();

And here.
Attachment #8641038 - Flags: review?(jdemooij)
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

Bug 1188616 - CPOW monitoring should use PRMJ_Now instead of PR_IntervalNow;r?jandem
Attachment #8641038 - Flags: review?(jdemooij)
https://reviewboard.mozilla.org/r/14413/#review13129

> Why do you need this include? Does this file exist?

Post bug 1181175, it's the include that defines `PRMJ_Now`, as per nbp's request.

> I think PRMJ_Now is an internal JS function, it seems nicer to use JS_Now. It just forwards to PRMJ_Now but it's the offical API.

Ok, will do.
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

https://reviewboard.mozilla.org/r/14413/#review13185

Thanks. JS_Now is probably a bit faster/nicer than PR_Now, for instance on Windows 8+ it uses GetSystemTimePreciseAsFileTime, but PR_Now should also work.
Attachment #8641038 - Flags: review?(jdemooij) → review+
Assignee: nobody → dteller
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem
Attachment #8641038 - Attachment description: MozReview Request: Bug 1188616 - CPOW monitoring should use PRMJ_Now instead of PR_IntervalNow;r?jandem → MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem
Comment on attachment 8641038 [details]
MozReview Request: Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem

Bug 1188616 - CPOW monitoring should use JS_Now instead of PR_IntervalNow;r=jandem
Sorry about the snafu.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a386c032c9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3a45337c8727
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.