benchmark: add "byGroup" config option

[BeniCheni]

Resolves #26425

• Implement the byGroup benchmark config option
• Add an example with buffer-from-by-group.js benchmark
• Update writing-and-running-benchmarks.md to mention the byGroup option:

example in "buffer-from-by-group.js" benchmark

const bench = common.createBenchmark(main, {
  groupA: {
    source: [
      'array',
      'arraybuffer',
      'arraybuffer-middle',
      'buffer',
      'uint8array',
      'string',
      'string-utf8',
      'string-base64',
      'object',
    ],
    len: [10, 2048],
    n: [2048]
  },
  groupB: {
    source: [
      'buffer',
      'string',
    ],
    len: [2048],
    n: [2048]
  }
}, { byGroup: true });

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

This change causes benchmark/test-benchmark-buffer to fail. (Run make test-benchmark to run the benchmark tests. Because they can be slow, they are only run once a day in CI rather than on every run.)

The benchmark tests are designed to do a minimal test that all benchmarks work. In the past, we've had problems with benchmarks breaking and nobody noticing.

To make sure the benchmark tests don't take hours to run, each benchmark file is sent a series of command-line --set flags such that it only runs one configuration. If any benchmark runs two configurations (like the new one here does) or more than two configurations, the test fails.

Would it be possible to make it so that command-line --set flags disable the group and return to a matrix? (Maybe add default values for each configuration value so that a user does not need to specify every value with --set but only the ones they care about?) If --set disables the grouping, then the test can be made to pass again with minimal changes.

Another (perhaps easier) option is to provide a command-line option that disables certain groups. So, if you had three configuration groups specified, you could use a command-line option to say "I only care about the first group, don't run the other ones".

I'm open to other ideas, but those are the two ideas that come to mind for me. I think being able to override the settings at the command line is a good feature anyway, even if it wasn't for the testing issue.

[BeniCheni]

Hi @Trott, thanks for the insight. I've updated to try the following pattern, and was able to pass ./node test/benchmark/test-benchmark-buffer.js. PTAL? (Thanks.)

Use NODEJS_BENCHMARK_BYPASS_GROUP: 1 flag to indicate to use only the configs from the first group. For any benchmark with group, (e.g. buffer-from-by-group.js in this PR), the test/benchmark/test-benchmark-buffer.js would use this flag:

runBenchmark('buffers',
[
   ...
],
{
  ...
  NODEJS_BENCHMARK_BYPASS_GROUP: 1
});

In benchmark/common.js, this block would process group / configs in first group / configs not in group conditionally in functions:

if (byGroup) {
  if (process.env.NODEJS_BENCHMARK_BYPASS_GROUP) {
    enqueueConfigsInFirstGroup(configs);
  } else {
    enqueueConfigsInGroups(configs);
  }
} else {
  enqueueConfigs(configs);
}

[Trott]

PTAL? (Thanks.)

Use NODEJS_BENCHMARK_BYPASS_GROUP: 1 flag to indicate to use only the configs from the first group.

Seems like a good solution to me.

[BeniCheni]

Nit: camelCase? parsedArgs

Per @Trott's comment updated related variable names to parsedArgs.

[BeniCheni]

Hi @BridgeAR, saw the proposed TODO note in this comment in #27021, and the self-requested for a review.

// TODO(BridgeAR): Change this benchmark to use grouped arguments when
// implemented. https://github.com/nodejs/node/issues/26425

Please let me know if there's any update I could make to this PR to support the goal of using grouped arguments for benchmark. Happy to update further. Thanks.

[sam-github]

This seems useful to me, does anyone know what is blocking it?

[Trott]

This seems useful to me, does anyone know what is blocking it?

Needs a rebase and needs reviews, but I don't think there's anything blocking it.

[lundibundi]

It looks like there is no indication of Group in the output, this will result in hard to understand results of such benchmarks.
i.e. the bench in this PR results in

buffers/buffer-from-by-group.js n=1 len=10 source="array": 301.2715164351144
buffers/buffer-from-by-group.js n=1 len=2048 source="array": 116.51600370614103
buffers/buffer-from-by-group.js n=1 len=10 source="arraybuffer": 2,001.965930543794
buffers/buffer-from-by-group.js n=1 len=2048 source="arraybuffer": 1,816.325863390499
buffers/buffer-from-by-group.js n=1 len=10 source="arraybuffer-middle": 1,627.6915915080074
buffers/buffer-from-by-group.js n=1 len=2048 source="arraybuffer-middle": 1,450.5724684246638
buffers/buffer-from-by-group.js n=1 len=10 source="buffer": 362.11180709000433
buffers/buffer-from-by-group.js n=1 len=2048 source="buffer": 284.6799215536008
buffers/buffer-from-by-group.js n=1 len=10 source="uint8array": 417.65703591307556
buffers/buffer-from-by-group.js n=1 len=2048 source="uint8array": 284.73496015703705
buffers/buffer-from-by-group.js n=1 len=10 source="string": 540.6369784880546
buffers/buffer-from-by-group.js n=1 len=2048 source="string": 390.73962713280343
buffers/buffer-from-by-group.js n=1 len=10 source="string-utf8": 846.1926408318411
buffers/buffer-from-by-group.js n=1 len=2048 source="string-utf8": 314.03287296114155
buffers/buffer-from-by-group.js n=1 len=10 source="string-base64": 524.7889298923973
buffers/buffer-from-by-group.js n=1 len=2048 source="string-base64": 123.99171487361215
buffers/buffer-from-by-group.js n=1 len=10 source="object": 1,607.3655920891895
buffers/buffer-from-by-group.js n=1 len=2048 source="object": 1,594.090388113187
buffers/buffer-from-by-group.js n=1 len=2048 source="buffer": 238.9843737677368
buffers/buffer-from-by-group.js n=1 len=2048 source="string": 258.60321164500607

Making it impossible to distinguish

buffers/buffer-from-by-group.js n=1 len=2048 source="buffer": 284.6799215536008
# and
buffers/buffer-from-by-group.js n=1 len=2048 source="buffer": 238.9843737677368

Should we perhaps just always run the first group of the benchmark unless specified which group to run by some option?

[lundibundi]

I think this can be simplified to

const parseConfig = (config) => {
  const parsedArgs = this._parseArgs(process.argv.slice(2), config);
  this.options = parsedArgs.cli;
  this.extra_options = parsedArgs.extra;
  this.queue = [ ...this.queue, ...this._queue(this.options) ];
}

if (byGroup) {
  let usedConfigs = Object.values(configs);
  if (process.env.NODEJS_BENCHMARK_BYPASS_GROUP)
    usedConfigs = usedConfigs[0];
  usedConfigs.forEach(parseConfig);
} else {
  parseConfig(configs);
}

[github-actions]

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.