timers: improve setImmediate() performance by mscdex · Pull Request #8655 · nodejs/node

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

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

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

@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

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

Can this be a helper fn?

What do you mean?

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

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 a284f41Compare September 19, 2016 17:59

mscdex force-pushed the timers-improve-immediate-perf branch from a284f41 to dcc9fe0Compare 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 e66340aCompare 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 c5ce7f4Compare 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 67f1a2aCompare 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 1bb9048Compare October 5, 2016 06:43

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

mscdex merged commit 0ed8839 into nodejs:masterOct 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

Uh oh!

Uh oh!

Conversation

mscdex commented Sep 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123Sep 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 19, 2016

Uh oh!

mscdex commented Sep 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Sep 19, 2016

Uh oh!

richardlauSep 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Sep 27, 2016

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 29, 2016

Uh oh!

mscdex commented Oct 5, 2016

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 5, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

ErisDS commented Oct 13, 2016

Uh oh!

MylesBorins commented Oct 13, 2016

Uh oh!

Trott commented Oct 13, 2016

Uh oh!

kirrg001 commented Oct 13, 2016

mscdex commented Sep 19, 2016•
edited
Loading

Fishrock123Sep 19, 2016•
edited
Loading

mscdex commented Sep 19, 2016•
edited
Loading

richardlauSep 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