lib: move duplicate spliceOne into internal/util#16221
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added events 
vsemozhetbyt commented Oct 15, 2017
lib/url.js Outdated
There was a problem hiding this comment.
As this module already uses const, maybe we can use const here too?
vsemozhetbyt commented Oct 15, 2017
Multiple early fails in the CI. Something is temporarily wrong? |
starkwangforce-pushed the move-splice-one branch from 45c4a94 to f9d267bCompare
BridgeAR commented Oct 15, 2017
@bmeurer are there plans to optimize splice sometime "soon"? |
starkwang commented Oct 15, 2017
And |
starkwangforce-pushed the move-splice-one branch from f9d267b to a4bd3acCompare
bmeurer commented Oct 15, 2017
We're working on As for |
lib/events.js Outdated
There was a problem hiding this comment.
It must have a lazy load here because (#15623):
It is because events.js is loaded before all the other modules in the dependency tree (because of bootstrapping process. Lazy loading errors prevents errors from being loaded before events is loaded.
starkwang commented Oct 15, 2017
@vsemozhetbyt Could you please restart a new CI? |
vsemozhetbyt commented Oct 15, 2017
lib/events.js Outdated
There was a problem hiding this comment.
Why not just use list.splice(position, 1) here?
This was introduced 3 years ago in d3f8db1
There was a problem hiding this comment.
According to #14082, it seems like Array#splice is still much slower than spliceOne.
There was a problem hiding this comment.
Than I'd rather have the simpler lazy loader, like #14167
varspliceOne; ... if(!spliceOne)spliceOne=require('internal/util').spliceOne;
refack commented Oct 16, 2017
Run a microbenchmark: >console.time('native');for(leti=0;i<1e8;++i){global.x=[1,2,3,4];global.x.splice(2,1);};console.timeEnd('native') native: 31554.122ms>console.time('spliceOne');for(leti=0;i<1e8;++i){global.x=[1,2,3,4];spliceOne(global.x,2);};console.timeEnd('spliceOne') spliceOne: 3718.840msThat shows at ~10x better performance For |
starkwangforce-pushed the move-splice-one branch from a4bd3ac to f180a61Comparelib/events.js Outdated
There was a problem hiding this comment.
It would be better to write
if(spliceOne===undefined)spliceOne=require('internal/util').spliceOne;Otherwise there is a type conversion while evalutating if spliceOne is truthy.
There was a problem hiding this comment.
Same code in https://github.com/nodejs/node/blob/master/lib/events.js#L74
Should I rewrite them all?
There was a problem hiding this comment.
I would keep it as is to reduce the churn.
There was a problem hiding this comment.
Existing code is tricky but when adding new code and we're certain that something is either defined or undefined then a strict comparison is always better.
(Also, for example, domain can be null or undefined and maybe even other falsey values that we don't test for.)
lpinca commented Oct 20, 2017
@refack |
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code.
starkwangforce-pushed the move-splice-one branch from f180a61 to 380389aComparestarkwang commented Oct 20, 2017
Pushed commit to address comment |
refack commented Oct 20, 2017
refack commented Oct 20, 2017
Landed in 7a71cd7 🍾 |
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: #16221 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
refack commented Oct 20, 2017
P.S. we should write a benchmark that compares the two so we know when V8 catches up. |
MylesBorins commented Oct 23, 2017
This does not land cleanly on the 8.x Would someone be willing to manually backport? |
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: nodejs#16221 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: nodejs#16221 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
refack commented Oct 24, 2017
Backport PR: #16441 |
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: nodejs/node#16221 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: #16221 Backport-PR-URL: #16433 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: #16221 Backport-PR-URL: #16433 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins commented Nov 16, 2017
Should this be backported to |
lib/url.js and lib/events.js are using the same spliceOne function. This change is to move it into the internal/util for avoiding duplicate code. PR-URL: nodejs/node#16221 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
12 participants
lib/url.js and lib/events.js are using the same spliceOne function.This change is to move it into the internal/util for avoiding duplicate code.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url, events, util