timers: improve setTimeout/Interval performance by mscdex · Pull Request #8661 · nodejs/node

mscdex · 2016-09-20T06:20:49Z

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 improves timers performance by making functions inlineable and avoiding the creation of extra closures/functions.

This commit also makes setTimeout/Interval argument handling consistent with that of setImmediate.

These changes give ~22% improvement in the existing 'breadth' timers benchmark.

mscdex · 2016-09-20T06:21:28Z

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

fhinkel · 2016-09-20T10:41:39Z

lib/timers.js

I remember we had a long discussion about this, I think for http. Have we ever come to a conclusion whether this is a good idea?

The objects here are not exposed, so it's not an issue in this case.

OK. And it does improve performance? Because - at least for me - it decreases readability.

I didn't measure it specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object?

@fhinkel Just curious. Do you mean using a Map instead of Object.create? #7581 (comment)

For me it increases readability, but please check performance - I recall there being a regression when using .create(null) at one point.

@Florian-R Thanks, that's the discussion I was thinking of. But in this case I meant {} vs .create(null).

@benjamingr I'm not sure what regression you're referring to. These objects are created at startup, not during runtime, so the time to create them doesn't matter as much.

I understand this is not about performance, but just out of curiosity, I measured the performance difference.

console.time("{}"); for (var i = 100000; i--;) { var a = {}; } console.timeEnd("{}"); var create = Object.create; console.time("create"); for (var i = 100000; i--;) { var b = create(null); } console.timeEnd("create");

{}: 5.792ms
create: 15.221ms

Probably, calling a function is heavier than {}

Literals are always faster than instantiation and builder function calls, in JS and most languages.

fhinkel · 2016-09-20T10:49:18Z

lib/timers.js

Thanks. I like this much better than using arguments.

fhinkel · 2016-09-20T10:51:39Z

lib/timers.js

Can we check for typeof args1 == 'undefined' rather than using length? Or is undefined a valid value for args1?

I would say it's a valid value.

fhinkel

Thanks for the speedup. LGTM.

benjamingr

Changes look good to me. Left one comment but it should not block merging.

benjamingr · 2016-09-20T11:47:57Z

lib/timers.js

For me it increases readability, but please check performance - I recall there being a regression when using .create(null) at one point.

Fishrock123 · 2016-09-20T14:34:10Z

lib/timers.js

Unsure why we'd want this in an extra function

I think extracting it into a function increases readability.

The main reason is to keep the insert() size down, but it does also help a bit with readability I suppose.

Fishrock123 · 2016-09-20T14:34:25Z

lib/timers.js

...args ?

Rest args are still slow.

Great move.

Fishrock123 · 2016-09-20T14:37:38Z

lib/timers.js

Why is this a new function?

To keep function sizes down.

IMHO, there's no repetead call, you add a short flow path (and creates a mini and fake cyclomatic complexity), function is in the same script and right under the function (so you just added brackets and more names to read). Function call is slower than direct structured code (and even minimal performance gains, in this call stack queen, is important).

I would not do this separation.

Fishrock123 · 2016-09-20T14:38:35Z

lib/timers.js

unsure if changing this won't have side-effects somewhere

I grepped lib/ and ._repeat isn't used anywhere except timers.js.

I'm more concerned about user code, I do understand that it is unlikely but it has been the way it was an awfully long time in the wild.

Would you rather I leave the variable set to null since it wouldn't be used anymore and add a new property that stores the interval value?

Fishrock123 · 2016-09-20T14:39:20Z

lib/timers.js

should only be timer._handle I think

Unfortunately not, as _http_outgoing and net have a ._handle and manually enroll() pre-made objects. Node will crash since those ._handle instances are not only not Timeout instances, but they also do not have a .start() method.

And the reason this change is necessary now is that the rearming code no longer has access to the scope where the Timeout instance was created.

Fishrock123 · 2016-09-20T14:40:09Z

lib/timers.js

i would just check if (timer._repeat) iirc that is what is done elsewhere.

I don't think it's safe really since ._repeat is now being used to store the interval value, which would not work for the 0 interval.

@mscdex you can't have an interval of 0. It becomes 1.

Fishrock123 · 2016-09-20T14:41:55Z

lib/timers.js

I don't see any tangible benefit to doing this.

We only use Number value keys.

As was previously mentioned, I didn't measure the change specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object? It has nothing to do with key name collisions in this case.

jasnell

LGTM

Fishrock123 · 2016-09-20T20:00:32Z

lib/timers.js

if ontimeout() throws will it deo-opt that function?

Which function? tryOnTimeout()? ontimeout()? The user's callback?

I don't know that throwing causes a deopt or not, but adding a try-* block certainly does cause a deopt for the containing function.

but adding a try-* block certainly does cause a deopt for the containing function.

Right, which is why it was constrained to within one separate function.

I'm curious if a throw fork a user callback de-opts ontimeout() along the way though, since that could be somewhat unpleasant.

Maybe we shouldn't worry about it? Idk the sate of benchmarking this stuff is already so bad 😐

I just wrote a little test and as far as I can tell a function (or any calling functions in the call stack) shouldn't get deopted if that function throws.

Try deopt any container function. Node.js should, IMHO, always test for nullity and use flags/optionals and never try-catch in the inner functions... This pattern is not really a requirement in functional programming and there's huge performance benefits on native calls (basically, in any platform too. try-catch system is expensive).

Fishrock123 · 2016-09-20T20:08:07Z

lib/timers.js

I'm more concerned about user code, I do understand that it is unlikely but it has been the way it was an awfully long time in the wild.

ronkorving · 2016-09-21T05:59:29Z

lib/timers.js

It probably doesn't matter much, but you could use len here instead of arguments.length.

mscdex · 2016-09-21T06:39:34Z

CI again after recent changes: https://ci.nodejs.org/job/node-test-pull-request/4188/

benjamingr · 2016-09-21T09:07:56Z

Still LGTM.

jasnell · 2016-09-21T15:56:55Z

@mscdex ... it would also be good to have a citgm run on this.

mscdex · 2016-09-21T16:14:17Z

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/393/

mscdex · 2016-09-21T19:19:48Z

citgm looks ok, osx and ppcle had a few npm install issues it seems though but that does not appear to be related to this PR.

imyller

LGTM

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

@Fishrock123 Thoughts?

Fishrock123 · 2016-09-30T19:58:44Z

seems fine to me

mscdex · 2016-10-01T05:51:51Z

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

EDIT: CI is green

This commit improves timers performance by making functions inlineable and avoiding the creation of extra closures/functions. This commit also makes setTimeout/Interval argument handling consistent with that of setImmediate. These changes give ~22% improvement in the existing 'breadth' timers benchmark. PR-URL: nodejs#8661 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> 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 improves timers performance by making functions inlineable and avoiding the creation of extra closures/functions. This commit also makes setTimeout/Interval argument handling consistent with that of setImmediate. These changes give ~22% improvement in the existing 'breadth' timers benchmark. PR-URL: #8661 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> 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:48:08Z

adding LTS watch for now, although it will need quite a bit of time to bake before landing.

@mscdex do you see any reason we should not land it in the future?

mscdex · 2016-10-07T00:39:07Z

@thealphanerd No, it should be ok.

This commit improves timers performance by making functions inlineable and avoiding the creation of extra closures/functions. This commit also makes setTimeout/Interval argument handling consistent with that of setImmediate. These changes give ~22% improvement in the existing 'breadth' timers benchmark. PR-URL: #8661 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034

* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) nodejs/node#8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) nodejs/node#8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) nodejs/node#8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) nodejs/node#8100 * npm: Upgraded to 3.10.8 (Kat Marchan) nodejs/node#8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) nodejs/node#8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) nodejs/node#8661 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

leodutra

There's some point to enhance. Object.create is bad at that first lines (literal is faster than Object.create() and I do not see real memory benefits).

There's space for micro optimizations, but the overall improvements are great and makes the commit rich enough.

leodutra · 2016-10-14T14:40:05Z

lib/timers.js

Literals are always faster than instantiation and builder function calls, in JS and most languages.

leodutra · 2016-10-14T14:44:59Z

lib/timers.js

Try deopt any container function. Node.js should, IMHO, always test for nullity and use flags/optionals and never try-catch in the inner functions... This pattern is not really a requirement in functional programming and there's huge performance benefits on native calls (basically, in any platform too. try-catch system is expensive).

leodutra · 2016-10-14T14:46:47Z

lib/timers.js

Great move.

leodutra · 2016-10-14T14:59:37Z

lib/timers.js

IMHO, there's no repetead call, you add a short flow path (and creates a mini and fake cyclomatic complexity), function is in the same script and right under the function (so you just added brackets and more names to read). Function call is slower than direct structured code (and even minimal performance gains, in this call stack queen, is important).

I would not do this separation.

leodutra · 2016-10-14T15:00:48Z

lib/timers.js


 function unrefdHandle() {
-  this.owner._onTimeout();
+  ontimeout(this.owner);


This is always better for internal workings. ALWAYS, Node community.
Avoid this, globals, function calls, function separation in minimal pieces and inner functions... and we'll always have fine code with max JS performance.

Fishrock123 · 2016-10-14T15:25:17Z

@leodutra this was already merged. If you want, you could submit a new PR.

jbergstroem · 2016-10-14T15:26:19Z

I think performance-related improvements to try/catch/finally landed in v8 54.

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 20, 2016

fhinkel reviewed Sep 20, 2016

View reviewed changes

lib/timers.js Outdated

Copy link

Member

fhinkel Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I like this much better than using arguments.

fhinkel reviewed Sep 20, 2016

View reviewed changes

fhinkel approved these changes Sep 20, 2016

View reviewed changes

benjamingr approved these changes Sep 20, 2016

View reviewed changes

Fishrock123 suggested changes Sep 20, 2016

View reviewed changes

jasnell approved these changes Sep 20, 2016

View reviewed changes

Fishrock123 reviewed Sep 20, 2016

View reviewed changes

mscdex force-pushed the timers-improve-settimeout-interval-perf branch from d7a7828 to b8fcf3d Compare September 20, 2016 21:22

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

ronkorving reviewed Sep 21, 2016

View reviewed changes

mscdex force-pushed the timers-improve-settimeout-interval-perf branch from b8fcf3d to c6268a7 Compare September 21, 2016 06:37

imyller approved these changes Sep 26, 2016

View reviewed changes

Fishrock123 approved these changes Sep 30, 2016

View reviewed changes

mscdex force-pushed the timers-improve-settimeout-interval-perf branch from c6268a7 to 92c3623 Compare October 1, 2016 05:51

mscdex force-pushed the timers-improve-settimeout-interval-perf branch from 92c3623 to c8c2544 Compare October 1, 2016 07:01

mscdex merged commit c8c2544 into nodejs:master Oct 1, 2016

mscdex deleted the timers-improve-settimeout-interval-perf branch October 1, 2016 07:03

MylesBorins added dont-land-on-v4.x and removed dont-land-on-v4.x labels Oct 6, 2016

leodutra reviewed Oct 14, 2016

View reviewed changes

nodejs locked and limited conversation to collaborators Oct 14, 2016

Uh oh!

Conversation

mscdex commented Sep 20, 2016

Checklist

Affected core subsystem(s)

Description of change

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!

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!

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!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr 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!

Fishrock123 Sep 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!

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!

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

Fishrock123 Sep 20, 2016 •

edited

Loading

mscdex Sep 20, 2016 •

edited

Loading