Improve async.queue start up performance
#1448
Conversation
lib/internal/queue.js
Outdated
| if (!isWaitingForProcessing) { | ||
| isWaitingForProcessing = true; | ||
| setImmediate(q.process); | ||
| } |
There was a problem hiding this comment.
Another way to implement this would be to add a separate scheduleProcessing function that sets up q.process to run once on the next tick. That way the logic isn't spread across _insert and process
There was a problem hiding this comment.
if (!isWaitingForProcessing) {
isWaitingForProcessing = true;
setImmediate(function () {
isWaitingForProcessing = false;
q.process();
});
}There was a problem hiding this comment.
@aearly I update the PR with @ex1st's suggestion (thanks @ex1st). The scheduling is now completely contained within the _insert function, so there shouldn't be a way to permanently pause the queue.
One minor change is that if q.process is updated between calling q.push and the next tick, the new q.process will run as opposed to the old one. Previously, the old q.process would run. The new style follows the q.drain style, so it could technically be considered a bug fix. I'm probably just bikeshedding though, as q.process isn't mentioned anywhere in our docs, and there would probably be a lot of other issues if someone reassigns it. However, if we want to keep the previous behaviour, it's a small change:
if (!isWaitingForProcessing) {
isWaitingForProcessing = true;
var _process = q.process;
setImmediate(function () {
isWaitingForProcessing = false;
_process();
});
}The updated benchmark:
$ ./perf/benchmark.js --grep queue
Latest tag is v2.5.0
Comparing v2.5.0 with current on Node v7.10.0
--------------------------------------
queue(10) v2.5.0 x 37,864 ops/sec ±1.87% (29 runs sampled), 0.0264ms per run
queue(10) current x 41,483 ops/sec ±0.71% (31 runs sampled), 0.0241ms per run
current is faster
--------------------------------------
queue(100) v2.5.0 x 5,647 ops/sec ±1.57% (26 runs sampled), 0.177ms per run
queue(100) current x 6,111 ops/sec ±0.77% (32 runs sampled), 0.164ms per run
current is faster
--------------------------------------
queue(1000) v2.5.0 x 616 ops/sec ±1.71% (32 runs sampled), 1.62ms per run
queue(1000) current x 685 ops/sec ±0.57% (33 runs sampled), 1.46ms per run
current is faster
--------------------------------------
queue(30000) v2.5.0 x 17.56 ops/sec ±1.73% (31 runs sampled), 56.9ms per run
queue(30000) current x 20.09 ops/sec ±2.08% (20 runs sampled), 49.8ms per run
current is faster
--------------------------------------
queue(100000) v2.5.0 x 4.18 ops/sec ±5.79% (9 runs sampled), 240ms per run
queue(100000) current x 5.98 ops/sec ±2.86% (12 runs sampled), 167ms per run
current is faster
--------------------------------------
queue(200000) v2.5.0 x 1.96 ops/sec ±15.76% (4 runs sampled), 511ms per run
queue(200000) current x 2.99 ops/sec ±4.47% (6 runs sampled), 335ms per run
current is faster
--------------------------------------
current faster overall (554ms total vs. 810ms total)
current won more benchmarks (6 vs. 0)
There was a problem hiding this comment.
I wouldn't worry about the q.process change. It's a footgun if a user messes with it.
Essentially, this PR prevents multiple same-tick
q.pushcalls from callingq.processmultiple times.Admittedly, this is slightly biased towards our benchmarks, as most
asyncusers probably aren't callingq.push1000+ times on start-up. However, in the worst-case, if they call it once, or use an array to push multiple tasks at once, it only adds one additional check, and two assignments to thequeue.pushcost.The main drawback of this change is that if
q.processis reassigned, thequeuewill be permanently paused. I don't think that's a supported use case right now though.