timers: improve setImmediate() performance #8655

mscdex · 2016-09-19T17:44:41Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

timers

Description of change

This commit avoids re-creating a new immediate queue object every time the immediate queue is processed. Additionally, a few functions are tweaked to make them inlineable.

These changes result in ~6-7% improvement in the existing setImmediate() benchmarks and should help reduce GC work.

Fishrock123 · 2016-09-19T17:51:58Z

lib/timers.js



 function processImmediate() {
-  const queue = immediateQueue;
-  var domain, immediate;
+  var p = immediateQueue.head;


p is poorly named

Changed to ptr. Is that sufficient?

why not just keep it as immediate?

mscdex · 2016-09-19T17:59:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4131/

mscdex · 2016-09-19T23:06:27Z

I've now tweaked a few of the functions used by setImmediate() and others to be inlineable, which gives another ~3% boost in performance, upping the improvement to ~6-7%.

mscdex · 2016-09-19T23:07:58Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/4138/

richardlau · 2016-09-20T04:30:23Z

lib/internal/process/promises.js

@@ -45,6 +45,21 @@ function setupPromises(scheduleMicrotasks) {
    }
  }

+  function emitWarning() {
+    const warning = new Error('Unhandled promise rejection ' +
+                              `(rejection id: ${uid}): ${reason}`);


uid and reason not defined here.

mscdex · 2016-09-20T06:26:30Z

CI again again: https://ci.nodejs.org/job/node-test-pull-request/4143/

Fishrock123 · 2016-09-20T14:47:59Z

lib/internal/process/promises.js

-              'Node.js process with a non-zero exit code.',
-              'DeprecationWarning');
-          }
+          emitWarning(uid, reason);


@mscdex err, why is this included?

To allow for more inlining. emitPendingUnhandledRejections() is used by setImmediate().

jasnell

LGTM

imyller

LGTM

jasnell · 2016-09-27T14:09:16Z

ping @Fishrock123 ... does this LGTY?

Fishrock123

Functionality LGTM, I'd like some commenting around the new linkedlist stuff personally, I find them notoriously hard to read / understand and I doubt I am alone.

OK otherwise if CI passes.

Fishrock123 · 2016-09-27T14:11:55Z

lib/timers.js


-  immediateQueue = L.create();
+  immediateQueue.head = immediateQueue.tail = null;


a comment would be helpful

What did you have in mind? To me it's self-explanatory, it's effectively clearing the linked list.

I know what it does, I think it just may look strange to someone else encountering it.

Fishrock123 · 2016-09-27T14:13:36Z

lib/timers.js

+        immediateQueue.head = next;
+      } else {
+        immediateQueue.head = next;
+        immediateQueue.tail = oldTail;


Can this be a helper fn?

What do you mean?

Fishrock123 · 2016-09-27T14:16:04Z

lib/timers.js

+    this.head = item;
+  }
+  this.tail = item;
+};


I probably should specify: comments on how these two modify their respective properties

mscdex · 2016-09-29T08:16:27Z

@Fishrock123 Comments added, let me know if they are sufficient.

mscdex · 2016-10-05T01:57:26Z

ping @Fishrock123

Fishrock123

lgtm

mscdex · 2016-10-05T03:21:03Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/4389/

EDIT: checking again after rebasing just to be sure flakiness is unrelated: https://ci.nodejs.org/job/node-test-commit/5451/

This commit avoids re-creating a new immediate queue object every time the immediate queue is processed. Additionally, a few functions are tweaked to make them inlineable. These changes give ~6-7% boost in setImmediate() performance in the existing setImmediate() benchmarks. PR-URL: nodejs#8655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

This commit avoids re-creating a new immediate queue object every time the immediate queue is processed. Additionally, a few functions are tweaked to make them inlineable. These changes give ~6-7% boost in setImmediate() performance in the existing setImmediate() benchmarks. PR-URL: #8655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins · 2016-10-06T23:56:17Z

adding to lts watch but unsure if this should land. Will definitely need time to sit in a release for a while

/cc @mscdex

This commit avoids re-creating a new immediate queue object every time the immediate queue is processed. Additionally, a few functions are tweaked to make them inlineable. These changes give ~6-7% boost in setImmediate() performance in the existing setImmediate() benchmarks. PR-URL: #8655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Conflicts: lib/internal/process/promises.js

ErisDS · 2016-10-13T08:51:37Z

Hey there!

We started having test failures due to this change. There's more information on the issue & PR linked just above this comment. To pull some of that here:

The last job never get's executed, because setImmediate does not get triggered!

We have a temporary fix in place in the PR, that reduces the timeout and this appears to work (the tests pass). Would love a little bit of input into how this change has impacted the functionality and whether we've found a bug or are doing something wrong :)

MylesBorins · 2016-10-13T08:54:34Z

/cc @nodejs/ctc

Trott · 2016-10-13T18:47:01Z

@ErisDS I don't suppose you have a minimal test that could be used to demonstrate the immediate running in v6.7.0 and not running in v6.8.0, do you? I'm guessing not because you probably would have mentioned it, but I'm having trouble replicating the problem, probably because I am Doing Something Wrong.

kirrg001 · 2016-10-13T18:52:24Z

@Trott i will provide an example file asap

kirrg001 · 2016-10-13T18:59:55Z

var jobs = [Date.now() + 1000, Date.now() + 2000, Date.now() + 3000];

jobs.forEach(function(timestamp) {
    var timeout = setTimeout(function() {
        clearTimeout(timeout);

        (function retry() {
            var immediate = setImmediate(function() {
                clearImmediate(immediate);

                if (Date.now() < timestamp) {
                    return retry();
                }

                console.log("FINISHED JOB");
            });
        }());
    }, timestamp - 200);
});

v6.8.0 - does not work, you will see 1 x FINISHED JOB
v4.4.7 && v6.7.0 - works as expected, you will see 3 x FINISHED JOB

Fishrock123 · 2016-10-13T19:13:51Z

Moving to a new issue so we can help you better: #9084

please put all comments in that thread rather than here

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 19, 2016

Fishrock123 reviewed Sep 19, 2016

View reviewed changes

mscdex force-pushed the timers-improve-immediate-perf branch from 5f4c590 to a284f41 Compare September 19, 2016 17:59

mscdex force-pushed the timers-improve-immediate-perf branch from a284f41 to dcc9fe0 Compare September 19, 2016 23:04

richardlau reviewed Sep 20, 2016

View reviewed changes

mscdex force-pushed the timers-improve-immediate-perf branch from dcc9fe0 to e66340a Compare September 20, 2016 06:25

Fishrock123 reviewed Sep 20, 2016

View reviewed changes

jasnell approved these changes Sep 20, 2016

View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

imyller approved these changes Sep 26, 2016

View reviewed changes

Fishrock123 reviewed Sep 27, 2016

View reviewed changes

mscdex force-pushed the timers-improve-immediate-perf branch from e66340a to 67f1a2a Compare September 29, 2016 08:15

Fishrock123 approved these changes Oct 5, 2016

View reviewed changes

mscdex force-pushed the timers-improve-immediate-perf branch from 67f1a2a to 1bb9048 Compare October 5, 2016 06:43

mscdex force-pushed the timers-improve-immediate-perf branch from 1bb9048 to 0ed8839 Compare October 5, 2016 07:11

mscdex merged commit 0ed8839 into nodejs:master Oct 5, 2016

mscdex deleted the timers-improve-immediate-perf branch October 5, 2016 07:13

MylesBorins added the lts-watch-v4.x label Oct 6, 2016

This was referenced Oct 12, 2016

SchedulingDefault_spec.js fails with Node v6.8.0 (setImmediate problem) TryGhost/Ghost#7555

Closed

🎨 decrease timeout for scheduling TryGhost/Ghost#7556

Merged

Fishrock123 mentioned this pull request Oct 13, 2016

setImmediate regression in v6.8.0 #9084

Closed

nodejs locked and limited conversation to collaborators Oct 13, 2016

nodejs unlocked this conversation Oct 13, 2016

mscdex mentioned this pull request Nov 22, 2016

timers: fix regression with clearImmediate() #9086

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: improve setImmediate() performance #8655

timers: improve setImmediate() performance #8655

mscdex commented Sep 19, 2016 •

edited

Loading

Fishrock123 Sep 19, 2016 •

edited

Loading

mscdex Sep 19, 2016

Fishrock123 Sep 19, 2016

mscdex Sep 19, 2016

mscdex commented Sep 19, 2016

mscdex commented Sep 19, 2016 •

edited

Loading

mscdex commented Sep 19, 2016

richardlau Sep 20, 2016 •

edited

Loading

mscdex Sep 20, 2016

mscdex commented Sep 20, 2016

Fishrock123 Sep 20, 2016

mscdex Sep 20, 2016

jasnell left a comment

imyller left a comment

jasnell commented Sep 27, 2016

Fishrock123 left a comment

Fishrock123 Sep 27, 2016

mscdex Sep 27, 2016

Fishrock123 Sep 27, 2016

Fishrock123 Sep 27, 2016

mscdex Sep 29, 2016

Fishrock123 Sep 27, 2016

mscdex commented Sep 29, 2016

mscdex commented Oct 5, 2016

Fishrock123 left a comment

mscdex commented Oct 5, 2016 •

edited

Loading

MylesBorins commented Oct 6, 2016

ErisDS commented Oct 13, 2016

MylesBorins commented Oct 13, 2016

Trott commented Oct 13, 2016

kirrg001 commented Oct 13, 2016

kirrg001 commented Oct 13, 2016 •

edited

Loading

Fishrock123 commented Oct 13, 2016 •

edited

Loading


		immediateQueue = L.create();
		immediateQueue.head = immediateQueue.tail = null;

timers: improve setImmediate() performance #8655

timers: improve setImmediate() performance #8655

Conversation

mscdex commented Sep 19, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Sep 19, 2016

mscdex commented Sep 19, 2016 • edited Loading

mscdex commented Sep 19, 2016

richardlau Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

imyller left a comment

Choose a reason for hiding this comment

jasnell commented Sep 27, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Sep 29, 2016

mscdex commented Oct 5, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

mscdex commented Oct 5, 2016 • edited Loading

MylesBorins commented Oct 6, 2016

ErisDS commented Oct 13, 2016

MylesBorins commented Oct 13, 2016

Trott commented Oct 13, 2016

kirrg001 commented Oct 13, 2016

kirrg001 commented Oct 13, 2016 • edited Loading

Fishrock123 commented Oct 13, 2016 • edited Loading

mscdex commented Sep 19, 2016 •

edited

Loading

Fishrock123 Sep 19, 2016 •

edited

Loading

mscdex commented Sep 19, 2016 •

edited

Loading

richardlau Sep 20, 2016 •

edited

Loading

mscdex commented Oct 5, 2016 •

edited

Loading

kirrg001 commented Oct 13, 2016 •

edited

Loading

Fishrock123 commented Oct 13, 2016 •

edited

Loading