New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 328700 link

Starred by 15 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 259680



Sign in to add a comment

Why is setInterval(..., N) interpreted as setInterval(...,N+1)?

Reported by jer...@duckware.com, Dec 14 2013

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1738.0 Safari/537.36

Steps to reproduce the problem:
Run this code, changing "5" to any value you want, like 5, 100, 10000:
------------------------------------
<!DOCTYPE html>
<html><body onload="setInterval(timer, 5)">
<script>
var _tick=0;
var _times=0;
function timer() {
  var tick = window.performance.now();
  if (++_times<20) {
    console.log((tick-_tick)+"ms");
    }
  _tick = tick;
  }
</script></body></html>
------------------------------------

What is the expected behavior?
timer() is called back in the requested number of milliseconds.

What went wrong?
timer() is called back in the requested number of milliseconds, PLUS ONE.

Did this work before? N/A 

Chrome version: 33.0.1738.0  Channel: canary
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 11.9 r900
 
setinterval.jpg
126 KB View Download

Comment 1 by tkent@chromium.org, Dec 17 2013

Cc: kbr@chromium.org
Labels: Cr-Blink

Comment 2 by kbr@chromium.org, Dec 17 2013

Cc: esprehn@chromium.org briander...@chromium.org jchaffraix@chromium.org
Labels: -OS-Windows OS-All
Interesting. This behavior does seem to be reliable. Reproducible in Chrome Canary on OS X too.

Needs someone to investigate this more deeply. Maybe there's an off-by-one error in the scheduling of the SharedTimer. Would be helpful if someone could dig up my most recent bug fix in this area, which involved a fencepost issue with timer scheduling.

Comment 3 Deleted

Comment 4 by kbr@chromium.org, Dec 21 2013

Blockedon: chromium:259680
Cc: jam...@chromium.org
The bug I was thinking of, where the timer alignment code wasn't working well, was  Issue 259680 .

Comment 5 by jer...@duckware.com, Dec 21 2013

My best guess for the cause of the +1 issue (and the fix)...

The trigger for this +1 issue is the (new) increased accuracy in monotonicallyIncreasingTime() -- from milliseconds to microseconds.

In ThreadTimers::sharedTimerFiredInternal, the "fireTime" in setNextFireTime(fireTime+interval) is set to monotonicallyIncreasingTime().  So, "fireTime" previously always looked like X.XXX000 (microseconds zero; so millisecond aligned).  But now, it looks like (X.XXXYYY), where YYY (microseconds) is almost certainly non-zero (due to microsecond accurate time).

And that non-zero YYY is the cause.  In WebKitPlatformSupportImpl::setSharedTimerFireInterval any sleep time is intentionally ROUNDED UP to a millisecond boundary.  So any non-zero microseconds in a sleep time will cause a +1 to the next millisecond boundary.

QUESTION: Was "fireTime+interval" where fireTime is set to monotonicallyIncreasingTime() intentional (as opposed to basing it off of m_nextFireTime)?  Worded another way, was any operating system DELAY in calling sharedTimerFiredInternal() intentionally DESIGNED to propagate to all future JavaScript timeouts – or was that an accident?  If that delay propagation was intentional, then the fix for the +1 issue must be found elsewhere. (However, delays in calling multiple JavaScript callbacks are NOT propagated, which argues that an OS delay propagation to JavaScript was unintentional).

So assuming that the OS delay propagation was unintentional, the fix for the +1 issue is to change the argument in setNextFireTime() from:

    interval ? fireTime + interval : 0

to:

    interval ? m_nextFireTime+floor((fireTime-m_nextFireTime+interval)/interval)*interval : 0

The intent of this formula (for interval non-zero) is to produce a ‘next time’ that equals what m_nextFireTime would be set to after executing this code:

    while (m_nextFireTime<=fireTime) {
        m_nextFireTime += interval;
        }

which is just the 'the next interval-aligned time (from the last scheduled fire time), which is AFTER the current fire time' (and of course, this argument must be computed before m_nextFireTime is set to zero).  This formula could be simplified if assumptions are made, but it is designed to handle these (edge) conditions:

  - m_nextFireTime equals fireTime
  - interval could be variable (isn’t it in background tabs?)
  - accounts for missed intervals

Of course, this is all from looking at source code online (not actually running code), so I could be wrong...

> The bug I was thinking of, where the timer alignment code wasn't working well, was   Issue 259680  .

@kbr's patch in  issue 259680  is related to setInterval, but I don't see how that patch could have introduced this behavior unless the rounded down time is eventually used to calculate the delay for the next setInterval event.

> The trigger for this +1 issue is the (new) increased accuracy in monotonicallyIncreasingTime() -- from milliseconds to microseconds.

Do we have proof that there is a regression here due to the precision? I'd be surprised if precision was the cause, because all platforms other than Windows have always used high precision timers.  Canary is the only version of Chrome that will use higher precision timers on Windows at the moment. I don't have a Windows machine right now, but a quick test would be to see if this behavior is the same in stable since stable still uses low-precision timers.

Regarding what behavior we should be targeting:
On my Macbook Air, I don't see the exact +1 behavior being reported here. Instead, I see the actual interval is always above the requested interval, but can be within a few microseconds of the requested time. I still don't think this is correct though. I tested with Firefox and the actual intervals are above and below the requested interval, which likely means they are trying to prevent the phase from drifting. Avoiding drift is the right thing to do IMHO.

I'm not familiar with what timing code is actually active without adding a number of trace events. I'll try taking a look at this in detail on Thursday if we don't have a solution by then.  Thanks @jerryj for doing some preliminary investigation.
Cc: fmea...@chromium.org
> Do we have proof that there is a regression here...

Yes (at least under Windows).  Attached is output from http://www.duckware.com/test/chrome/setinterval.html from Chrome 31.0.1650.63 m, where the deltas are 100ms (instead of 101ms from Canary).  However, note there are certain 'N' under stable that *do* cause the +1 situation (at least for me; still trying to figure out the root cause of that), like N=200.

> I tested with Firefox and the actual intervals are above and below the requested interval ... Avoiding drift is the right thing to do IMHO.

Regardless of the proposed fix above fixing the "N+1" issue, the proposed fix above 'should' fix the drift issue in Chrome (someone then needs to check the proposed fix also makes the patch in  issue 259680  moot).

A WARNING: For anyone testing under Windows, CLOSE ALL APPS except for the browser being tested.  This is because the timeBeginPeriod() that apps use to obtain more accurate timers (and sleeps/etc) is a "global Windows setting".  Firefox is only accurate to 15.625ms, but open a Chrome window, and Firefox all of sudden is accurate to 1ms -- because Chrome uses timeBeginPeriod(1), a global setting, that then affects Firefox.

TIP: Under a Windows DOS (administrator) window , enter "powercfg /energy -duration 5" and look for any 'Outstanding Timer Request' in the generated 'energy-report.html' -- easy to spot what app is causing the system to go into high-resolution timer mode.
chrome-31-0-1650-63-m.jpg
119 KB View Download
> I'd be surprised if precision was the cause, because all platforms other than Windows have always used high precision timers.

Under Windows... setInterval(...,100) under "Version 31.0.1650.63 m" results in 100ms delays logged to the console.  But then run the same chrome version with --enable-high-resolution-time, and 101ms delays are logged to the console.

The cause of the N+1 issue in Windows non-high-resolution time mode for very specific values of N has been found.  See  Issue 331502 

Comment 11 by kbr@chromium.org, Jan 3 2014

Mergedinto: 331502
Status: Duplicate
Thanks for the investigation. I'm going to duplicate this bug into the other so that all of the people are properly CC'd and the other bug is cross-referenced.

The status of this bug as closed and a duplicate of  Issue 331502  is in ERROR.  The two issues are completely independent.

Comment 13 by kbr@chromium.org, Jan 9 2014

Mergedinto:
Status: Untriaged
OK -- un-duplicating.

Labels: Cr-Internals-GPU-Scheduling
Will this issue be addressed/fixed, especially since the fix is known (see comment #5)?
Will this issue be addressed/fixed, especially since the fix is known (see comment #5)?
Cc: a deleted user
Labels: -Pri-2 Pri-1
Owner: briander...@chromium.org
Status: Assigned
Bumping priority and assigning to self.
Cc: -a deleted user sunn...@chromium.org
Labels: -Cr-Internals-GPU-Scheduling Hotlist-Scheduling Cr-Internals-Compositing
Cc: alexclarke@chromium.org skyos...@chromium.org
Labels: -Cr-Internals-Compositing
Sami, Alex, before I dig into this - is this already a known issue? Will any of the upcoming timer heap changes fix this?
Yes, we ran into this previously but looks like we failed to file a bug for it. The timer heap refactoring will retain this behavior for backwards compatibility but AFAIK there's no actual reason to keep the the extra millisecond there.

You might want to wait until the timer heap goes away (https://codereview.chromium.org/956333002/) since it will change a lot of the code around this. Also see crbug.com/402694 for a related issue and some problems we ran into while fixing it.
 Issue 434749  has been merged into this issue.
On Windows stable (Version 43.0.2357.130 m) I see the timer firing about every 5.2 ms. A recent change that is in canary (https://codereview.chromium.org/1154953002) stops Chrome on Windows from busy-waiting for the exact requested time, which means that waits are rounded up to the next ms boundary, which means that the timer then fires every 6.0 ms (more or less).

This may make now a good time to fix the underlying bug as suggested in comment #5, to make setInterval more stable.

See also this fix to get FileVideoCaptureDevice to avoid drifting: https://codereview.chromium.org/1162903004/

Labels: hotlist-trend-0701
After seeing some strange timer behavior at www.vsynctester.com under Canary (using custom hz, which internally is implemented via setTimeout), I retested this issue and today, under Canary 46.0.2468.0, the N+1 issue appears to have changed to N+2.

Using the original example code in this issue, "setInterval(timer, 5)" now results in a 7ms interval.  See attached.
timing.jpg
54.6 KB View Download
Thanks for pointing this out Jerry. Now that the code for timers in Blink has been rewritten[1] I think we could do some simple changes to make setInterval() more robust.

[1] Probably what caused this regression since we tried to preserve the drifting behavior of the earlier code but couldn't do it perfectly.
skyos/27: Rather than maintaining prior bad behavior, why can't timers just be make 100% accurate?  After all, if you now have a non-conforming N+2 that apparently no one else has noticed (how did it even pass quality control), making a confirming 'N' (fully 100% accurate) would likely go just as unnoticed as well?
> Rather than maintaining prior bad behavior, why can't timers just be make 100% accurate?

Right, that's exactly what I was proposing. We didn't do that initially in the refactor just to avoid changing too many things at once.
skyos/29: ok, thanks.  Please keep in mind that www.vsynctester.com can be used to very effectively graph how well setTimeout() is working (or not).  At that site, turn off ALL options, then turn back on the 'inter-frame' and 'late' performance lines, then turn on the custom Hz option (implemented via setTimeout) and play around with various Hz values, and visually see how well/accurate setTimeout() is working...
Cc: -alexclarke@chromium.org picksi@chromium.org
Owner: alexclarke@chromium.org
So I think we can make this better, there's a couple of things that currently prevent drift free setInterval:

1. TimerBase::runInternal does not account for the difference between when a timer was due to fire and when it actually did.  I've known about this for a while, but landing the timer refactor was hard enough without altering stuff like this.  Still I think the dist has settle sufficiently for is to change that.

2. The interface between blink and chromium for delayed tasks accepts an integer delay in miliseconds.  This doesn't provide enough resolution to properly schedule the next task.  We should use a double here.

Still we need to be realistic about our expectations of the browser's ability to actually run the task at the requested time.  The thread is cooperatively multi-tasked and not all tasks play nice (if something hogs the thread for a long time, you're out of luck) and the renderer scheduler may decide to run something else first that it deems (rightly or wrongly) to be more important.  Also there's some overhead between the OS sending the chromium message loop the delayed wakeup and the js actually getting executed, since it has to jump through a bunch of layers to get there.  Finally we need to accept that V8 may need to do some compilation which will  affect at least the first couple of calls the function.

Also on windows Now() isn't always high resolution, I suspect that can cause additional inaccuracy.

Labels: -Pri-1 Pri-2
alexcla/31: "we need to be realistic about our expectations of the browser's ability to actually run the task at the requested time".  OK, let's be "realistic"...

The entire problem is that Chrome timer internals is a mess and buggy (after personally reviewing the code first hand).  Stop blaming multiple threads, the scheduler, overhead, V8, etc -- as those excuses are provably just a red herring.

Because I can fix the problem in JavaScript by adding two extra characters into JavaScript.  Say I want a timer to fire every 8ms, so I call "setInterval(timer,8)".  Canary then calls timer() back every 10ms.  But then I change the call to "setInterval(timer,8-2)" and voila, Canary then calls timer() back VERY RELIABLY every 8ms.  See attached evidence.

Be "realistic" and put the blame for this issue squarely where it belongs -- on the ugly timer code inside Chrome.  Don't blame other issues that have nothing to do with the problem.
alexcla6-8.jpg
59.1 KB View Download
alexcla8-10.jpg
61.3 KB View Download
I cannot reproduce this on Chrome Unstable 46.0.2467.2 dev on Ubuntu 14.04. This is probably Windows only.
Cc: brucedaw...@chromium.org
https://codereview.chromium.org/1154953002 may have come into play recently and is Windows only. If so, it would be nice to figure out a way to fix this issue while still keeping the power optimization.
As a data point, on a Win7 box with M44 I'm seeing the following: 

MS:  run test
5.325ms +3.23
5.176ms +3.40
5.147ms +3.55
5.300ms +3.85
5.128ms +3.98
5.123ms +4.10
5.119ms +4.22
5.168ms +4.39
5.117ms +4.51
5.126ms +4.63
5.143ms +4.77
5.137ms +4.91
5.138ms +0.05
5.141ms +0.19
5.139ms +0.33
5.141ms +0.47
5.139ms +0.61
5.154ms +0.76
5.198ms +0.96

We can see the timer drifting, because the +0.xx numbers (which are time modulo interval) are not consistent.  The patch I have out for review make this very consistent on the long term.  The timers are taking ~100ms longer to fire than they should because the chrome - blink interface currently takes an integer number of miliseconds as the delay.  Changing that to a double gets rid of most of the delay.
bisects:

r331218 - +0.1 behavior
r331228 - +1.0 behavior

r333283 - +1.0 behavior
r333334 - +2.0 behavior

Project Member

Comment 38 by bugdroid1@chromium.org, Aug 3 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef61ed25f923dbb4a7330beece3f788571200fdc

commit ef61ed25f923dbb4a7330beece3f788571200fdc
Author: alexclarke <alexclarke@chromium.org>
Date: Mon Aug 03 14:06:46 2015

Chromium support for WebScheduler::postTimerTask with high precision

This is needed for (relativly) drift free setInterval.

BUG= 328700 

Review URL: https://codereview.chromium.org/1256823007

Cr-Commit-Position: refs/heads/master@{#341522}

[modify] http://crrev.com/ef61ed25f923dbb4a7330beece3f788571200fdc/components/scheduler/child/web_scheduler_impl.cc
[modify] http://crrev.com/ef61ed25f923dbb4a7330beece3f788571200fdc/components/scheduler/child/web_scheduler_impl.h

Project Member

Comment 39 by bugdroid1@chromium.org, Aug 3 2015

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=199903

------------------------------------------------------------------
r199903 | alexclarke@chromium.org | 2015-08-03T17:21:40.775857Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/public/platform/WebScheduler.h?r1=199903&r2=199902&pathrev=199903
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/Timer.cpp?r1=199903&r2=199902&pathrev=199903
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/TimerTest.cpp?r1=199903&r2=199902&pathrev=199903

Fix the drift in repeating timers.

Currently repeating blink timers ignore the difference between when the
timer was due to fire and when it actually did.  In addition the delay
reported to chromium was rounded up to the nearest millisecond,
introducing unnecessary extra delay.

(Note seperatly DOMTimers have additional clamping).

This fairly broken behaviour has been there for a long time, and I
really hope nothing has come to depend on it!

Judging by
http://www.duckware.com/test/chrome/timeraccuracy.html this patch makes
things a lot better at least on linux. The actual delays are much
closer to the requested ones, and the run time modulo interval shows
the drift is gone.

BUG= 328700 

Review URL: https://codereview.chromium.org/1261993006
-----------------------------------------------------------------
On linux with a tip of tree build I now get the following timings.

5.200ms +4.85
5.000ms +4.85
4.980ms +4.83
5.015ms +4.85
5.010ms +4.86
4.955ms +4.81
5.040ms +4.85
4.995ms +4.85
4.995ms +4.84
4.990ms +4.83
5.010ms +4.84
5.000ms +4.84
5.000ms +4.84
5.000ms +4.84
5.000ms +4.84
5.000ms +4.84
5.000ms +4.84
5.005ms +4.85
4.995ms +4.84

Realistically it's not going to get much better than that for the reasons mentioned in #31 (note by scheduler I mean the chrome renderer scheduler not the kernel scheduler).

If I get time later I'll try and build on windows too, to see what the state of play is there.

Comment 41 Deleted

Status: Fixed
Project Member

Comment 43 by bugdroid1@chromium.org, Aug 12 2015

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=200375

------------------------------------------------------------------
r200375 | jam@chromium.org | 2015-08-12T03:11:58.296573Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/public/platform/WebScheduler.h?r1=200375&r2=200374&pathrev=200375
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/Timer.cpp?r1=200375&r2=200374&pathrev=200375
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/TimerTest.cpp?r1=200375&r2=200374&pathrev=200375

Revert of Fix the drift in repeating timers. (patchset #2 id:20001 of https://codereview.chromium.org/1261993006/ )

Reason for revert:
this change caused a large amount of flakes, see https://code.google.com/p/chromium/issues/detail?id=517488

and also graphs on 
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.linux%20Builder%20linux_chromium_rel_ng%20Success%20Rate%20(Last%20100%20Builds)
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder%20win_chromium_x64_rel_ng%20Success%20Rate%20(Last%20100%20Builds)
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder%20win_chromium_rel_ng%20Success%20Rate%20(Last%20100%20Builds)

this was hard to track down because browser_tests weren't printing renderer crashes. i fixed that in an unlanded cl and tried reenabling all the tests and they all had
ASSERTION FAILED: now >= m_unalignedNextFireTime
see the try runs on this patchset
https://codereview.chromium.org/1284083002/#ps20001
i.e. one example:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/92022/steps/browser_tests%20%28with%20patch%29/logs/MSE_ClearKey_Prefixed_EncryptedMediaTest.Playback_Multiple_VideoAudio_WebM_0

BUG= 517488 

Original issue's description:
> Fix the drift in repeating timers.
> 
> Currently repeating blink timers ignore the difference between when the
> timer was due to fire and when it actually did.  In addition the delay
> reported to chromium was rounded up to the nearest millisecond,
> introducing unnecessary extra delay.
> 
> (Note seperatly DOMTimers have additional clamping).
> 
> This fairly broken behaviour has been there for a long time, and I
> really hope nothing has come to depend on it!
> 
> Judging by
> http://www.duckware.com/test/chrome/timeraccuracy.html this patch makes
> things a lot better at least on linux. The actual delays are much
> closer to the requested ones, and the run time modulo interval shows
> the drift is gone.
> 
> BUG= 328700 
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199903

TBR=jochen@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 328700 

Review URL: https://codereview.chromium.org/1290633002
-----------------------------------------------------------------

Comment 44 by jam@chromium.org, Aug 12 2015

Status: Assigned
reopening since I reverted the change
Labels: -Hotlist-Scheduling Cr-Blink-Scheduler
Labels: -Cr-Blink-Scheduler Cr-Blink-Scheduling
Project Member

Comment 47 by bugdroid1@chromium.org, Sep 23 2015

Labels: merge-merged-2490
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/559d0cced70cdc0d4e68cd3bcefcbadd69b2e7f8

commit 559d0cced70cdc0d4e68cd3bcefcbadd69b2e7f8
Author: jam@chromium.org <jam@chromium.org>
Date: Wed Aug 12 03:11:58 2015

Revert of Fix the drift in repeating timers. (patchset #2 id:20001 of https://codereview.chromium.org/1261993006/ )

Reason for revert:
this change caused a large amount of flakes, see https://code.google.com/p/chromium/issues/detail?id=517488

and also graphs on 
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.linux%20Builder%20linux_chromium_rel_ng%20Success%20Rate%20(Last%20100%20Builds)
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder%20win_chromium_x64_rel_ng%20Success%20Rate%20(Last%20100%20Builds)
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium.win%20Builder%20win_chromium_rel_ng%20Success%20Rate%20(Last%20100%20Builds)

this was hard to track down because browser_tests weren't printing renderer crashes. i fixed that in an unlanded cl and tried reenabling all the tests and they all had
ASSERTION FAILED: now >= m_unalignedNextFireTime
see the try runs on this patchset
https://codereview.chromium.org/1284083002/#ps20001
i.e. one example:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/92022/steps/browser_tests%20%28with%20patch%29/logs/MSE_ClearKey_Prefixed_EncryptedMediaTest.Playback_Multiple_VideoAudio_WebM_0

BUG= 517488 

Original issue's description:
> Fix the drift in repeating timers.
> 
> Currently repeating blink timers ignore the difference between when the
> timer was due to fire and when it actually did.  In addition the delay
> reported to chromium was rounded up to the nearest millisecond,
> introducing unnecessary extra delay.
> 
> (Note seperatly DOMTimers have additional clamping).
> 
> This fairly broken behaviour has been there for a long time, and I
> really hope nothing has come to depend on it!
> 
> Judging by
> http://www.duckware.com/test/chrome/timeraccuracy.html this patch makes
> things a lot better at least on linux. The actual delays are much
> closer to the requested ones, and the run time modulo interval shows
> the drift is gone.
> 
> BUG= 328700 
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199903

TBR=jochen@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 328700 

Review URL: https://codereview.chromium.org/1290633002

git-svn-id: svn://svn.chromium.org/blink/trunk@200375 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[modify] http://crrev.com/559d0cced70cdc0d4e68cd3bcefcbadd69b2e7f8/third_party/WebKit/Source/platform/Timer.cpp
[modify] http://crrev.com/559d0cced70cdc0d4e68cd3bcefcbadd69b2e7f8/third_party/WebKit/Source/platform/TimerTest.cpp
[modify] http://crrev.com/559d0cced70cdc0d4e68cd3bcefcbadd69b2e7f8/third_party/WebKit/public/platform/WebScheduler.h

Project Member

Comment 48 by bugdroid1@chromium.org, Sep 29 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d7bb8a499af076a64680f36b7b9c5b64fe334d0

commit 0d7bb8a499af076a64680f36b7b9c5b64fe334d0
Author: alexclarke <alexclarke@chromium.org>
Date: Tue Sep 29 12:37:13 2015

Fix the drift in repeating timers (try #2)

Currently repeating blink timers ignore the difference between when the
timer was due to fire and when it actually did.  In addition the delay
reported to chromium was rounded up to the nearest millisecond,
introducing unnecessary extra delay.

(Note seperatly DOMTimers have additional clamping).

This fairly broken behaviour has been there for a long time, and I
really hope nothing has come to depend on it!

Judging by
http://www.duckware.com/test/chrome/timeraccuracy.html this patch makes
things a lot better at least on linux. The actual delays are much
closer to the requested ones, and the run time modulo interval shows
the drift is gone.

Based on https://codereview.chromium.org/1261993006/ but without the
assert.  I've added test coverage to make sure nothing bad happens
if the timer fires a little earlier than expected (which can happen
since the delay gets quantized down to milliseconds on some platforms).

BUG= 328700 

Review URL: https://codereview.chromium.org/1373503002

Cr-Commit-Position: refs/heads/master@{#351295}

[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/components/scheduler/child/web_task_runner_impl.cc
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/components/scheduler/child/web_task_runner_impl.h
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/third_party/WebKit/Source/platform/Timer.cpp
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/third_party/WebKit/Source/platform/TimerTest.cpp
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp
[modify] http://crrev.com/0d7bb8a499af076a64680f36b7b9c5b64fe334d0/third_party/WebKit/public/platform/WebTaskRunner.h

Status: Fixed
Hopefully that patch will stay landed this time :)

Sign in to add a comment