Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

repl: use String#repeat instead of Array#join #3900

Merged
merged 1 commit into from
Dec 21, 2015
Merged

repl: use String#repeat instead of Array#join #3900

merged 1 commit into from
Dec 21, 2015

Conversation

evanlucas
Copy link
Contributor

String#repeat is quite a bit faster than new Array().join().

@evanlucas evanlucas added the repl Issues and PRs related to the REPL subsystem. label Nov 18, 2015
@Fishrock123
Copy link
Contributor

LGTM, evidence of said perf would be nice of course.

@evanlucas
Copy link
Contributor Author

In the common case here:

This is testing:

'  '.repeat(1)

// vs

new Array(2).join('  ')

String#repeat 14.5 ns/op
Array#join 255.3 ns/op

@cjihrig
Copy link
Contributor

cjihrig commented Nov 18, 2015

LGTM. The speedup is nice. Not sure how important it is in the REPL.

@evanlucas
Copy link
Contributor Author

The advantage is the speedup in showing the prompt IMO

@cjihrig
Copy link
Contributor

cjihrig commented Nov 18, 2015

I don't think anyone will perceive < 1us :-)

It's still nice to know for more performance critical places.

@evanlucas
Copy link
Contributor Author

yea, true

@ronkorving
Copy link
Contributor

It seems that the only place where this pattern emerged is in repl.js, unless my grep is not revealing enough.

$ grep -r 'new Array(' .
./dns.js:      var args = new Array(arguments.length + 1);
./domain.js:    var args = new Array(len - 1);
./events.js:      args = new Array(len - 1);
./events.js:  var copy = new Array(i);
./fs.js:  const chunks = new Array(len);
./internal/child_process.js:  // Don't concat() a new Array() because it would be sparse, and
./net.js:    var chunks = new Array(data.length << 1);
./querystring.js:var hexTable = new Array(256);
./repl.js:    var levelInd = new Array(this.lines.level.length).join('..');
./repl.js:    self.lines.push(new Array(self.lines.level.length).join('  ') + cmd);
./timers.js:      var args = new Array(length - 2);
./timers.js:      var args = new Array(length - 2);
./timers.js:      args = new Array(len - 1);
./util.js:  var output = new Array(value.length);

silverwind
silverwind reviewed Dec 21, 2015
View reviewed changes
lib/repl.js
var levelInd = new Array(this.lines.level.length).join('..');
const len = this.lines.level.length
? this.lines.level.length - 1
: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could fit on a single line.

@silverwind
Copy link
Contributor

LGTM with style nit. By the way: https://jsperf.com/string-repeat-native-vs-array-join-vs-loop

@evanlucas
Copy link
Contributor Author

Fixed the nit

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

silverwind
silverwind reviewed Dec 21, 2015
View reviewed changes
lib/repl.js
self.lines.push(new Array(self.lines.level.length).join(' ') + cmd);
const len = self.lines.level.length
? self.lines.level.length - 1
: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please this one too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh missed that one. Fixed

@silverwind
Copy link
Contributor

CI is happy, let's land it!

@evanlucas
Copy link
Contributor Author

k, landing now

@evanlucas
repl: use String#repeat instead of Array#join
50125e2 
String#repeat is quite a bit faster than new Array().join().

PR-URL: nodejs#3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@evanlucas evanlucas closed this Dec 21, 2015
@evanlucas evanlucas deleted the stringrepeat branch December 21, 2015 17:00
@evanlucas evanlucas merged commit 50125e2 into nodejs:master Dec 21, 2015
@evanlucas
Copy link
Contributor Author

Landed in 50125e2. Thanks!

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
@evanlucas @Fishrock123
repl: use String#repeat instead of Array#join
09c9110 
String#repeat is quite a bit faster than new Array().join().

PR-URL: nodejs#3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@evanlucas @Fishrock123
repl: use String#repeat instead of Array#join
d11930d 
String#repeat is quite a bit faster than new Array().join().

PR-URL: nodejs#3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
@evanlucas
repl: use String#repeat instead of Array#join
dbf6537 
String#repeat is quite a bit faster than new Array().join().

PR-URL: #3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@evanlucas
repl: use String#repeat instead of Array#join
9855fab 
String#repeat is quite a bit faster than new Array().join().

PR-URL: #3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas
repl: use String#repeat instead of Array#join
d260d0c 
String#repeat is quite a bit faster than new Array().join().

PR-URL: nodejs#3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@evanlucas @Fishrock123 @cjihrig @ronkorving @silverwind @jasnell @MylesBorins