From d5e9cfc692103c5a5bbde63b78728cbc7bd4f5d6 Mon Sep 17 00:00:00 2001 From: Giovanni Date: Thu, 15 Aug 2024 15:00:13 +0200 Subject: [PATCH 1/6] benchmark: adds groups to better separate benchmarks Fixes: https://github.com/nodejs/node/issues/26425 Co-Authored-By: Yaman Kassir --- benchmark/common.js | 31 ++++++++++++------- benchmark/http/headers.js | 28 ++++++++++++----- .../writing-and-running-benchmarks.md | 17 ++++++++++ 3 files changed, 57 insertions(+), 19 deletions(-) diff --git a/benchmark/common.js b/benchmark/common.js index efe21e871571fc..b4978e8e140b18 100644 --- a/benchmark/common.js +++ b/benchmark/common.js @@ -22,27 +22,36 @@ class Benchmark { this.name = require.main.filename.slice(__dirname.length + 1); // Execution arguments i.e. flags used to run the jobs - this.flags = process.env.NODE_BENCHMARK_FLAGS ? - process.env.NODE_BENCHMARK_FLAGS.split(/\s+/) : - []; + this.flags = process.env.NODE_BENCHMARK_FLAGS?.split(/\s+/) ?? []; // Parse job-specific configuration from the command line arguments const argv = process.argv.slice(2); const parsed_args = this._parseArgs(argv, configs, options); + this.options = parsed_args.cli; this.extra_options = parsed_args.extra; + this.combinationFilter = typeof options.combinationFilter === 'function' ? options.combinationFilter : allow; + + if (options.byGroups) { + this.queue = []; + const groupNames = process.env.NODE_RUN_BENCHMARK_GROUPS?.split(',') ?? Object.keys(configs); + + for (const groupName of groupNames) { + const config = { ...configs[groupName][0], group: groupName }; + const parsed_args = this._parseArgs(argv, config, options); + + this.options = parsed_args.cli; + this.extra_options = parsed_args.extra; + this.queue = this.queue.concat(this._queue(this.options)); + } + } else { + this.queue = this._queue(this.options); + } + if (options.flags) { this.flags = this.flags.concat(options.flags); } - if (typeof options.combinationFilter === 'function') - this.combinationFilter = options.combinationFilter; - else - this.combinationFilter = allow; - - // The configuration list as a queue of jobs - this.queue = this._queue(this.options); - if (this.queue.length === 0) return; diff --git a/benchmark/http/headers.js b/benchmark/http/headers.js index e995f380cef151..62612d9fda1911 100644 --- a/benchmark/http/headers.js +++ b/benchmark/http/headers.js @@ -4,10 +4,22 @@ const common = require('../common.js'); const http = require('http'); const bench = common.createBenchmark(main, { - n: [10, 600], - len: [1, 100], - duration: 5, -}); + fewHeaders: { + n: [10], + len: [1, 5], + duration: 5, + }, + mediumHeaders: { + n: [50], + len: [1, 10], + duration: 5, + }, + manyHeaders: { + n: [600], + len: [1, 100], + duration: 5, + }, +}, { byGroups: true }); function main({ len, n, duration }) { const headers = { @@ -15,10 +27,9 @@ function main({ len, n, duration }) { 'Transfer-Encoding': 'chunked', }; - // TODO(BridgeAR): Change this benchmark to use grouped arguments when - // implemented. https://github.com/nodejs/node/issues/26425 - const Is = [ ...Array(Math.max(n / len, 1)).keys() ]; - const Js = [ ...Array(len).keys() ]; + const Is = [...Array(n / len).keys()]; + const Js = [...Array(len).keys()]; + for (const i of Is) { headers[`foo${i}`] = Js.map(() => `some header value ${i}`); } @@ -27,6 +38,7 @@ function main({ len, n, duration }) { res.writeHead(200, headers); res.end(); }); + server.listen(0, () => { bench.http({ path: '/', diff --git a/doc/contributing/writing-and-running-benchmarks.md b/doc/contributing/writing-and-running-benchmarks.md index f70ff965a8d7d1..18fb45a58128de 100644 --- a/doc/contributing/writing-and-running-benchmarks.md +++ b/doc/contributing/writing-and-running-benchmarks.md @@ -492,6 +492,23 @@ The arguments of `createBenchmark` are: * `options` {Object} The benchmark options. Supported options: * `flags` {Array} Contains node-specific command line flags to pass to the child process. + + * `byGroups` option for processing `configs` by groups: + ```js + const bench = common.createBenchmark(main, { + groupA: { + source: ['array'], + len: [10, 2048], + n: [50], + }, + groupB: { + source: ['buffer', 'string'], + len: [2048], + n: [50, 2048], + } + }, { byGroups: true }); + ``` + * `combinationFilter` {Function} Has a single parameter which is an object containing a combination of benchmark parameters. It should return `true` or `false` to indicate whether the combination should be included or not. From 699665ffac3b436d2a899320e6a3229c77bbb70d Mon Sep 17 00:00:00 2001 From: Giovanni Date: Sat, 17 Aug 2024 09:59:30 +0200 Subject: [PATCH 2/6] doc: adjusted benchmarks docs to describe how to use the byGroups flag --- doc/contributing/writing-and-running-benchmarks.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/doc/contributing/writing-and-running-benchmarks.md b/doc/contributing/writing-and-running-benchmarks.md index 18fb45a58128de..1cf080cc6d3f2f 100644 --- a/doc/contributing/writing-and-running-benchmarks.md +++ b/doc/contributing/writing-and-running-benchmarks.md @@ -272,6 +272,15 @@ process/bench-env.js operation="query" n=1000000: 3,625,787.2150573144 process/bench-env.js operation="delete" n=1000000: 1,521,131.5742806569 ``` +Benchmarks can also have groups, giving the developer greater flexibility in differentiating between test cases. +By default, all groups are executed when running the benchmarks. +However, it is possible to specify individual groups by setting the `NODE_RUN_BENCHMARK_GROUPS` +environment variable when running `compare.js`: + +```bash +NODE_RUN_BENCHMARK_GROUPS=fewHeaders,manyHeaders node http/headers.js +``` + ### Comparing Node.js versions To compare the effect of a new Node.js version use the `compare.js` tool. This @@ -493,7 +502,7 @@ The arguments of `createBenchmark` are: * `flags` {Array} Contains node-specific command line flags to pass to the child process. - * `byGroups` option for processing `configs` by groups: + * `byGroups` {Boolean} option for processing `configs` by groups: ```js const bench = common.createBenchmark(main, { groupA: { From 35abdb05ae7e06e8cb7a5b66b76363109f10df8f Mon Sep 17 00:00:00 2001 From: Giovanni Date: Sun, 18 Aug 2024 08:29:08 +0200 Subject: [PATCH 3/6] benchmark: updated the benchmark.js file to keep in consideration groups --- test/common/benchmark.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/benchmark.js b/test/common/benchmark.js index d9c1cdc627d994..ffa0a45673bbb5 100644 --- a/test/common/benchmark.js +++ b/test/common/benchmark.js @@ -31,10 +31,10 @@ function runBenchmark(name, env) { // that the benchmark file runs just one set of options. This helps keep the // benchmark tests from taking a long time to run. Therefore, each benchmark // file should result in three lines of output: a blank line, a line with - // the name of the benchmark file, and a line with the only results that we - // get from testing the benchmark file. + // the name of the benchmark file, and from 1 to N lines with the only results that we + // get from testing the benchmark file, depending on how many groups the benchmark has. assert.ok( - /^(?:\n.+?\n.+?\n)+$/.test(stdout), + /^(?:\n.+?\n.+?\n)+\n.+?$/gm.test(stdout), `benchmark file not running exactly one configuration in test: ${stdout}`, ); }); From 9d964a0d45420ba5799c00c04768a91acf31ede9 Mon Sep 17 00:00:00 2001 From: Giovanni Date: Sun, 18 Aug 2024 18:59:29 +0200 Subject: [PATCH 4/6] benchmark: updated the benchmark.js script to be more strict --- test/common/benchmark.js | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/test/common/benchmark.js b/test/common/benchmark.js index ffa0a45673bbb5..94383d55ca470f 100644 --- a/test/common/benchmark.js +++ b/test/common/benchmark.js @@ -27,16 +27,27 @@ function runBenchmark(name, env) { child.on('exit', (code, signal) => { assert.strictEqual(code, 0); assert.strictEqual(signal, null); + // This bit makes sure that each benchmark file is being sent settings such // that the benchmark file runs just one set of options. This helps keep the - // benchmark tests from taking a long time to run. Therefore, each benchmark - // file should result in three lines of output: a blank line, a line with - // the name of the benchmark file, and from 1 to N lines with the only results that we - // get from testing the benchmark file, depending on how many groups the benchmark has. - assert.ok( - /^(?:\n.+?\n.+?\n)+\n.+?$/gm.test(stdout), - `benchmark file not running exactly one configuration in test: ${stdout}`, - ); + // benchmark tests from taking a long time to run. Therefore, stdout should be composed as follows: + // The first and last lines should be empty. + // Each test should be separated by a blank line. + // The first line of each test should contain the test's name. + // The second line of each test should contain the configuration for the test. + // If the test configuration is not a group, there should be exactly two lines. + // Otherwise, it is possible to have more than two lines. + + const splitTests = stdout.split(/\n\s*\n/); + + for (let testIdx = 1; testIdx < splitTests.length - 1; testIdx++) { + const lines = splitTests[testIdx].split('\n'); + assert.ok(/.+/.test(lines[0])); + + if (!lines[1].includes('group="')) { + assert.strictEqual(lines.length, 2, `benchmark file not running exactly one configuration in test: ${stdout}`); + } + } }); } From f9e929b6b36a8183cc092d83bb50ddd0e97e9a04 Mon Sep 17 00:00:00 2001 From: Giovanni Bucci Date: Mon, 19 Aug 2024 08:27:10 +0200 Subject: [PATCH 5/6] doc: update benchmark description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Vinicius Lourenço <12551007+H4ad@users.noreply.github.com> --- doc/contributing/writing-and-running-benchmarks.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/contributing/writing-and-running-benchmarks.md b/doc/contributing/writing-and-running-benchmarks.md index 1cf080cc6d3f2f..1f62774f71a093 100644 --- a/doc/contributing/writing-and-running-benchmarks.md +++ b/doc/contributing/writing-and-running-benchmarks.md @@ -272,10 +272,11 @@ process/bench-env.js operation="query" n=1000000: 3,625,787.2150573144 process/bench-env.js operation="delete" n=1000000: 1,521,131.5742806569 ``` -Benchmarks can also have groups, giving the developer greater flexibility in differentiating between test cases. -By default, all groups are executed when running the benchmarks. -However, it is possible to specify individual groups by setting the `NODE_RUN_BENCHMARK_GROUPS` -environment variable when running `compare.js`: +#### Grouping benchmarks + +Benchmarks can also have groups, giving the developer greater flexibility in differentiating between test cases and also helping reduce the time to run the combination of benchmark parameters. + +By default, all groups are executed when running the benchmark. However, it is possible to specify individual groups by setting the `NODE_RUN_BENCHMARK_GROUPS` environment variable when running `compare.js`: ```bash NODE_RUN_BENCHMARK_GROUPS=fewHeaders,manyHeaders node http/headers.js From 8a0610454412f1e3b8f4f861db926794bd0c36ed Mon Sep 17 00:00:00 2001 From: Giovanni Date: Tue, 20 Aug 2024 07:41:53 +0200 Subject: [PATCH 6/6] doc: linting in benchmark docs and in comments in benchmark.js --- doc/contributing/writing-and-running-benchmarks.md | 7 +++++-- test/common/benchmark.js | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/contributing/writing-and-running-benchmarks.md b/doc/contributing/writing-and-running-benchmarks.md index 1f62774f71a093..21059252926757 100644 --- a/doc/contributing/writing-and-running-benchmarks.md +++ b/doc/contributing/writing-and-running-benchmarks.md @@ -274,9 +274,12 @@ process/bench-env.js operation="delete" n=1000000: 1,521,131.5742806569 #### Grouping benchmarks -Benchmarks can also have groups, giving the developer greater flexibility in differentiating between test cases and also helping reduce the time to run the combination of benchmark parameters. +Benchmarks can also have groups, giving the developer greater flexibility in differentiating between test cases +and also helping reduce the time to run the combination of benchmark parameters. -By default, all groups are executed when running the benchmark. However, it is possible to specify individual groups by setting the `NODE_RUN_BENCHMARK_GROUPS` environment variable when running `compare.js`: +By default, all groups are executed when running the benchmark. +However, it is possible to specify individual groups by setting the +`NODE_RUN_BENCHMARK_GROUPS` environment variable when running `compare.js`: ```bash NODE_RUN_BENCHMARK_GROUPS=fewHeaders,manyHeaders node http/headers.js diff --git a/test/common/benchmark.js b/test/common/benchmark.js index 94383d55ca470f..7211ff8703e0e8 100644 --- a/test/common/benchmark.js +++ b/test/common/benchmark.js @@ -27,7 +27,7 @@ function runBenchmark(name, env) { child.on('exit', (code, signal) => { assert.strictEqual(code, 0); assert.strictEqual(signal, null); - + // This bit makes sure that each benchmark file is being sent settings such // that the benchmark file runs just one set of options. This helps keep the // benchmark tests from taking a long time to run. Therefore, stdout should be composed as follows: