timers: optimize same-tick unref #8372

Fishrock123 · 2016-09-01T17:20:56Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

timers

Description of change

The logic behind this revolves around the following:

A timer will never fire before nextTick.
(Thus it is ok to schedule it during nextTick.)
Timer properties are unimportant until scheduled.

See discussion in #6699 for more
background.

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

Fishrock123 · 2016-09-01T17:22:01Z

@misterdjules how do you feel about this?

It should be possible to shim _handle so as to make sure this does not break anything. (but this does not yet do that.)

misterdjules · 2016-09-02T00:26:11Z

Thank you for the heads up @Fishrock123, I appreciate it!

I don't understand what the goal of this PR is. It's likely that I'm missing something obvious, so please consider the questions below as genuine questions to help me understand what this PR is about, and not as rhetorical questions aimed at dismissing it.

In the title, it mentions that it optimizes something, but I don't understand what use case or code path is optimized, and in what way. So my first question is: what problem does this PR solve? If this is about making some code path(s) run faster, are there some benchmarks somewhere that we can take a look at?

It also refers to #6699, but my understanding of #6699 is that it is about being able to differentiate between internal/external timers correctly. How is this PR related to that?

Lastly, the second comment of this PR mentions " shim[ing] _handle so as to make sure this does not break anything.". How does this PR breaks anything related to using the _handle property?

Thank you!

Fishrock123 · 2016-09-02T01:06:42Z

In the title, it mentions that it optimizes something, but I don't understand what use case or code path is optimized, and it what way. So my first question is: what problem does this PR solve? If this is about making some code path(s) run faster, are there some benchmarks somewhere that we can take a look at?

This makes same-tick unref() use the internal unref handle pooling.

e.g.

// optimized
setTimeout(() => {}, 100).unref()

// not optimized
const time r= setTimeout(() => {}, 100)
setImmediate(() => {
  timer.unref()
})

How is this PR related to that?

It is not. It is related to the discussions therein.

ronkorving · 2016-09-02T01:46:46Z

lib/timers.js

+function insertNT() {
+  insertNTScheduled = false;
+  let timer;
+  while (timer = queuedTimers.shift()) {


If this is about performance, wouldn't a linked list make more sense here?

The logic behind this revolves around the following: 1. A timer will never fire before nextTick. - Thus it is ok to schedule it during nextTick. 2. Timer properties are unimportant until scheduled. See discussion in nodejs#6699 for more background.

Fishrock123 · 2016-09-02T15:18:11Z

Updated with @ronkorving's suggestion.

How does this PR breaks anything related to using the _handle property?

This currently does not ensure there is always a _handle property on unref() -- if you hit the optimization it won't be there.
However, exposing the pooled handle is "dangerous" since someone may accidentally close more than they expect. As such, we should "shim" _handle for those. (WIP)

jasnell · 2016-09-02T16:50:47Z

lib/timers.js

  this.owner._onTimeout();
  if (!this.owner._repeat)
    this.owner.close();
 }

+function createOwnHandle() {
+  var now = TimerWrap.now();


extremely minor nit: this could/should be const

misterdjules · 2016-09-02T19:47:06Z

Just to make sure I understand this correctly: is one goal of this PR to make unrefed external timers use TimersList instances to avoid creating a separate TimerWrap instance for each unrefed external timer that has the same delay?

If so, what prevents us from doing that even in the following example you mentioned previously:

const time r= setTimeout(() => {}, 100)
setImmediate(() => {
  timer.unref()
})

?

It seems to me that this PR tries to achieve two things that are orthogonal to each other:

Make external unrefed timers not create a TimerWrap instance for each of them.
Optimize setTimeout by scheduling its heavy lifting to when nextTick callbacks are processed.

Is that correct? If these two points are orthogonal, I would suggest implementing them in two separate PRs.

I would also like to see numbers that illustrate the impact of point 2) on performance.

Fishrock123 · 2016-09-02T20:48:57Z

Just to make sure I understand this correctly: is one goal of this PR to make unrefed external timers use TimersList instances to avoid creating a separate TimerWrap instance for each unrefed external timer that has the same delay?

Effectively.

If so, what prevents us from doing that even in the following example you mentioned previously:

@misterdjules if you know how to do that arbitrarily delayed linked-list insert in less-than-linear (O(n)) time, please let me know. :)

Note: This is the problem I have been trying to work around the entire time.

Edit: even with a sorting algo, you cannot insert or view arbitrary places in the list without traversing it.

I would also like to see numbers that illustrate the impact of point 2) on performance.

I will get numbers if it comes down to the line but the only thing significant added is the nextTick. Grabbing a timer from that new list is O(1) per timer. Negligible.

misterdjules · 2016-09-02T23:22:49Z

If so, what prevents us from doing that even in the following example you mentioned previously:

@misterdjules if you know how to do that arbitrarily delayed linked-list insert in less-than-linear (O(n)) time, please let me know. :)

Note: This is the problem I have been trying to work around the entire time.

Edit: even with a sorting algo, you cannot insert or view arbitrary places in the list without traversing it.

Right, I wasn't aware of the trade-offs you had in mind, but that clarifies things, thanks!

Would unrefing external timers be called often enough in most use cases that the insertion time would be a problem? It was in the case of internal (and by definition unrefed) timers because these timers were restarted and thus re-inserted very frequently (like on every I/O event on a given net.Socket), but it seems that it might not be the case of external unrefed timers.

Coming up with sample code and benchmarks that represents the typical use cases we have in mind could help here.

I'm asking these questions because my first impression is that the inconsistency between external unrefed timers that share a TimersList's TimerWrap instance and those who have their own TimerWrap instance makes things more complex for node's core maintainers and potentially for users too, depending on what the _handle shim you mentioned looks like.

I would also like to see numbers that illustrate the impact of point 2) on performance.

I will get numbers if it comes down to the line but the only thing significant added is the nextTick. Grabbing a timer from that new list is O(1) per timer. Negligible.

I realize now that the use of a nextTick callback to insert timers was not necessarily meant to be an optimization in itself, but was just a way to know whether an external timer should be unrefed at insertion time. Is that correct? If so then what I said about separating these two changes wouldn't make sense, and we wouldn't need separate benchmarks for that.

Fishrock123 · 2016-09-03T00:01:58Z

but was just a way to know whether an external timer should be unrefed at insertion time. Is that correct?

Correct. It's like the Promises & catch() problem. There is no way to know if it follows it directly, or is called at some point in the future, if at all.

Would unrefing external timers be called often enough in most use cases that the insertion time would be a problem? It was in the case of internal (and by definition unrefed) timers because these timers were restarted and thus re-inserted very frequently (like on every I/O event on a given net.Socket), but it seems that it might not be the case of external unrefed timers.

I really have no idea, it would also be very hard to identify in the wild, buried in async layers. It could potentially tank perf pretty hard though in any application using lots of TCP connections.

Edit: hard may still be minor, but it could become significant in any perf profiles.

I originally was thinking of having something like setTimeoutUnref()... but that's adding more APIs and isn't browser-consistent anymore.

depending on what the _handle shim you mentioned looks like.

It would probably look exactly like a TimerWrap, except it wouldn't behave 100% like one under the hood, i.e. closing it would just close that timer and not the entire pool.

misterdjules · 2016-09-06T22:33:34Z

Would unrefing external timers be called often enough in most use cases that the insertion time would be a problem?

I really have no idea, it would also be very hard to identify in the wild, buried in async layers. It could potentially tank perf pretty hard though in any application using lots of TCP connections.

That wasn't clear from my original question: what I wanted to ask is whether unrefing a timer asynchronously is a common use case. If not, then it might be OK to have such unref calls be slower. I'm wondering if there are use cases when calling unref asynchronously allows users to do something that calling it synchronously doesn't allow. Do you have examples in mind?

I wanted to experiment with another approach to making unrefed timers use TimerList instances, but without the inconsistency of having some of them not use TimersList. I tried having Timeout instances store a reference to their TimersList instance, and have Timeout.prototype.{ref,unref} methods increment/decrement a refed counter to determine when their associated TimersList instance's underlying timer handler can be unrefed.

The result is a WIP commit that still needs to deal with the issue you mentioned with the _handle property of each Timeout instance. That is, with that change, someTimer._handle.close() closes the underlying TimerWrap instance, regardless of the state of other Timeout instance that belong to the same timer list. It might be possible to solve that problem, but I wanted to share that experiment with you before moving forward with this in case you'd think it might be an interesting approach.

Fishrock123 · 2016-09-06T23:15:54Z

what I wanted to ask is whether unrefing a timer asynchronously is a common use case.

The indeterminism makes me hope not. cc @brycebaril who is the only person I know to use unref() on timers.

I'm wondering if there are use cases when calling unref asynchronously allows users to do something that calling it synchronously doesn't allow. Do you have examples in mind?

Uh, I guess you can unref some time from a downstream library you may be using? I can't think of anything else really.

The result is a WIP commit that still needs to deal with the issue you mentioned with the _handle property of each Timeout instance.

Huh. That is an interesting but somewhat more complex approach.

Am I correct in saying that the logic behind this revolves around the fact that unrefed handles don't matter if there are still refed handles?

I suppose the big question would be if refing and unrefing a handle potentially a large number of times poses any cost on perf and/or anything else. cc @trevnorris?

brycebaril · 2016-09-07T01:16:06Z

what I wanted to ask is whether unrefing a timer asynchronously is a common use case.
The indeterminism makes me hope not. cc @brycebaril who is the only person I know to use unref() on timers.

Pretty much every time I've used it, the unref call is synchronous, though I can imagine rare cases where you wouldn't immediately know if you needed to call unref or not.

In either case typically the total number of live timers at any given time shouldn't be that large for most use-cases. Any timer-centric use-case beyond that is probably a lot less likely to have unref'd timers.

misterdjules · 2016-09-12T23:54:25Z

@Fishrock123

Am I correct in saying that the logic behind this revolves around the fact that unrefed handles don't matter if there are still refed handles?

That is correct.

jasnell · 2017-03-01T02:06:48Z

@Fishrock123 ... still interested in this?

BridgeAR · 2017-08-26T09:21:51Z

Ping @Fishrock123

jasnell · 2017-08-29T04:12:47Z

@BridgeAR ... given the lack of any activity, I think it's safe just to close this. @Fishrock123 can reopen if it's something he's still interested in pursuing

BridgeAR · 2017-08-29T04:28:30Z

I suppose the big question would be if refing and unrefing a handle potentially a large number of times poses any cost on perf and/or anything else

I am relatively certain that I actually ran into this issue once and that I removed the refing / unrefing again because of the immense performance penalty. Therefore I think it would still be nice to get this in.

That is why I still had hope 😄

jasnell · 2017-08-29T04:40:56Z

Yeah, it's a shame these older PRs weren't followed up on

Fishrock123 · 2017-10-07T21:28:09Z

the discussion here is valuable and should be moved into an issue

Fishrock123 · 2017-10-09T17:43:35Z

Moving to the following issue I've created, which should really have been done instead of just hands-off closing this. How will anyone ever know that they could potentially pick this up otherwise?

#16105

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

Fishrock123 added the wip Issues and PRs that are still a work in progress. label Sep 1, 2016

Fishrock123 changed the title ~~Timers optimize unref~~ timers: optimize same-tick unref Sep 1, 2016

mscdex added the performance Issues and PRs related to the performance of Node.js. label Sep 1, 2016

ronkorving reviewed Sep 2, 2016
View reviewed changes

Fishrock123 added 2 commits September 2, 2016 11:15

timers: optimize same-tick unref

65640c2

The logic behind this revolves around the following: 1. A timer will never fire before nextTick. - Thus it is ok to schedule it during nextTick. 2. Timer properties are unimportant until scheduled. See discussion in nodejs#6699 for more background.

tests: update for timers unref optimization

620412b

Fishrock123 force-pushed the timers-optimize-unref branch from 6f1a877 to 620412b Compare September 2, 2016 15:15

jasnell reviewed Sep 2, 2016
View reviewed changes

Fishrock123 mentioned this pull request Sep 6, 2016

timers: do not expose .unref()._handle._list #8422

Closed

3 tasks

Fishrock123 closed this Sep 6, 2016

Fishrock123 reopened this Sep 6, 2016

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

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

Fishrock123 mentioned this pull request Jun 12, 2017

setTimeout callback order isn't guaranteed #13579

Closed

jasnell closed this Aug 29, 2017

Fishrock123 reopened this Oct 7, 2017

Fishrock123 mentioned this pull request Oct 9, 2017

Optimizing 'unreferenced' timers #16105

Closed

Fishrock123 closed this Oct 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: optimize same-tick unref #8372

timers: optimize same-tick unref #8372

Fishrock123 commented Sep 1, 2016 •

edited

Loading

Fishrock123 commented Sep 1, 2016

misterdjules commented Sep 2, 2016 •

edited

Loading

Fishrock123 commented Sep 2, 2016

ronkorving Sep 2, 2016

Fishrock123 Sep 2, 2016

Fishrock123 commented Sep 2, 2016

jasnell Sep 2, 2016

misterdjules commented Sep 2, 2016

Fishrock123 commented Sep 2, 2016 •

edited

Loading

misterdjules commented Sep 2, 2016

Fishrock123 commented Sep 3, 2016 •

edited

Loading

misterdjules commented Sep 6, 2016

Fishrock123 commented Sep 6, 2016 •

edited

Loading

brycebaril commented Sep 7, 2016

misterdjules commented Sep 12, 2016

jasnell commented Mar 1, 2017

BridgeAR commented Aug 26, 2017

jasnell commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

jasnell commented Aug 29, 2017

Fishrock123 commented Oct 7, 2017

Fishrock123 commented Oct 9, 2017

timers: optimize same-tick unref #8372

timers: optimize same-tick unref #8372

Conversation

Fishrock123 commented Sep 1, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Sep 1, 2016

misterdjules commented Sep 2, 2016 • edited Loading

Fishrock123 commented Sep 2, 2016

ronkorving Sep 2, 2016

Choose a reason for hiding this comment

Fishrock123 Sep 2, 2016

Choose a reason for hiding this comment

Fishrock123 commented Sep 2, 2016

jasnell Sep 2, 2016

Choose a reason for hiding this comment

misterdjules commented Sep 2, 2016

Fishrock123 commented Sep 2, 2016 • edited Loading

misterdjules commented Sep 2, 2016

Fishrock123 commented Sep 3, 2016 • edited Loading

misterdjules commented Sep 6, 2016

Fishrock123 commented Sep 6, 2016 • edited Loading

brycebaril commented Sep 7, 2016

misterdjules commented Sep 12, 2016

jasnell commented Mar 1, 2017

BridgeAR commented Aug 26, 2017

jasnell commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

jasnell commented Aug 29, 2017

Fishrock123 commented Oct 7, 2017

Fishrock123 commented Oct 9, 2017

Fishrock123 commented Sep 1, 2016 •

edited

Loading

misterdjules commented Sep 2, 2016 •

edited

Loading

Fishrock123 commented Sep 2, 2016 •

edited

Loading

Fishrock123 commented Sep 3, 2016 •

edited

Loading

Fishrock123 commented Sep 6, 2016 •

edited

Loading