performance.now() pauses during sleep on Linux and macOS, which is inconsistent with the High Resolution Time spec
Categories
(Core :: Performance, defect)
Tracking
()
Performance Impact | none |
People
(Reporter: bugzilla, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxp])
Attachments
(2 files)
8.57 KB,
patch
|
Details | Diff | Splinter Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
Expected Behavior
Calling performance.now()
provides an accurate monotonic clock, including system sleep time. For additional details on the W3C High Resolution Time recommendation and explanation of similar noncompliant behavior in Chrome (on macOS, Linux, Android, and Fuchsia), see Issue #4713 on MDN.
Steps to Reproduce
On a Linux device...
- Open a tab, navigate to content, open a browser console, and run
performance.now()
. - Put the device to sleep for a period of time, then wake the device.
- In the same open tab, run
performance.now()
. The difference in timestamps will not include the time that the system was suspended.
Proposed Resolution
It appears that the issue is in TimeStamp_posix.cpp
. The implementation of TimeStamp
on POSIX systems relies on CLOCK_MONOTONIC
, which includes sleep time on macOS 10.12+ but doesn't include sleep time on Linux. Using CLOCK_BOOTTIME
on Linux should resolve the issue, but may pose other challenges as discussed in the related issues.
Related Issues
Comment 1•4 years ago
|
||
The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.
Reporter | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 3•4 years ago
|
||
Jonathan has done a great summary on this today in the WebPerf meeting. A summary is that we generally agree that the clock behaviour should be the same across all implementations and across all platforms, however it's very complex to change the behaviour. Not only due to the compatibilty concerns, but also for things like if you change the behaviour for performance.now()
, many other timings which rely on performance.now()
are also going to be changed.
Reporter | ||
Comment 4•4 years ago
|
||
Thanks Sean! For what it's worth, during the conversation today, I found it helpful to think about breakage risks in two categories.
- There are breakage risks from changing browser monotonic clock APIs (e.g.,
TimeStamp
in Firefox). They're used all over the place, and who knows what expectations callers have accumulated over the years. An alternative path forward would be to create a new type of well-defined monotonic clock that ticks during sleep, then use the new clock forPerformance
andDOMHighResTimeStamp
implementations. That's not ideal—it involves developing and maintaining another type of clock, and it involves tracking down all the uses ofDOMHighResTimeStamp
outside ofPerformance
—but it might be less painful. Firefox already has a version of this concept inAwakeTimeStamp
(and, to a lesser extent,Uptime
). - There are breakage risks from changing the behavior of
Performance
andDOMHighResTimeStamp
for web content. It's a positive sign, though, that there's already correct behavior on major platforms and that hasn't caused test failures or bug reports.
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 5•4 years ago
|
||
Thanks to Paul for flagging that macOS is still using TimeStamp_darwin.cpp
, because the switch to TimeStamp_posix.cpp
was backed out. After a little more testing, I was able to reproduce this behavior in Firefox 88.0.1 on macOS 11.3.1. (I suspect I didn't spot the issue in prior testing because I didn't wait long enough in system sleep; macOS was in a light sleep mode where the clock continued to tick.) I've updated the title to include macOS.
Reporter | ||
Comment 6•4 years ago
|
||
Here's an initial implementation of a TimeStamp
variation that ticks during sleep on all platforms. If this direction seems reasonable, the next step would be updating performance.now()
and all the other DOMHighResTimeStamp
uses. I haven't included FuzzyFox support since that's getting removed (Bug 1666222).
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 7•4 years ago
|
||
Gabriele asked about replacing TimeStamp
entirely, rather than implementing a new class. I completely agree with the end goal; there should be no ambiguity about expected system sleep behavior. In the near term, I lean toward implementing a new class, for a couple reasons.
-
TimeStamp
is used throughout the Mozilla codebase for a wide variety of purposes, with unclear expectations from callers. In some instances, swappingTimeStamp
withAwakeTimeStamp
orBootTimeStamp
would be trivial. In others, I wouldn't be confident making the switch without a close look from a module owner. -
There's a bunch of legacy functionality in
TimeStamp
, such as the FuzzyFox implementation,QueryPerformanceCounter
corner cases for old hardware, and slightly varying timestamps and timestamp math. I wouldn't be confident making decisions about which functionality is necessary, outdated, or bloat.
The approach I had in mind was recommending AwakeTimeStamp
and BootTimeStamp
as lean versions of TimeStamp
with well-defined system sleep behavior, deprecating TimeStamp
in favor of the two new classes, and then gradually replacing TimeStamp
.
Comment 8•4 years ago
|
||
SGTM. I feel your worry about replacing TimeStamp,
I did some heavy surgery on that class in the past and testing that your change works everywhere is challenging. While I don't expect any surprises in switching from CLOCK_MONOTONIC
to CLOCK_BOOTTIME
on desktop Linux, I expect plenty of problems on Android. We've encountered buggy kernel implementations of CLOCK_MONOTONIC
there (cough NVidia cough) so I expect more problems with CLOCK_BOOTTIME
.
Since FuzzyFox support is being removed (see bug 1666222) we can do the following:
- Introduce the new
BootTimeStamp
class - Change
TimeStamp
toBootTimeStamp
where it fixes known problems (e.g.Performance.now()
) - File a bug to evaluate using
BootTimeStamp
as a replacement forTimeStamp
after FuzzyFox is removed
Note, the Windows part of TimeStamp
is high on my priority list of things that needs to be overhauled so we might as well do that part within 3.
Updated•3 years ago
|
Comment 9•2 years ago
|
||
Clearing this needinfo (as part of going through my old needinfos :) ).
https://github.com/mdn/content/issues/4713 is still open
Updated•1 year ago
|
Updated•1 year ago
|
Comment 10•20 days ago
|
||
User of Unity reported this as an issue just recently. I developed a monkeypatch to fix the issue in JS land:
(()=>{
var p0 = performance.now(), d0 = Date.now(), offset = 0, P = performance;
P.now_ = P.now;
P.now2 = () => {
var p = performance.now_(), d = Date.now(), po = p + offset, dd = d - d0;
if (dd-po+p0 > 1e4) ret = p0 + dd, offset = ret - po;
else ret = po;
d0 = d;
return p0 = ret;
};
})();
The above code cross-references against Date.now()
to hide any big >10s jumps in performance.now()
.
Used one of those convenient JS microbenchmarking sites to measure the performance impact of such a monkeypatch:
Firefox Windows:
performance.now()
: 606 Mops/sec- monkeypatched: 10Mops/sec
Chrome Windows:
performance.now()
: 10 Mops/sec- monkeypatched: 5 Mops/sec
Quite happy to see how hyperoptimized Firefox performance was on performance.now()
, (assuming Firefox didn't optimize out the performance.now()
altogether), but that really makes the monkeypatch version look super bad.
In any case, I hope the performance impact of the implementation of performance.now()
will not be worsened after this issue is fixed, since performance overhead in performance.now()
can cause quality problems to e.g. real-time game engines timing.
Comment 11•19 days ago
|
||
How about we try this, since now we have https://searchfox.org/mozilla-central/source/mozglue/misc/AwakeTimeStamp.cpp, that we've been using for some time in media, and works well? I've recently added a non "low resolution" for all platforms.
Comment 12•11 days ago
|
||
Yeah, that's a good idea. We haven't encountered a single issue with that one and it's been a while.
Comment 13•8 days ago
|
||
Ripple effect is a bit high for this, at least:
- DOMNavigation API
- Glean
- Firefox Profiler
I'm started munching through that, but it's even more worrying that all of this is incorrect (and inconsistent between platform).
Comment 14•8 days ago
|
||
Comment 15•8 days ago
|
||
WIP patch attached that does a lot of it, but it goes everywhere and I'm not sure I have that kind of time right now.
Description
•