-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
timers: assorted cleanup + improvements #18486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some smaller things. I noticed that the commit descriptions are less informative than what's in your PR here, think you could update them?
Speaking of that, it's pretty nuts that removing Timer.now()
has that kind of perf impact, however your PR says 40% but you commit says 80%.. which is it? :P
Oh and, could you explain 5. a bit more? I think I have a hint of what's going on there but I suspect it'd be pretty difficult to understand in any detail.
message: 'Timeout Error' | ||
})); | ||
|
||
let called = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.mustCall()
doesn't work here? It checks for one and only one call by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can potentially cause an infinite loop since it only checks at exit
and this tests the fact that calling ref
on unref'd
handle doesn't cause the C++ loop to continue forever.
This is related to the owner
check in C++. Someone could, in the future, think to optimize that by checking whether the handle is ref
or unref
, instead of checking for presence of an owner
, but that would actually be incorrect and would cause an infinite loop on this test.
@@ -9,9 +9,16 @@ const t = setInterval(() => { | |||
cntr++; | |||
if (cntr === 1) { | |||
common.busyLoop(100); | |||
// ensure that the event loop passes before the second interval | |||
setImmediate(() => assert.strictEqual(cntr, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these new callbacks also have common.mustCall()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted not to include since setImmediate
is guaranteed to run and if it doesn't, others tests will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm yes but I'm not sure that's guaranteed to be true for all APIs so we do need to be careful about that path of doing things,
Yep, no worries. Will update later today.
Not sure. I got 80% on my local machine but I'm getting 40% on benchmark CI but also 10% on a couple of other tests which I didn't see locally. From doing other benchmark runs in the past, I suspect that some of our performance gains can be platform specific (for example, frequency of GC would be one consideration). In terms of why the big jump, the function itself does very little so constantly re-entering C++ accounts for a huge part of it.
There was a PR recently (e647c5d#diff-0a5d4868b2b9b17cf9e2c11f1bd1311e) that fixed the case where an interval executing longer than its timeout value would block the event loop. It solved that issue for most cases except where the trip from |
Imo put down something like
Ah yes, thanks. |
Instead of using nextTick to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days...
Pass in Timer.now() as an argument of kOnTimeout instead of always re-entering C++ to get it. Also don't constantly call Timer.now() from ontimeout, even when it isn't needed. Improves performance on our pooled benchmark by upwards of 40%.
Instead of calling stop + start from the refresh method of Timeout, instead just call start as libuv already calls stop if necessary.
When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). Refs: nodejs#15072
4746614
to
4dacb85
Compare
I've updated the commit messages with a bit more info. |
Instead of using nextTick to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days... PR-URL: #18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Pass in Timer.now() as an argument of kOnTimeout instead of always re-entering C++ to get it. Also don't constantly call Timer.now() from ontimeout, even when it isn't needed. Improves performance on our pooled benchmark by upwards of 40%. PR-URL: #18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: #18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Instead of calling stop + start from the refresh method of Timeout, instead just call start as libuv already calls stop if necessary. PR-URL: #18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). PR-URL: #18486 Refs: #15072 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
When it's going to be included in Node release? |
Instead of using nextTick to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days... PR-URL: nodejs#18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Pass in Timer.now() as an argument of kOnTimeout instead of always re-entering C++ to get it. Also don't constantly call Timer.now() from ontimeout, even when it isn't needed. Improves performance on our pooled benchmark by upwards of 40%. PR-URL: nodejs#18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: nodejs#18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Instead of calling stop + start from the refresh method of Timeout, instead just call start as libuv already calls stop if necessary. PR-URL: nodejs#18486 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). PR-URL: nodejs#18486 Refs: nodejs#15072 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
There a few changes in this PR, all to timers, and it's best to review each commit independently. It will make way more sense that way.
Instead of using
nextTick
to process failed lists, just attempt to process them again from C++ if the process is still alive. This also allows the removal of domain specific code in timers. (The current behaviour is not quite ideal as it means that all lists after the failed one will process on an arbitrary nextTick, even if they're — say — not due to fire for another 2 days...)Pass in
Timer.now()
as an argument of kOnTimeout instead of always re-entering C++ to get it. Also don't constantly callTimer.now()
fromontimeout
, even when it isn't needed. Improves performance on our pooled benchmark by roughly 40%.Switch to use
const
where appropriate.Instead of calling stop + start from the refresh method of Timeout, instead just call start as libuv already calls stop if necessary.
When an interval takes exactly as long to run as its timeout setting, it can sometimes block the event loop if exactly 1ms elapses before
start()
is called. This commit fixes that.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
timers