-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
benchmark: reduce string concatenations #12455
Conversation
'Uint32Array', | ||
'Float32Array', | ||
'Float64Array', | ||
'Uint8ClampedArray', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indented too far.
benchmark/_http-benchmarkers.js
Outdated
'https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md##http-benchmark-requirements ' + | ||
'for further instructions.')); | ||
callback(new Error(`Could not locate required http benchmarker. See ${ | ||
requirementsURL} for further instructions.`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the odd indentation here and elsewhere? Shouldn't they line up with the beginning of the string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to stress the subordinate state of the next part. I'll try to decrease indentation.
`| ${fraction(completedRunsForFile, runsPerFile)} runs ` + | ||
`| ${fraction(completedConfig, scheduledConfig)} configs]` + | ||
`: ${caption} `; | ||
return `[${getTime(diff)}|% ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I'm not particularly a fan of this style to avoid the extra spacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ${
followed by a newline to avoid including spaces due to indentation.
Some indentation was reduced. New CI: https://ci.nodejs.org/job/node-test-pull-request/7435/ |
@@ -54,9 +54,3 @@ function main(conf) { | |||
} | |||
bench.end(n); | |||
} | |||
|
|||
function buildString(str, times) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny we even had a recursive implementation :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I am even sorry to retire it)
' buff.' + fn + '(i, 0, ' + JSON.stringify(noAssert) + ');', | ||
'}' | ||
].join('\n')); | ||
var testFunction = new Function('buff', ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much nicer
benchmark/common.js
Outdated
@@ -44,7 +44,7 @@ Benchmark.prototype._parseArgs = function(argv, configs) { | |||
for (const arg of argv) { | |||
const match = arg.match(/^(.+?)=([\s\S]*)$/); | |||
if (!match || !match[1]) { | |||
console.error('bad argument: ' + arg); | |||
console.error(`bad argument: ${arg}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.error('bad argument:', arg)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would imply we need inspect()
for arg
(as we do for Error objects), while arg
is a simple string.
@@ -30,14 +30,14 @@ function main(conf) { | |||
var len = +conf.millions * 1e6; | |||
var clazz = conf.buf === 'fast' ? Buffer : require('buffer').SlowBuffer; | |||
var buff = new clazz(8); | |||
var fn = 'read' + conf.type; | |||
var fn = `read${conf.type}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a reminder that this will make it impossible to run these benchmarks to compare against anything older than 4.0.0. That's not an objection, by any means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but there were many other template literals in benchmarks before this edition, including common.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I don't think it really matters. I can't see anyone wanting to run the benchmarks for v0.10 or v0.12 anymore, especially since it wouldn't really do any good (it's not as if the V8 team can/will revert to v0.10/v0.12-era V8 code if there is a performance issue in node v4.x+).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, as I said, it's not an objection. Just noting the change. We may want to let folks know just so that they're aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of benchmarks don't run on v4.x or older anyways...I've seen people on IRC
asking why some benchmark doesn't run and turns out they are using v4.x to run it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
' for (var i = 0; i <= n; i++)' + | ||
' buf.' + method + '();' + | ||
'}'; | ||
const fnString = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not a fan of multiline literals.
New CI to be sure: https://ci.nodejs.org/job/node-test-pull-request/7520/ |
1 Linux fail seems unrelated. Timeout in UPD. Waiting for #12518 |
CI: https://ci.nodejs.org/job/node-test-pull-request/7538/ Sigh... Aborted. New CI: https://ci.nodejs.org/job/node-test-pull-request/7539/ |
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in bbbb1f6...d8965d5 |
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12455 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12735 Refs: #12455 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: nodejs#12735 Refs: nodejs#12455 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #12735 Refs: #12455 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Backport-PR-URL: #13835 PR-URL: #12735 Refs: #12455 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Backport-PR-URL: #13835 PR-URL: #12735 Refs: #12455 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Checklist
Affected core subsystem(s)
benchmark
I. Presupposition
String concatenations usually increase syntax noise and add confusion (a reader often has to check if string concatenation or number addition is intended). Moreover, it seems this method is not optimal performance-wise.
II. Cases
This PR aims two cases of string concatenation:
'a' + b + 'c'
);String
('' + notString
). This approach, while idiomatic, is not most declarative.III. Performance
The added benchmark compares these variants:
'a' + b + 'c'
vs['a', b, 'c'].join('')
vs`a${b}c`
String(notString)
vs'' + notString
vs`${notString}`
Results with Node.js 8.0.0 rc:
IV. Commits
Reduce string concatenations: replace concatenations with template literals, rewrap,
String.prototype.repeat()
,String()
. Some replacements do not add indisputable readability gain, but I tried to be consistent.Add a benchmark for string concatenations vs counterparts.
This commit is added here to avoid PR race condition and merge conflicts. It fixes an URL in
_http-benchmarkers.js
(removes duplicate hash symbol).This commit is added here to avoid PR race condition and merge conflicts. It fixes an evident typo in
_http-benchmarkers.js
:Error()
constructor has only one parameter, whilecallback()
has 5 (compare a previous call andcallback()
signature).All changed files were minimally tested. CI for linting and new tests concerning benchmarks will be launched.