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.

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: replace more [].join() with ''.repeat() #12317

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
Affected core subsystem(s)

benchmark

In #12170 I forgot to check cases without new keyword. So these are 2 missing ones addressed.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. http Issues or PRs related to the http subsystem. labels Apr 10, 2017
@vsemozhetbyt
Copy link
Contributor Author

@@ -19,7 +19,7 @@ function main(conf) {
const dur = +conf.dur;
const len = +conf.len;

const msg = '"' + Array(len).join('.') + '"';
const msg = `"${'.'.repeat(len)}"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is doing something different from before, it should be '.'.repeat(len - 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in the strict sense. But it is not a test, so there are no assertions or checks. It seems, the intention was the length should be [64, 256, 1024, 4096], just + 1 was not bothered to be added. Or do I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure -- since you only ran the linter, I had assumed that this wasn't intended to make any functional changes to the code. (Since it seems like the functional changes are intentional, please ignore this review.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've run only the linter because it seems CI does not run benchmarks. There are some new tests for this, but they cover only net and http benchmarks for now. But I've run these files locally and have not found any difference or errors. However, to be on the safe side: @nodejs/benchmarking , @nodejs/performance — what do you think?

@joyeecheung
Copy link
Member

I have opened #12326 to test child_process benchmarks. Also this can still run the existing http benchmark tests since it touches http benchmarks.

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

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

Landed in 0f69f40

@vsemozhetbyt vsemozhetbyt deleted the benchmark-string-repeat-2 branch April 12, 2017 21:33
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
Refs: #12170

PR-URL: #12317
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
Refs: #12170

PR-URL: #12317
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
Refs: #12170

PR-URL: #12317
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

should this be backported to v6.x?

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

ping @vsemozhetbyt

@vsemozhetbyt
Copy link
Contributor Author

It seems this falls within #12170 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants