Closed
Bug 1188616
Opened 9 years ago
Closed 9 years ago
Improve reliability of the CPOW clock
Categories
(Toolkit :: Performance Monitoring, defect)
Toolkit
Performance Monitoring
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(1 file)
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 .
Comment 1•9 years ago
|
||
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).
Assignee | ||
Comment 2•9 years ago
|
||
Good idea.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Summary: Improve reliability of CPOW monitoring → Improve reliability of the CPOW clock
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1188616 - CPOW monitoring should use PRMJ_Now instead of PR_IntervalNow;r?jandem
Attachment #8641038 -
Flags: review?(jdemooij)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → dteller
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34697af2829
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e86b04035c
Keywords: checkin-needed
Comment 11•9 years ago
|
||
Backed out for bustage. Please verify fully green on Try before the next checkin-needed request. https://treeherder.mozilla.org/logviewer.html#?job_id=12418972&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/1d20bd2d28d0
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
Sorry about the snafu. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a386c032c9
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3a45337c8727
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a45337c8727
Status: NEW → RESOLVED
Closed: 9 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.
Description
•