lib: move duplicate spliceOne into internal/util by starkwang · Pull Request #16221 · nodejs/node

starkwang · 2017-10-15T16:05:48Z

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), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

url, events, util

vsemozhetbyt · 2017-10-15T16:15:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10728/

vsemozhetbyt · 2017-10-15T16:19:12Z

As this module already uses const, maybe we can use const here too?

vsemozhetbyt · 2017-10-15T16:23:36Z

Multiple early fails in the CI. Something is temporarily wrong?

BridgeAR · 2017-10-15T16:41:13Z

@bmeurer are there plans to optimize splice sometime "soon"?

starkwang · 2017-10-15T16:45:59Z

@bmeurer are there plans to optimize splice sometime "soon"?

And Array.join: https://github.com/nodejs/node/blob/master/lib/internal/util.js#L261

bmeurer · 2017-10-15T17:13:12Z

We're working on Array#slice, but mostly on the clone case, which should also be usable for Array#splice. Which case is most interesting for Array#splice?

As for Array#join (and Array#toString): One day... hopefully not too far away...

starkwang · 2017-10-15T17:14:10Z

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 · 2017-10-15T17:17:30Z

@vsemozhetbyt Could you please restart a new CI?

vsemozhetbyt · 2017-10-15T17:20:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10729/

refack · 2017-10-15T22:54:43Z

Why not just use list.splice(position, 1) here?
This was introduced 3 years ago in d3f8db1

According to #14082, it seems like Array#splice is still much slower than spliceOne.

Than I'd rather have the simpler lazy loader, like #14167
varspliceOne; ... if(!spliceOne)spliceOne=require('internal/util').spliceOne;

refack · 2017-10-16T21:56:08Z

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.840ms

That shows at ~10x better performance

For url I get it, but IMHO removeListener is not a hot path and is not worth the lazy load mechanics.

BridgeAR · 2017-10-18T14:52:31Z

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.

Same code in https://github.com/nodejs/node/blob/master/lib/events.js#L74
Should I rewrite them all?

I would keep it as is to reduce the churn.

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 · 2017-10-20T14:18:26Z

@refackEventEmitter#removeListener() is used extensively in _http_server.js so I'd say it's equally important.

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.

starkwang · 2017-10-20T14:37:02Z

Pushed commit to address comment

refack · 2017-10-20T19:26:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10876/

refack · 2017-10-20T19:52:24Z

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 <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-10-20T19:53:11Z

P.S. we should write a benchmark that compares the two so we know when V8 catches up.

MylesBorins · 2017-10-23T22:19:10Z

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 <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-10-24T13:25:58Z

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 <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins · 2017-11-16T21:35:06Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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 <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. url Issues and PRs related to the legacy built-in url module. util Issues and PRs related to the built-in util module. labels Oct 15, 2017

vsemozhetbyt reviewed Oct 15, 2017
View reviewed changes

starkwang force-pushed the move-splice-one branch from 45c4a94 to f9d267bCompare October 15, 2017 16:35

TimothyGu approved these changes Oct 15, 2017
View reviewed changes

BridgeAR approved these changes Oct 15, 2017
View reviewed changes

vsemozhetbyt approved these changes Oct 15, 2017
View reviewed changes

starkwang force-pushed the move-splice-one branch from f9d267b to a4bd3acCompare October 15, 2017 17:12

starkwang commented Oct 15, 2017
View reviewed changes

addaleax mentioned this pull request Oct 15, 2017
mass early fails in CI #16223
Closed

jasnell approved these changes Oct 15, 2017
View reviewed changes

refack reviewed Oct 15, 2017
View reviewed changes

starkwang force-pushed the move-splice-one branch from a4bd3ac to f180a61Compare October 17, 2017 14:32

BridgeAR approved these changes Oct 18, 2017
View reviewed changes

lpinca approved these changes Oct 20, 2017
View reviewed changes

lib: move duplicate spliceOne into internal/util
380389a
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.

starkwang force-pushed the move-splice-one branch from f180a61 to 380389aCompare October 20, 2017 14:36

apapirovski approved these changes Oct 20, 2017
View reviewed changes

refack approved these changes Oct 20, 2017
View reviewed changes

refack self-assigned this Oct 20, 2017

refack closed this Oct 20, 2017

MylesBorins added the backport-requested-v8.x label Oct 23, 2017

lpinca mentioned this pull request Oct 24, 2017
[v8.x backport] lib: move duplicate spliceOne into internal/util #16433
Closed

refack mentioned this pull request Oct 24, 2017
[v8.x backport] lib: move duplicate spliceOne into internal/util #16441
Closed
2 tasks

jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged

gibfahn mentioned this pull request Oct 31, 2017
v8.9.0 proposal #16630
Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jun 19, 2018

refack removed their assignment Oct 12, 2018

Uh oh!

Uh oh!

Conversation

starkwang commented Oct 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Oct 15, 2017

Uh oh!

BridgeAR commented Oct 15, 2017

Uh oh!

starkwang commented Oct 15, 2017

Uh oh!

bmeurer commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkwang commented Oct 15, 2017

Uh oh!

vsemozhetbyt commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovskiOct 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 20, 2017

Uh oh!

starkwang commented Oct 20, 2017

Uh oh!

refack commented Oct 20, 2017

Uh oh!

refack commented Oct 20, 2017

Uh oh!

refack commented Oct 20, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

refack commented Oct 24, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

starkwang commented Oct 15, 2017•
edited
Loading

apapirovskiOct 20, 2017•
edited
Loading