benchmark: replace more [].join() with ''.repeat() #12317
Conversation
nodejs-github-bot
added
benchmark
| const len = +conf.len; | ||
|
|
||
| const msg = '"' + Array(len).join('.') + '"'; | ||
| const msg = `"${'.'.repeat(len)}"`; |
There was a problem hiding this comment.
This is doing something different from before, it should be '.'.repeat(len - 1).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
|
I have opened #12326 to test child_process benchmarks. Also this can still run the existing http benchmark tests since it touches http benchmarks. |
|
Landed in 0f69f40 |
Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>
Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>
Refs: #12170 PR-URL: #12317 Reviewed-By: James M Snell <[email protected]>
|
should this be backported to v6.x? |
|
ping @vsemozhetbyt |
|
It seems this falls within #12170 (comment) |
7 participants
Checklist
Affected core subsystem(s)
benchmark
In #12170 I forgot to check cases without
newkeyword. So these are 2 missing ones addressed.