node: improve performance of nextTick #985
Conversation
| @@ -340,7 +340,8 @@ | |||
| function _tickCallback() { | |||
| var callback, threw, tock; | |||
|
|
|||
| scheduleMicrotasks(); | |||
| if (microtasksScheduled) | |||
There was a problem hiding this comment.
What's the point? It is checked inside already
There was a problem hiding this comment.
Test for yourself. It removes a third of execution time:
'use strict';
const ITER = 1e6;
let i = ITER;
let t = process.hrtime();
(function runner() {
if (--i > 0) process.nextTick(runner);
else printTime(process.hrtime(t));
}());
function printTime(t) {
console.log(((t[0] * 1e9 + t[1]) / ITER).toFixed(1) + ' ns/op');
}There was a problem hiding this comment.
Oh, I thought it was if (!microtasksScheduled). So it's basically the same as if you remove scheduleMicrotasks
There was a problem hiding this comment.
I wonder why no tests break. This does though
setTimeout(function() {
process.nextTick(function(){});
Promise.resolve().then(function() {
console.log('hi');
});
}, 10);
setTimeout(function(){}, 1000);There was a problem hiding this comment.
Yeah. I thought V8 would have been a little better at inlining the conditional of the function call, but it does seem to make enough difference to require placing the conditional here.
There was a problem hiding this comment.
@vkurchatkin How is it breaking? Like, 'hi' doesn't display?
There was a problem hiding this comment.
ah, wow. totally missed what you were saying about the conditional. let me fix it and push up a fix. one moment.
There was a problem hiding this comment.
Displays after 1000ms timeout. I'm going to add tests for this branch of execution.
trevnorris
force-pushed
the
faster-nexttick
branch
2 times, most recently
from
25a89e5 to
b1a861a
Compare
|
@vkurchatkin I've updated the name of |
|
This is still wrong: if (microtasksScheduled)
scheduleMicrotasks(); |
| }); | ||
|
|
||
| tickInfo[kLength]++; | ||
| process.nextTick(runMicrotasksCallback); |
There was a problem hiding this comment.
Maybe it's better to use nextTick instead of process.nextTick? don't like when we rely on globals being untouched
trevnorris
force-pushed
the
faster-nexttick
branch
2 times, most recently
from
020dd4a to
c26204e
Compare
|
@vkurchatkin thanks. forgot to force push my latest change. also fixed using |
trevnorris
force-pushed
the
faster-nexttick
branch
2 times, most recently
from
2411e99 to
efe37ec
Compare
|
@vkurchatkin Just made a change to move scheduling the microtask queue processor to the end of the tick callback. makes a small functional change in the way it works, but all callbacks will still be executed. mind verifying? |
|
Can't verify it right now but it looks like it breaks intended behavior completely. When we are done with tick queue it's too late to schedule microtaks because: a) they should run on nextTick queue b) we should be preparared to go on with nextTicks scheduled inside microtasks |
|
@vkurchatkin hm. all tests are currently passing. i'll try to write up additional tests to show what you're talking about, but I'll most likely need some help to make sure they're correct. |
|
@trevnorris tests probably don't cover these cases. When there are no nextTick callbacks microtasks are executed directly from |
Couple micro optimizations to improve performance of process.nextTick(). Removes ~60ns of execution time. Also added small threshold to test that allows timer to fire early on the order if microseconds.
trevnorris
force-pushed
the
faster-nexttick
branch
from
efe37ec to
6810d3e
Compare
|
@vkurchatkin I've updated the PR to execute the microtask queue only when the queue is totally depleted, but will also return and continue execution if additional things were added. This is in line with what my prior attempt meant to do. PTAL. |
|
LTGM. this is something I wanted to do in the first place, but decided to choose safer solution |
|
Heh. Then I'll be happy to take the blame in the future if it breaks. :) |
Couple micro optimizations to improve performance of process.nextTick(). Removes ~60ns of execution time. Also added small threshold to test that allows timer to fire early on the order if microseconds. PR-URL: #985 Reviewed-By: Vladimir Kurchatkin <[email protected]>
|
@vkurchatkin Thanks for the review. Landed in e0835c9. |
|
Do we have any benchmark numbers on this? it's always good to communicate via numbers, even if they are very rough, keep that in mind whenever adding perf changes because people love seeing it but when it's just a vague "improve performance" then it's hard to make an assessment about how it impacts you. |
|
@rvagg Here's the script I used to test: 'use strict';
const ITER = 1e6;
let i = ITER;
let t = process.hrtime();
(function runner() {
if (--i > 0) process.nextTick(runner);
else printTime(process.hrtime(t));
}());
function printTime(t) {
console.log(((t[0] * 1e9 + t[1]) / ITER).toFixed(1) + ' ns/op');
}Similar to |
Couple micro optimizations to improve performance of process.nextTick().
Removes ~80ns of execution time.
Also added small threshold to test that allows timer to fire early on
the order if microseconds.
R=@bnoordhuis