lib,tools: fix perm deopts #13384
lib,tools: fix perm deopts #13384
Conversation
nodejs-github-bot
added
events
mscdex
added
events
mscdex
force-pushed
the
lib-fix-perm-deopts
branch
3 times, most recently
from
336d5c2
to
a67671f
Compare
| return this.listener.call(this.target, arguments[0], arguments[1], | ||
| arguments[2]); | ||
| default: | ||
| const args = new Array(arguments.length); |
There was a problem hiding this comment.
Can arrayClone() be reused here or does it have a negative performance impact?
There was a problem hiding this comment.
No, because you cannot pass arguments without penalty except in only one or two cases. apply() is supposed to be one of those cases (which is what was used here before) but for some reason it can still cause a deopt (noticed this in some net benchmarks and tests).
There was a problem hiding this comment.
does (...args) work (for everything) any better?
Ref: #12183
There was a problem hiding this comment.
Rest parameters are still slower than fast path cases like those used above. I think rest params in general are getting faster, but still not quite where they need to be to replace fast paths.
|
Do you think it's prudent to mark these anti-DRY code points for reevaluation after TF&I? Something simple like |
|
IMHO adding such comments is not worth it when we're going to have to look at a large part of core after V8 5.9/6.0 anyway. |
This fixes a regression from 53c88fa so that special arguments can once again be passed to the node executable when running tests. PR-URL: nodejs#13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
mscdex
force-pushed
the
lib-fix-perm-deopts
branch
from
a67671f
to
d081548
Compare
This fixes a regression from 53c88fa so that special arguments can once again be passed to the node executable when running tests. PR-URL: #13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #13384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
|
Is this applicable to v6.x? |
|
ping @mscdex |
MylesBorins
added
the
baking-for-lts
|
Possibly, but I won't have time to backport anytime soon. |
MylesBorins
removed
the
baking-for-lts
CI: https://ci.nodejs.org/job/node-test-pull-request/8429/
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)