From 996719fcf9c6ebe4b14c5f2820e8f67dcbd832a3 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 26 Nov 2022 15:39:42 +0100 Subject: [PATCH 1/5] benchmark: make benchmarks runnable in older versions of Node.js Also remove the require-cachable.js benchmarks because now all builtin modules are cacheable, it would be comparing oranges to apples when we try to compare the performance of loading all cacheable modules in different Node.js binaries since the set of modules are just different. Comparison of startup performance that involves loading of the long-standing, stable builtins is already covered by the require-builtins benchmark. --- benchmark/common.js | 8 ++++---- benchmark/fixtures/require-cachable.js | 10 ---------- benchmark/misc/startup.js | 2 +- 3 files changed, 5 insertions(+), 15 deletions(-) delete mode 100644 benchmark/fixtures/require-cachable.js diff --git a/benchmark/common.js b/benchmark/common.js index 6ed230ffde4231..918eaa5e0adbb4 100644 --- a/benchmark/common.js +++ b/benchmark/common.js @@ -38,7 +38,7 @@ class Benchmark { this.config = this.queue[0]; process.nextTick(() => { - if (Object.hasOwn(process.env, 'NODE_RUN_BENCHMARK_FN')) { + if (process.env.NODE_RUN_BENCHMARK_FN !== undefined) { fn(this.config); } else { // _run will use fork() to create a new process for each configuration @@ -91,7 +91,7 @@ class Benchmark { process.exit(1); } const [, key, value] = match; - if (Object.hasOwn(configs, key)) { + if (configs[key] !== undefined) { if (!cliOptions[key]) cliOptions[key] = []; cliOptions[key].push( @@ -290,10 +290,10 @@ function sendResult(data) { if (process.send) { // If forked, report by process send process.send(data, () => { - if (Object.hasOwn(process.env, 'NODE_RUN_BENCHMARK_FN')) { + if (process.env.NODE_RUN_BENCHMARK_FN !== undefined) { // If, for any reason, the process is unable to self close within // a second after completing, forcefully close it. - setTimeout(() => { + require('timers').setTimeout(() => { process.exit(0); }, 5000).unref(); } diff --git a/benchmark/fixtures/require-cachable.js b/benchmark/fixtures/require-cachable.js deleted file mode 100644 index 105652a51855eb..00000000000000 --- a/benchmark/fixtures/require-cachable.js +++ /dev/null @@ -1,10 +0,0 @@ -'use strict'; - -const { internalBinding } = require('internal/test/binding'); -const { - builtinCategories: { canBeRequired } -} = internalBinding('builtins'); - -for (const key of canBeRequired) { - require(`node:${key}`); -} diff --git a/benchmark/misc/startup.js b/benchmark/misc/startup.js index dea5a31753f8fe..561c516155bc01 100644 --- a/benchmark/misc/startup.js +++ b/benchmark/misc/startup.js @@ -64,7 +64,7 @@ function main({ dur, script, mode }) { throughput: 0 }; - setTimeout(() => { + require('timers').setTimeout(() => { state.go = false; }, dur * 1000); From bc05a5f624b88594daed6f34218abdec5162e382 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 5 Dec 2022 11:07:08 +0100 Subject: [PATCH 2/5] benchmark: rewrite the startup performance benchmark The previous benchmark was inaccurate in several ways: - It previously measured "how many workers/child processes can finish loading in parallel in 1 second" which is prone to wrong interpretations. It's also not typical for users to spin up >20 workers/child processes in parallel, which was what the benchmark usually ended up doing. The parallelism can also be greatly affected by system configurations. - The time spent on loading a Node.js binary for the first time varies greatly depending on system configurations, but it was inevitable for the benchmark to load Node.js multiple times to reduce the influence of fluctuations. This patch rewrites the benchmark so that: - It now measures "how long it takes to finish 30 workers/child processes in serial" which is generally more in line with how other benchmarks are written and with the figures one gets from doing something like `time node index.js` or `hyperfine 'node index.js'` - It now warms up the loading of the Node.js instance to reduce the statistical outliers, so that we get more meaningful figures when trying to compare the startup performance of different Node.js binaries. --- benchmark/misc/startup.js | 90 +++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 51 deletions(-) diff --git a/benchmark/misc/startup.js b/benchmark/misc/startup.js index 561c516155bc01..2a1e62922a783f 100644 --- a/benchmark/misc/startup.js +++ b/benchmark/misc/startup.js @@ -1,80 +1,68 @@ 'use strict'; const common = require('../common.js'); -const { spawn } = require('child_process'); +const { spawn, spawnSync } = require('child_process'); const path = require('path'); let Worker; // Lazy loaded in main const bench = common.createBenchmark(main, { - dur: [1], script: [ 'benchmark/fixtures/require-builtins', - 'benchmark/fixtures/require-cachable', 'test/fixtures/semicolon', ], - mode: ['process', 'worker'] -}, { - flags: ['--expose-internals'] + mode: ['process', 'worker'], + count: [30], }); -function spawnProcess(script) { +function spawnProcess(script, bench, state) { const cmd = process.execPath || process.argv[0]; - const argv = ['--expose-internals', script]; - return spawn(cmd, argv); -} + while (state.finished < state.count) { + const child = spawnSync(cmd, [script]); + if (child.status !== 0) { + console.log('---- STDOUT ----'); + console.log(child.stdout.toString()); + console.log('---- STDERR ----'); + console.log(child.stderr.toString()); + throw new Error(`Child process stopped with exit code ${child.status}`); + } + state.finished++; + if (state.finished === 0) { + // Finished warmup. + bench.start(); + } -function spawnWorker(script) { - return new Worker(script, { stderr: true, stdout: true }); + if (state.finished == state.count) { + bench.end(state.count); + } + } } -function start(state, script, bench, getNode) { - const node = getNode(script); - let stdout = ''; - let stderr = ''; - - node.stdout.on('data', (data) => { - stdout += data; - }); - - node.stderr.on('data', (data) => { - stderr += data; - }); - - node.on('exit', (code) => { +function spawnWorker(script, bench, state) { + const child = new Worker(script); + child.on('exit', (code) => { if (code !== 0) { - console.error('------ stdout ------'); - console.error(stdout); - console.error('------ stderr ------'); - console.error(stderr); - throw new Error(`Error during node startup, exit code ${code}`); + throw new Error(`Worker stopped with exit code ${code}`); } - state.throughput++; - - if (state.go) { - start(state, script, bench, getNode); + state.finished++; + if (state.finished === 0) { + // Finished warmup. + bench.start(); + } + if (state.finished < state.count) { + spawnProcess(script, bench, state); } else { - bench.end(state.throughput); + bench.end(state.count); } }); } -function main({ dur, script, mode }) { - const state = { - go: true, - throughput: 0 - }; - - require('timers').setTimeout(() => { - state.go = false; - }, dur * 1000); - +function main({ count, script, mode }) { script = path.resolve(__dirname, '../../', `${script}.js`); + const state = { count, finished: -3 }; if (mode === 'worker') { - Worker = require('worker_threads').Worker; - bench.start(); - start(state, script, bench, spawnWorker); + Worker = require('worker_threads').Worker; // Warm up. + spawnWorker(script, bench, state); } else { - bench.start(); - start(state, script, bench, spawnProcess); + spawnProcess(script, bench, state); } } From 4b1b7628d954f7389033400ad4e241a2a647a925 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 5 Dec 2022 22:59:35 +0100 Subject: [PATCH 3/5] fixup! benchmark: rewrite the startup performance benchmark --- benchmark/misc/startup.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/benchmark/misc/startup.js b/benchmark/misc/startup.js index 2a1e62922a783f..e3ac4697552905 100644 --- a/benchmark/misc/startup.js +++ b/benchmark/misc/startup.js @@ -58,9 +58,10 @@ function spawnWorker(script, bench, state) { function main({ count, script, mode }) { script = path.resolve(__dirname, '../../', `${script}.js`); - const state = { count, finished: -3 }; + const warmup = 3; + const state = { count, finished: -warmup }; if (mode === 'worker') { - Worker = require('worker_threads').Worker; // Warm up. + Worker = require('worker_threads').Worker; spawnWorker(script, bench, state); } else { spawnProcess(script, bench, state); From 9f829385b9e930971f46335bebeab80c8722ed96 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 5 Dec 2022 23:00:18 +0100 Subject: [PATCH 4/5] fixup! fixup! benchmark: rewrite the startup performance benchmark --- benchmark/misc/startup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/misc/startup.js b/benchmark/misc/startup.js index e3ac4697552905..4fcf728cf71a09 100644 --- a/benchmark/misc/startup.js +++ b/benchmark/misc/startup.js @@ -31,7 +31,7 @@ function spawnProcess(script, bench, state) { bench.start(); } - if (state.finished == state.count) { + if (state.finished === state.count) { bench.end(state.count); } } From ee2cbe3043004883b7cfd974dcbb5e89bb46e1b0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 6 Dec 2022 15:45:44 +0100 Subject: [PATCH 5/5] fixup! fixup! fixup! benchmark: rewrite the startup performance benchmark --- benchmark/misc/startup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/misc/startup.js b/benchmark/misc/startup.js index 4fcf728cf71a09..c2cf7f2de658be 100644 --- a/benchmark/misc/startup.js +++ b/benchmark/misc/startup.js @@ -1,6 +1,6 @@ 'use strict'; const common = require('../common.js'); -const { spawn, spawnSync } = require('child_process'); +const { spawnSync } = require('child_process'); const path = require('path'); let Worker; // Lazy loaded in main