benchmark: replace [].join() with ''.repeat() #12170

vsemozhetbyt · 2017-04-02T21:46:48Z

Checklist

tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

benchmark

This change is mostly for readability as the edited fragments are not performance-wise concerned. However, a benchmark to compare both ways of long string creation is added to be on the safe side.

Results of the added benchmark with the last node.8.0.0-nightly20170402

es\string-repeat.js size=10 encoding="ascii" mode="Array" n=1000: 290,692.60419876396
es\string-repeat.js size=1000 encoding="ascii" mode="Array" n=1000: 102,836.05382685862
es\string-repeat.js size=1000000 encoding="ascii" mode="Array" n=1000: 135.32093280569134

es\string-repeat.js size=10 encoding="utf8" mode="Array" n=1000: 288,349.3871854649
es\string-repeat.js size=1000 encoding="utf8" mode="Array" n=1000: 94,707.65121331392
es\string-repeat.js size=1000000 encoding="utf8" mode="Array" n=1000: 115.63063143181027

es\string-repeat.js size=10 encoding="ascii" mode="repeat" n=1000: 1,551,397.8094262932
es\string-repeat.js size=1000 encoding="ascii" mode="repeat" n=1000: 695,483.8063550529
es\string-repeat.js size=1000000 encoding="ascii" mode="repeat" n=1000: 467,033.2877975878

es\string-repeat.js size=10 encoding="utf8" mode="repeat" n=1000: 1,659,899.8084475622
es\string-repeat.js size=1000 encoding="utf8" mode="repeat" n=1000: 661,734.5385725063
es\string-repeat.js size=1000000 encoding="utf8" mode="repeat" n=1000: 455,820.924368369

vsemozhetbyt · 2017-04-02T22:18:14Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/7978/

lpinca · 2017-04-03T13:03:20Z

benchmark/es/string-repeat.js

+    bench.end(n);
+  }
+
+  assert.strictEqual([...str].length, size);


Why [...str].length instead of just str.length?

It is unicode-proof, it counts code points, not char codes:

> '\ud83d\udc0e'.repeat(10).length 20 > [...'\ud83d\udc0e'.repeat(10)].length 10

lpinca · 2017-04-03T13:25:32Z

benchmark/es/string-repeat.js

+
+  if (conf.mode === 'Array') {
+    bench.start();
+    for (let i = 0; i < n; i++) str = new Array(size + 1).join(character);


I like this style but I think it's not widely used in the codebase.
Is it better to put the for body in the next line?

Also is it ok to use let in for loops like this now?

I will wrap to conform.

It seems let in loops is mostly optimized now. At least they are rather common in tests or benchmarks.

Also add a benchmark to compare both ways to create strings.

vsemozhetbyt · 2017-04-03T13:44:37Z

New linter CI: https://ci.nodejs.org/job/node-test-linter/7990/

Also add a benchmark to compare both ways to create strings. PR-URL: #12170 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-04-04T16:21:58Z

Landed in 74dc3bf

Also add a benchmark to compare both ways to create strings. PR-URL: nodejs#12170 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Like PR nodejs#12170 .

MylesBorins · 2017-04-18T23:28:47Z

lts?

Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>

gibfahn · 2017-05-15T20:49:55Z

ping @vsemozhetbyt , should this be backported to v6.x?

vsemozhetbyt · 2017-05-15T20:54:44Z

@gibfahn I am not sure. If we have some +1 I can try to backport though if this does not land cleanly.

gibfahn · 2017-06-17T23:52:51Z

I suspect that it's not worth trying to backport any more complex performance improvements to v6.x, all the benchmarks will have to be rerun etc.

Having said that, if anyone is willing to do the work, then by all means do backport perf improvements.

nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Apr 2, 2017

lpinca reviewed Apr 3, 2017

View reviewed changes

benchmark: replace [].join() with ''.repeat()

2960759

Also add a benchmark to compare both ways to create strings.

lpinca approved these changes Apr 3, 2017

View reviewed changes

jasnell approved these changes Apr 4, 2017

View reviewed changes

jasnell pushed a commit that referenced this pull request Apr 4, 2017

benchmark: replace [].join() with ''.repeat()

74dc3bf

Also add a benchmark to compare both ways to create strings. PR-URL: #12170 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell closed this Apr 4, 2017

vsemozhetbyt deleted the string-repeat branch April 4, 2017 16:39

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

JacksonTian mentioned this pull request Apr 10, 2017

test: replace [].join() with ''.repeat() #12305

Closed

2 tasks

vsemozhetbyt mentioned this pull request Apr 10, 2017

benchmark: replace more [].join() with ''.repeat() #12317

Closed

1 task

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

JacksonTian added a commit to JacksonTian/node that referenced this pull request Apr 11, 2017

test: replace [].join() with ''.repeat()

984c859

Like PR nodejs#12170 .

MylesBorins added the backport-requested-v6.x label Apr 18, 2017

MylesBorins added lts-watch-v6.x and removed backport-requested-v6.x labels Apr 18, 2017

evanlucas pushed a commit that referenced this pull request Apr 25, 2017

benchmark: replace more [].join() with ''.repeat()

7420571

Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>

evanlucas pushed a commit that referenced this pull request May 1, 2017

benchmark: replace more [].join() with ''.repeat()

32da4a2

Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>

evanlucas pushed a commit that referenced this pull request May 2, 2017

benchmark: replace more [].join() with ''.repeat()

f316b50

Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added dont-land-on-v6.x and removed lts-watch-v6.x labels Jun 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

benchmark: replace [].join() with ''.repeat() #12170

benchmark: replace [].join() with ''.repeat() #12170

vsemozhetbyt commented Apr 2, 2017 •

edited

Loading

vsemozhetbyt commented Apr 2, 2017

lpinca Apr 3, 2017

vsemozhetbyt Apr 3, 2017

lpinca Apr 3, 2017

vsemozhetbyt Apr 3, 2017

vsemozhetbyt commented Apr 3, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017

gibfahn commented May 15, 2017

vsemozhetbyt commented May 15, 2017 •

edited

Loading

gibfahn commented Jun 17, 2017

benchmark: replace [].join() with ''.repeat() #12170

benchmark: replace [].join() with ''.repeat() #12170

Conversation

vsemozhetbyt commented Apr 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Apr 2, 2017

lpinca Apr 3, 2017

Choose a reason for hiding this comment

vsemozhetbyt Apr 3, 2017

Choose a reason for hiding this comment

lpinca Apr 3, 2017

Choose a reason for hiding this comment

vsemozhetbyt Apr 3, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 3, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017

gibfahn commented May 15, 2017

vsemozhetbyt commented May 15, 2017 • edited Loading

gibfahn commented Jun 17, 2017

vsemozhetbyt commented Apr 2, 2017 •

edited

Loading

vsemozhetbyt commented May 15, 2017 •

edited

Loading