setTimeout() introduces artificial delay between ticks when doing synchronous work

Affected Versions: 0.11+

The issue came up during testing of node-toobusy. I found that in newer Node versions, there seemed to be an artificial delay introduced between setTimeout() calls if there is a significant amount of synchronous work in them. The delay seems to always equal the amount of work done, as if Node is measuring the time taken by the function and delaying it by that amount of time to keep the loop free.

Reproduction script:

function tightWork(duration) {
  var start = Date.now();
  while ((Date.now() - start) < duration) {
    for (var i = 0; i < 1e5;) i++;
  }
}

var count = 0;
var last = Date.now();

function load() {
  if (count++ > 10) return;
  // This measures the amount of time between setTimeout() being called,
  // and it actually firing.
  console.log('tick delta:', (Date.now() - last));
  tightWork(100);
  last = Date.now();
  setTimeout(load, 0);
}

load();

Output (similar on 0.11.x and above):

$ node -v
v5.6.0
$ node index
tick delta: 13
tick delta: 1
tick delta: 105
tick delta: 104
tick delta: 105
tick delta: 105
tick delta: 105
tick delta: 104
tick delta: 105
tick delta: 105
tick delta: 105

Yet this doesn't happen on 0.10:

$ node -v
v0.10.41
$ node index
tick delta: 8
tick delta: 2
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1
tick delta: 1

Hmmm, interesting. here is the result if you use setImmediate():

tick delta: 13
tick delta: 1
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0
tick delta: 0

@STRML

@STRML I think you are running into some overhead or issue from Date.now() in newer versions of V8.

Here is an example using process.hrtime():

function tightWork(duration) {
  var start = process.hrtime()[1];
  while ((process.hrtime()[1] - start) < duration) {
    for (var i = 0; i < 1e5;) i++;
  }
}

var count = 0;
var last = process.hrtime()[1];

function load() {
  if (count++ > 10) return;
  // This measures the amount of time between setTimeout() being called,
  // and it actually firing.
  console.log('tick delta (ns):', 0.000001 * (process.hrtime()[1] - last));
  tightWork(100);
  last = process.hrtime()[1];
  setTimeout(load, 0);
}

load();

Output:

tick delta: 13.153296
tick delta: 2.8150939999999998
tick delta: 2.7386399999999997
tick delta: 1.390803
tick delta: 1.105172
tick delta: 2.57055
tick delta: 1.378833
tick delta: 1.256799
tick delta: 1.335664
tick delta: 1.1811589999999998
tick delta: 1.3384239999999998

@STRML

@STRML Excellent analysis and report, thank you very much for doing this!

I believe this bug was actually fixed in node v0.10.39 and up and v0.12.5 and up. Would you mind checking that these versions fix your problem?

It is still present in node v4.x and up though. There's an open PR to fix it in these versions at #3063.

This issue is also duplicate of nodejs/node-v0.x-archive#15447 and nodejs/node-v0.x-archive#9333, so I'll close it for now, but feel free to let me know if I missed something.

@Fishrock123 It seems that the sample code you mentioned uses the part of process.hrtime's return value that is in nanoseconds, and so it busy-waits for 1e5 nanoseconds, which is not enough time to make this issue noticeable.

@misterdjules

Great. Thanks for the context @misterdjules. I'm going to link #3063 here for reference since GitHub doesn't seem to have picked that one up.

I can confirm 0.10.40-41 and 0.12.9 do not exhibit this issue.

Reopening to have one issue open about this problem in nodejs/node and not just in joyent/node-v0.x-archive as suggested by @onlucky.

@Fishrock123

/cc @Fishrock123

@zhangzifa

In #7386 @zhangzifa has suggested to use this patch as a fix:

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..fa3b3b7 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -169,7 +169,7 @@ function listOnTimeout() {
     // Check if this loop iteration is too early for the next timer.
     // This happens if there are more timers scheduled for later in the list.
     if (diff < msecs) {
-      this.start(msecs - diff, 0);
+      this.start(msecs, 0);
       debug('%d list wait because diff is %d', msecs, diff);
       return;
     }

But I'm not sure that is correct, consider the following:

var timer_a = {
  _idleStart: 40000, // Time when `a` was scheduled from
  _idleTimeout: 2000 // Duration (`msecs`)
}

var timer_b = {
  _idleStart: 41000, // Time when `b` was scheduled from
  _idleTimeout: 2000 // Duration (`msecs`)
}

Both timers will go into the same underlying list (backed by a timer_wrap handle) because they have the same ._idleTimeout (duration), and thus hit the aforementioned logic.

With that "fix", timer_a will fire at ~42000, but timer_b will fire at ~4400, rather than at ~4300, when it is supposed to. This is because we would now be rescheduling the full timer duration (msecs) from the first called timer rather than checking if it should fire before that full duration...

I'm still quite puzzled by this problem. Investigating more.

@zhangzifa

In #7386 @zhangzifa has suggested to use this patch as a fix:

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..fa3b3b7 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -169,7 +169,7 @@ function listOnTimeout() {
// Check if this loop iteration is too early for the next timer.
// This happens if there are more timers scheduled for later in the list.
if (diff < msecs) {
 this.start(msecs - diff, 0);
 this.start(msecs, 0);
debug('%d list wait because diff is %d', msecs, diff);
return;
}
But I'm not sure that is correct

That patch is not correct exactly for the reasons @Fishrock123 outlined later in that comment:

we would now be rescheduling the full timer duration (msecs) from the first called timer rather than checking if it should fire before that full duration...

As mentioned before, this issue is fixed by #3063, which needs to be updated like described in #3063 in order for it to land.

The problem is well understood, and has been fixed for over a year in v0.10.x and v0.12.x versions. The only reason why this issue is still open is that the fix for these versions need to be ported (with #3063), it is not because it's a problem that needs to be solved.

#3063 should be ready to be merged. If we could get a few new "LGTM" I'll land it.

@whitlockjc

@whitlockjc Before landing #3063, #3063 (comment) will need to be addressed. But please keep the conversation about #3063 in #3063. Thank you!

@zhangzifa

@zhangzifa It seems that, with your latest comment, you are referring to a different problem than this issue describes.

I believe you are referring to the same problem as #7346. Would you mind checking if it's indeed the case, and if so commenting in that issue?

Thank you!

timers: fix processing of nested timers

Navigation Menu

Repository navigation

setTimeout() introduces artificial delay between ticks when doing synchronous work #5426

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

setTimeout() introduces artificial delay between ticks when doing synchronous work #5426

Description

Activity

Fishrock123 commented on Feb 25, 2016

Fishrock123 commented on Feb 25, 2016

misterdjules commented on Feb 25, 2016

STRML commented on Feb 25, 2016

misterdjules commented on Mar 23, 2016

MylesBorins commented on Mar 24, 2016

Fishrock123 commented on Jun 24, 2016

1 remaining item

misterdjules commented on Jul 5, 2016

whitlockjc commented on Jul 5, 2016

misterdjules commented on Jul 5, 2016

misterdjules commented on Jul 5, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions