dns: avoid use of arguments#11359
Conversation
|
Does this have any effect on benchmarks? |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/6417/ |
targos
left a comment
targos
left a comment
There was a problem hiding this comment.
LGTM with a suggestion (untested perf-wise)
| if (asyncCallback.immediately) { | ||
| // The API already returned, we can invoke the callback immediately. | ||
| callback.apply(null, arguments); | ||
| callback.apply(null, args); |
There was a problem hiding this comment.
I'd rewrite to callback(...args)
There was a problem hiding this comment.
Spread arguments still appear to be quite a bit slower according to benchmarks. Even the ...args has mixed results that I haven't quite been able to track down the cause of.
| for (var i = 0; i < arguments.length; ++i) | ||
| args[i + 1] = arguments[i]; | ||
| args.unshift(callback); | ||
| process.nextTick.apply(null, args); |
There was a problem hiding this comment.
And process.nextTick(callback, ...args)
PR-URL: #11359 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]
|
Landed e36d0d3 |
PR-URL: nodejs#11359 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]
PR-URL: #11359 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]
6 participants
Self-explanatory ... avoid unnecessary uses of arguments
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
dns