-
Notifications
You must be signed in to change notification settings - Fork 29k
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
lib: simplify nextTick() usage #1612
Conversation
This commit removes unnecessary nextTick() closures and adds some shared nextTick() callbacks for better re-use.
args[0] = callback; | ||
for (var i = 1, a = 0; a < arguments.length; ++i, ++a) | ||
args[i] = arguments[a]; | ||
process.nextTick.apply(null, args); | ||
} | ||
}; | ||
} |
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.
DNS callbacks have either two or three arguments so maybe it's cleaner to write this as:
return function asyncCallback(arg0, arg1, arg2) {
if (asyncCallback.immediately) {
if (arguments.length === 3)
callback(arg0, arg1, arg2);
else
callback(arg0, arg1);
} else {
if (arguments.length === 3)
process.nextTick(callback, arg0, arg1, arg2);
else
process.nextTick(callback, arg0, arg1);
}
};
It's still ugly but it avoids allocating an array and going through .apply().
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.
Yeah I was going to do that initially but I wanted see if I could write dns
benchmarks to see what kind of difference it would make. Optimizing this could be a separate PR though I suppose...
Great job on all the cleanup. |
@iojs/collaborators Does this look alright to land or ? |
@@ -61,17 +61,16 @@ function makeAsync(callback) { | |||
// The API already returned, we can invoke the callback immediately. | |||
callback.apply(null, arguments); | |||
} else { | |||
process.nextTick(callMakeAsyncCbNT, callback, arguments); | |||
var args = new Array(arguments.length + 1); | |||
args[0] = callback; |
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 seems a little bit hacky.
The following code is easy to understand for me.
BUT, I have not bench which code is faster.
var args = [callback];
for (var i = 0; i <arguments.length; i++)
args.push(arguments[i]);
process.nextTick.apply(null, args);
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's faster to specify the length up-front since v8 then knows how much to (pre-)allocate.
Here is a jsperf showing the difference for a short array.
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.
Thanks! so this change is reasonable for me.
LGTM.
LGTM. The streams change has me on the edge a little bit too, but the tests pass and I couldn't wrap my head around a test case that would change functionality, so I'm good with it. |
This commit removes unnecessary nextTick() closures and adds some shared nextTick() callbacks for better re-use. PR-URL: #1612 Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Landed in 5abd4ac. |
This commit removes unnecessary nextTick() closures and adds some shared nextTick() callbacks for better re-use. PR-URL: nodejs/node#1612 Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
This commit removes unnecessary
nextTick()
closures and adds some sharednextTick()
callbacks for better re-use.