timers: improve setTimeout/Interval performance #8661

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

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

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

benjamingr

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

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

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

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

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

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

imyller

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

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

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

MylesBorins · 2016-10-06T23:48:08Z

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

leodutra

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

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

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!

timers: improve setTimeout/Interval performance #8661

timers: improve setTimeout/Interval performance #8661

Conversation

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

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

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!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment