-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
3x faster setImmediate #6436
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
3x faster setImmediate #6436
Changes from 6 commits
44243ad
d005b81
ad3bcb8
1f82a29
49b2611
c12ba47
6dcbd83
197aac2
4b54cd7
8179d32
77e0443
0e75f1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| 'use strict'; | ||
| var common = require('../common.js'); | ||
|
|
||
| var bench = common.createBenchmark(main, { | ||
| thousands: [2000], | ||
| type: ['depth', 'depth1', 'breadth', 'breadth1', 'breadth4', 'clear'] | ||
| }); | ||
|
|
||
| function main(conf) { | ||
| var N = +conf.thousands * 1e3; | ||
| switch (conf.type) { | ||
| case 'depth': | ||
| depth(N); | ||
| break; | ||
| case 'depth1': | ||
| depth1(N); | ||
| break; | ||
| case 'breadth': | ||
| breadth(N); | ||
| break; | ||
| case 'breadth1': | ||
| breadth1(N); | ||
| break; | ||
| case 'breadth4': | ||
| breadth4(N); | ||
| break; | ||
| case 'clear': | ||
| clear(N); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // setImmediate tail recursion, 0 arguments | ||
| function depth(N) { | ||
| var n = 0; | ||
| bench.start(); | ||
| setImmediate(cb); | ||
| function cb() { | ||
| n++; | ||
| if (n === N) | ||
| bench.end(N / 1e3); | ||
| else | ||
| setImmediate(cb); | ||
| } | ||
| } | ||
|
|
||
| // setImmediate tail recursion, 1 argument | ||
| function depth1(N) { | ||
| var n = 0; | ||
| bench.start(); | ||
| setImmediate(cb, 1); | ||
| function cb(a1) { | ||
| n++; | ||
| if (n === N) | ||
| bench.end(N / 1e3); | ||
| else | ||
| setImmediate(cb, 1); | ||
| } | ||
| } | ||
|
|
||
| // concurrent setImmediate, 0 arguments | ||
| function breadth(N) { | ||
| var n = 0; | ||
| bench.start(); | ||
| function cb() { | ||
| n++; | ||
| if (n === N) | ||
| bench.end(N / 1e3); | ||
| } | ||
| for (var i = 0; i < N; i++) { | ||
| setImmediate(cb); | ||
| } | ||
| } | ||
|
|
||
| // concurrent setImmediate, 1 argument | ||
| function breadth1(N) { | ||
| var n = 0; | ||
| bench.start(); | ||
| function cb(a1) { | ||
| n++; | ||
| if (n === N) | ||
| bench.end(N / 1e3); | ||
| } | ||
| for (var i = 0; i < N; i++) { | ||
| setImmediate(cb, 1); | ||
| } | ||
| } | ||
|
|
||
| // concurrent setImmediate, 4 arguments | ||
| function breadth4(N) { | ||
| var n = 0; | ||
| bench.start(); | ||
| function cb(a1, a2, a3, a4) { | ||
| n++; | ||
| if (n === N) | ||
| bench.end(N / 1e3); | ||
| } | ||
| for (var i = 0; i < N; i++) { | ||
| setImmediate(cb, 1, 2, 3, 4); | ||
| } | ||
| } | ||
|
|
||
| function clear(N) { | ||
| bench.start(); | ||
| function cb(a1) { | ||
| if (a1 === 2) | ||
| bench.end(N / 1e3); | ||
| } | ||
| for (var i = 0; i < N; i++) { | ||
| clearImmediate(setImmediate(cb, 1)); | ||
| } | ||
| setImmediate(cb, 2); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -502,24 +502,26 @@ Timeout.prototype.close = function() { | |
| }; | ||
|
|
||
|
|
||
| var immediateQueue = {}; | ||
| L.init(immediateQueue); | ||
| var immediateQueue = L.create(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really understand why this change is needed or is helpful but I guess it does clean things up a little.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prevents a hidden class change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. at top-level it was changed to match processImmediate (and it cleans things up a bit).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I bet the object ends up being access-optimized after enough runs anyway. Not to mention we can make objects access-optimized explicitly. That said - this change improves style anyway and is better coding.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably, though depends on how many immediates are queued in an event loop cycle. Out of curiosity, what are the ways of creating access-optimized objects? I know about
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andrasq conveniently, here is a StackOverflow answer I wrote about a technique petkaantonov used in bluebird (with making an object as a prototype of a function). This also works with Object.create so I guess that's another one.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be explicit - this specific change LGTM, even if it's not faster but it probably is given the old code.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a great writeup, and a handy reference, thanks! |
||
|
|
||
|
|
||
| function processImmediate() { | ||
| var queue = immediateQueue; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we're touching this code,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
| var domain, immediate; | ||
|
|
||
| immediateQueue = {}; | ||
| L.init(immediateQueue); | ||
| immediateQueue = L.create(); | ||
|
|
||
| while (L.isEmpty(queue) === false) { | ||
| immediate = L.shift(queue); | ||
| domain = immediate.domain; | ||
|
|
||
| if (!immediate._onImmediate) | ||
| continue; | ||
|
|
||
| if (domain) | ||
| domain.enter(); | ||
|
|
||
| immediate._callback = immediate._onImmediate; | ||
| tryOnImmediate(immediate, queue); | ||
|
|
||
| if (domain) | ||
|
|
@@ -540,7 +542,8 @@ function processImmediate() { | |
| function tryOnImmediate(immediate, queue) { | ||
| var threw = true; | ||
| try { | ||
| immediate._onImmediate(); | ||
| // make the actual call outside the try/catch to allow it to be optimized | ||
| runCallback(immediate); | ||
| threw = false; | ||
| } finally { | ||
| if (threw && !L.isEmpty(queue)) { | ||
|
|
@@ -554,67 +557,73 @@ function tryOnImmediate(immediate, queue) { | |
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a breaking change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, nvm, #6206 has not landed yet
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this would be breaking if #6206 had landed, as it doesn't get rid of the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That code changed since the comment was left here. |
||
|
|
||
| function runCallback(timer) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure we don't do this optimization anywhere else in the code more generically? It sounds like something that would go in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I use this construct in other projects, and found it hard to make generic; That being said, I agree, an optimized call invoker is a nice utility to have. |
||
| var argv = timer._argv; | ||
| var argc = argv ? argv.length : 0; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| switch (argc) { | ||
| // fast-path callbacks with 0-3 arguments | ||
| case 0: | ||
| return timer._callback(); | ||
| case 1: | ||
| return timer._callback(argv[0]); | ||
| case 2: | ||
| return timer._callback(argv[0], argv[1]); | ||
| case 3: | ||
| return timer._callback(argv[0], argv[1], argv[2]); | ||
| // more then 3 arguments run slower with .apply | ||
| default: | ||
| return timer._callback.apply(timer, argv); | ||
| } | ||
| } | ||
|
|
||
| function Immediate() { } | ||
|
|
||
| Immediate.prototype.domain = undefined; | ||
| Immediate.prototype._onImmediate = undefined; | ||
| Immediate.prototype._idleNext = undefined; | ||
| Immediate.prototype._idlePrev = undefined; | ||
|
|
||
| function createImmediate(callback, args) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we need the stylistic change of returning an Object rather than using a constructor. I think it would be more consistent to still have a That said, setting properties locally rather than on the prototype definitely helps with hidden classes and is preferable.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that is possible then we should probably go that way imo.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried various constructs, used the fastest, and used style as a tie-breaker. I timed the my experimental Immediate was (a separate test predeclared all properties as null)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't buy it that Putting object properties on the prototype is a perf killer. Not suggesting we stick to that. Definitely going to see an improvement compared to prototype properties anyway.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's weird, I just ran a set of micro-benchmarks and they're exactly like you say, I ran the same benchmark side-by-side with --trace-gc, and the The timers/immediate.js breadth benchmark shows a larger 55% difference in favor of { ... }
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Care to share the actual code you benched in both cases so I could have a look?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I double checked and it's identical in terms of performance. I think we should go with
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How did you test? I'm still seeing a very consisten difference with the sources from createImmediate version: (run with This is much more extreme a difference than in the other tests, but it's consistent. The "other test" is the one-liner from a larger suite I packaged up my timeit / runit utility as createImmediate:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. When I patch the original v6.0.0 branch with the setImmediate function and Edit: clarifiction: the 524ms is the new setImmediate without a prototype, I didn't notice that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found the reason, and it makes no sense, but the good news is that the workaround is If Measuring small runs of tasks with a tool that subtracts the timing loop overhead, Either way, the immediates processing rate is over 250% faster than before this change |
||
| return { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here RE: creating a new instance with a no prototype inheritance
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no difference; see reply above |
||
| _idleNext: null, | ||
| _idlePrev: null, | ||
| _callback: callback, | ||
| _argv: args, | ||
| _onImmediate: callback, | ||
| domain: process.domain, | ||
| }; | ||
| } | ||
|
|
||
| exports.setImmediate = function(callback, arg1, arg2, arg3) { | ||
| if (typeof callback !== 'function') { | ||
| throw new TypeError('"callback" argument must be a function'); | ||
| } | ||
|
|
||
| var i, args; | ||
| var len = arguments.length; | ||
| var immediate = new Immediate(); | ||
|
|
||
| L.init(immediate); | ||
|
|
||
| switch (len) { | ||
| switch (arguments.length) { | ||
| // fast cases | ||
| case 0: | ||
| case 1: | ||
| immediate._onImmediate = callback; | ||
| break; | ||
| case 2: | ||
| immediate._onImmediate = function() { | ||
| callback.call(immediate, arg1); | ||
| }; | ||
| args = [arg1]; | ||
| break; | ||
| case 3: | ||
| immediate._onImmediate = function() { | ||
| callback.call(immediate, arg1, arg2); | ||
| }; | ||
| args = [arg1, arg2]; | ||
| break; | ||
| case 4: | ||
| immediate._onImmediate = function() { | ||
| callback.call(immediate, arg1, arg2, arg3); | ||
| }; | ||
| args = [arg1, arg2, arg3]; | ||
| break; | ||
| // slow case | ||
| default: | ||
| var len = arguments.length; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the 1/2/3/more argument optimization really help with anything here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a good deal faster than iterating over arguments iirc
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, Normally either would be blazing fast, 75 million vs 48 million / second, but when tuning
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but we're not actually invoking anything here, is it still slow to pass
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so that's a great question, I checked, and the answer turns to be weird -- the "/more" case
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @benjamingr I changed the three lines that copy the callback params out of Turns out node v6 is much much faster to From the commit message: |
||
| args = new Array(len - 1); | ||
| for (i = 1; i < len; i++) | ||
| args[i - 1] = arguments[i]; | ||
|
|
||
| immediate._onImmediate = function() { | ||
| callback.apply(immediate, args); | ||
| }; | ||
| break; | ||
| } | ||
| var immediate = createImmediate(callback, args); | ||
|
|
||
| if (!process._needImmediateCallback) { | ||
| process._needImmediateCallback = true; | ||
| process._immediateCallback = processImmediate; | ||
| } | ||
|
|
||
| if (process.domain) | ||
| immediate.domain = process.domain; | ||
|
|
||
| L.append(immediateQueue, immediate); | ||
|
|
||
| return immediate; | ||
|
|
@@ -626,9 +635,6 @@ exports.clearImmediate = function(immediate) { | |
|
|
||
| immediate._onImmediate = undefined; | ||
|
|
||
| L.remove(immediate); | ||
|
|
||
| if (L.isEmpty(immediateQueue)) { | ||
| process._needImmediateCallback = false; | ||
| } | ||
| // leave queued, much faster overall to skip it later in processImmediate | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this is true,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the result is very reproducible, it's 3x faster to set/clear without the remove.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's ever possible to even have this show 1% in a v8 profile of an actual application, could you compile some benchmark results for us on these?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect actual applications make very few calls to clearImmediate The output of test from benchmark/timers/immediate.js This test creates 2m cleared immediates and shows 254% speedup. |
||
| return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, indeed, left over from my first attempt to bypass the unlink. Fixing. |
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,3 +103,8 @@ assert.equal(C, L.shift(list)); | |
| // list | ||
| assert.ok(L.isEmpty(list)); | ||
|
|
||
| const list2 = L.create(); | ||
| const list3 = L.create(); | ||
| assert.ok(L.isEmpty(list2)); | ||
| assert.ok(L.isEmpty(list3)); | ||
| assert.ok(list2 != list3); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe also add: L.init(list);
assert.deepEqual(list2, list);(I think that should pass)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, we'd block-scope the tests and the variables. For a later PR I suppose.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the assertion failed... which makes sense in hindsight, circular lists point to themselves, |
||
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.
Would the common case of no trailing list element make it better to check these in the opposite order?
Uh oh!
There was an error while loading. Please reload this page.
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.
linkedlist.js implements a circular list, the expected case is that both will be set or not set together.
This is just a fast-path optimization for newly created list nodes that have never been linked.
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.
Ah, right. Forgot about that.