benchmark: allow multiple values for same config #11819
Conversation
nodejs-github-bot
added
the
benchmark
|
Per 2826e63, maybe the correct term would be "config" rather than "option". |
|
Yes, those are |
seishun
changed the title
|
Updated commit message. |
|
@mscdex @nodejs/benchmarking |
benchmark/common.js
Outdated
| @@ -52,12 +52,13 @@ Benchmark.prototype._parseArgs = function(argv, configs) { | |||
| // Infer the type from the config object and parse accordingly | |||
| const isNumber = typeof configs[match[1]][0] === 'number'; | |||
| const value = isNumber ? +match[2] : match[2]; | |||
| cliOptions[match[1]] = [value]; | |||
| if (!cliOptions[match[1]]) cliOptions[match[1]] = []; | |||
There was a problem hiding this comment.
Can we assign match[1] to a variable and make the block of this conditional indented on the following line instead?
There was a problem hiding this comment.
Can we assign match[1] to a variable
Sure, if you suggest a variable name.
make the block of this conditional indented on the following line instead?
I'd then have to wrap it in braces, causing this simple default-assignment to take up three lines. But if that's how it's usually done in node's code, then so be it.
There was a problem hiding this comment.
As far as a variable name goes, I don't really have a preference. How about something simple like param?
There was a problem hiding this comment.
It shouldn't need braces.
Really? I think I've always seen single-line conditional bodies wrapped in braces in node's codebase, so I assumed that's our style.
As far as a variable name goes, I don't really have a preference.
If you don't have a preference, I'll go with config instead.
There was a problem hiding this comment.
Really? I think I've always seen single-line conditional bodies wrapped in braces in node's codebase, so I assumed that's our style.
It's a mix. There is no strict rule for it, although the one time you definitely would want to use braces for a single line is if the conditional part spans multiple lines.
Personally when I am visually scanning code and I see an if or similar control flow statement, I assume the next line is part of its body. So cases like this just it harder to read (at least for me). Also it's visually easier/quicker to see where the conditional part ends. About the only time I ever create such lines of code is when I'm really short on space when I'm trying to make a function inlineable with Crankshaft, and even then I try to leave at least an empty line afterwards.
|
@mscdex in case Github didn't send a notification: PTAL. |
|
LGTM |
|
Thanks, squashed and landed in 5d9b1e8a635473b4caafe09ad9c971e2aa297d93 |
|
@fhinkel it seems you changed the example command line in the commit message to span two lines. I think it should be on a single line for clarity and readability. Could you force-push? |
|
I went ahead and force-pushed. I'm okay with wrapping a command line if there is agreement that it should be, but I'd prefer to confirm it first. |
|
@seishun ... did you intend to reopen this? Is this landed? |
|
@jasnell It's not landed currently. I wanted to postpone landing until I get some input on whether the example command line in the commit message should be wrapped. (I think it shouldn't because it's not prose and wrapping would make it less clear) |
|
wrapping shouldn't be necessary, but I would prefer it to be included in a code block .. e.g. using ``` |
This allows running a benchmark with two or more values for the same config rather than just one or all of them, for example: ``` node benchmark/buffers/buffer-creation.js type=buffer() type=fast-alloc type=fast-alloc-fill ``` PR-URL: #11819 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Brian White <[email protected]>
|
Agreed. I've added a code block. |
|
Landed in 43fa0a8. |
This allows running a benchmark with two or more values for the same config rather than just one or all of them, for example: ``` node benchmark/buffers/buffer-creation.js type=buffer() type=fast-alloc type=fast-alloc-fill ``` PR-URL: #11819 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Brian White <[email protected]>
|
should thi sbackport? I'm not familiar with the benchmark setup so not in a good place to test this |
|
No, I don't think it's worth bothering. |
This allows running a benchmark with two or more values for the same option rather than just one or all of them, for example:
Checklist
Affected core subsystem(s)
benchmark