lib: move duplicate spliceOne into internal/util #16221

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

lib/url.js

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

lib/events.js

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

lib/events.js

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

var spliceOne; ... if (!spliceOne) spliceOne = require('internal/util').spliceOne;

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

Run a microbenchmark:

> console.time('native'); for (let i = 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 (let i = 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

lib/events.js

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

@refack EventEmitter#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 <[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 · 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 <[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 · 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 <[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 · 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 <[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]>

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 f9d267b Compare 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 a4bd3ac Compare 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 f180a61 Compare 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 380389a Compare 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!

lib: move duplicate spliceOne into internal/util #16221

lib: move duplicate spliceOne into internal/util #16221

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!

apapirovski Oct 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

apapirovski Oct 20, 2017 •

edited

Loading