1188616 - Improve reliability of the CPOW clock

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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

[Emanuel Hoogeveen [:ehoogeveen]]

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).

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Good idea.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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

Comment 6

[David Teller [:Yoric] - still alive but not very active]

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

[Jan de Mooij [:jandem]]

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.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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 9

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34697af2829

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e86b04035c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[David Teller [:Yoric] - still alive but not very active]

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 13

[David Teller [:Yoric] - still alive but not very active]

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 14

[David Teller [:Yoric] - still alive but not very active]

Sorry about the snafu.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a386c032c9

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3a45337c8727

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a45337c8727