lib,benchmark: address multiple review issues

Refs: #65606 (comment)
Refs: #65606 (comment)
Refs: #65606 (comment)
PR-URL: #65631
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Author: jasnell

benchmark/README.md

@@ -80,9 +80,9 @@ Rscript benchmark/compare.R < compare-node-bench.csv
 
 Pass `--analyze` to run the same Welch analysis inline. `--max-regression N`
 implies `--analyze` and makes the command fail only when the Holm-Bonferroni
-adjusted p-value is below 0.05 and the full 95% confidence interval is worse
-than `-N%`. Requiring both conditions prevents a noisy point estimate from
-failing a regression gate.
+adjusted one-sided p-value against the `N%` threshold is below 0.05 and the full
+95% confidence interval is worse than `-N%`. Requiring both conditions prevents
+a noisy point estimate from failing a regression gate.
 
 ```console
 ./node benchmark/compare-node-bench.js \

benchmark/_node-bench-analysis.js

@@ -31,8 +31,19 @@ function holmAdjust(pValues) {
   return adjusted;
 }
 
+function thresholdPValue(oldRates, newHistogram, scale, maxRegression) {
+  const factor = 1 - maxRegression / 100;
+  if (factor <= 0) return 1;
+  const thresholdHistogram = createRateHistogram(
+    oldRates.map((rate) => rate * factor), scale, 3);
+  const result = thresholdHistogram.welchTest(newHistogram);
+  if (Number.isNaN(result.pValue)) return 1;
+  return result.tStatistic > 0 ?
+    result.pValue / 2 : 1 - result.pValue / 2;
+}
+
 function isRegressionFailure(row, maxRegression) {
-  return row.pAdjusted < 0.05 &&
+  return row.pThresholdAdjusted < 0.05 &&
          row.improvement + row.ci95 < -maxRegression;
 }
 
@@ -80,22 +91,32 @@ function analyzeCompare(samples, scale, maxRegression) {
                     result.confidenceInterval.lower) / 2;
       return (half / (oldMean * scale)) * 100;
     };
-    rows.push({
+    const row = {
       ci95: ciPercent(w95),
       ci99: ciPercent(w99),
       ci999: ciPercent(w999),
       improvement,
       name,
       pValue: Number.isNaN(w95.pValue) ? 1 : w95.pValue,
       stars,
-    });
+    };
+    if (maxRegression !== undefined) {
+      row.pThreshold = thresholdPValue(
+        oldRates, newHistogram, scale, maxRegression);
+    }
+    rows.push(row);
   }
 
   const adjusted = holmAdjust(rows.map(({ pValue }) => pValue));
+  const thresholdAdjusted = maxRegression === undefined ? null :
+    holmAdjust(rows.map(({ pThreshold }) => pThreshold));
   let underpowered = 0;
   for (let index = 0; index < rows.length; index++) {
     const row = rows[index];
     row.pAdjusted = adjusted[index];
+    if (thresholdAdjusted !== null) {
+      row.pThresholdAdjusted = thresholdAdjusted[index];
+    }
     row.inconclusive = maxRegression > 0 &&
                        row.stars.trim() === '' &&
                        row.ci95 > maxRegression;
@@ -146,8 +167,13 @@ function analyzeCompare(samples, scale, maxRegression) {
     `After Holm-Bonferroni correction across ${rows.length} comparison` +
     `${rows.length === 1 ? '' : 's'}, ${significant} remain` +
     `${significant === 1 ? 's' : ''} significant at 5%.`,
-    '--max-regression uses the corrected values.',
   );
+  if (maxRegression !== undefined) {
+    output.push(
+      `For --max-regression, one-sided p-values against the ` +
+      `${maxRegression}% threshold were corrected separately.`,
+    );
+  }
 
   if (maxRegression > 0 && underpowered > 0) {
     output.push('');
@@ -159,21 +185,22 @@ function analyzeCompare(samples, scale, maxRegression) {
     );
   }
 
-  const failures = maxRegression > 0 ?
+  const failures = maxRegression !== undefined ?
     rows.filter((row) => isRegressionFailure(row, maxRegression)) : [];
   if (failures.length > 0) {
     output.push('');
     output.push(
       `FAIL: ${failures.length} benchmark${failures.length === 1 ? '' : 's'}` +
       ` regressed by more than ${maxRegression}% (the 95% interval excludes ` +
-      `the threshold and significance is family-wise corrected across ` +
+      `the threshold and its one-sided test is family-wise corrected across ` +
       `${rows.length} comparisons):`,
     );
     for (const failure of failures) {
       output.push(
         `  ${failure.name}  ${failure.improvement.toFixed(2)}% ` +
         `(95% CI up to ${(failure.improvement + failure.ci95).toFixed(2)}%, ` +
-        `adjusted p=${failure.pAdjusted.toExponential(2)})`,
+        `adjusted threshold p=` +
+        `${failure.pThresholdAdjusted.toExponential(2)})`,
       );
     }
   }

benchmark/compare-node-bench.js

@@ -36,9 +36,10 @@ async function main() {
   const runs = parseInteger(cli.optional.runs, 30, '--runs', 1);
   const warmup = parseInteger(cli.optional.warmup, 0, '--warmup', 0);
   const scale = parseInteger(cli.optional.scale, 1000, '--scale', 1);
+  const hasMaxRegression = cli.optional['max-regression'] !== undefined;
   const maxRegression = parseNumber(
     cli.optional['max-regression'], 0, '--max-regression', 0);
-  const analyze = !!cli.optional.analyze || maxRegression > 0;
+  const analyze = !!cli.optional.analyze || hasMaxRegression;
   const options = {
     namePattern: cli.optional['name-pattern'],
     nodeArgs: cli.optional['node-arg'],
@@ -96,7 +97,8 @@ async function main() {
   }
 
   if (analyze) {
-    const result = analyzeCompare(rows, scale, maxRegression);
+    const result = analyzeCompare(
+      rows, scale, hasMaxRegression ? maxRegression : undefined);
     process.stdout.write(result.output);
     if (result.failed) process.exitCode = 1;
     return;

doc/api/bench.md

@@ -349,6 +349,11 @@ samples, but their samples are discarded. An exception, rejection, timeout,
 abort, missing timing call, or duplicate timing call stops the current
 benchmark. Later benchmarks continue to run.
 
+After a timeout or abort, the runner briefly waits for asynchronous benchmark
+work to settle before continuing. If it remains pending, all later benchmarks
+that were selected to run fail without running so that their measurements
+cannot overlap with that work.
+
 For each warmup and measured callback, the runner subscribes to the configured
 diagnostics channels. Each publication queues a context diagnostic whose
 `message` is `{ name, message }`, containing the string channel name and the

doc/contributing/writing-and-running-benchmarks.md

@@ -721,8 +721,9 @@ Both parallel tools support inline analysis. `scatter-node-bench.js --analyze`
 uses the same `--xaxis`, `--category`, and `--no-chart` interface described for
 `scatter.js`. `compare-node-bench.js --analyze` performs Welch's t-test, while
 `--max-regression N` adds a corrected regression gate. The gate requires both a
-Holm-Bonferroni-adjusted p-value below 0.05 and a 95% confidence interval lying
-entirely beyond `-N%`; the point estimate alone cannot fail the command.
+Holm-Bonferroni-adjusted one-sided p-value against the `N%` threshold below 0.05
+and a 95% confidence interval lying entirely beyond `-N%`; the point estimate
+alone cannot fail the command.
 Scatter analysis reduces aggregated configurations to one value per outer
 process and uses disjoint process sets for consecutive Mann-Whitney comparisons
 so configurations sharing a process are not treated as independent samples.

lib/internal/bench_runner/harness.js

@@ -64,6 +64,7 @@ const {
 } = require('internal/bench_runner/benchmarks_stream');
 
 const { bigint: hrtime } = process.hrtime;
+const kCancellationGracePeriod = 100;
 const kHookNames = ['after', 'afterEach', 'before', 'beforeEach'];
 const kIsCliRunner = getOptionValue('--bench');
 let nextRunId = 0;
@@ -76,6 +77,20 @@ function eventLoopTurn() {
   return new Promise((resolve) => setImmediate(resolve));
 }
 
+async function settlesPromptly(work) {
+  const timeout = PromiseWithResolvers();
+  const timer = setTimeout(
+    () => timeout.resolve(false), kCancellationGracePeriod);
+  try {
+    return await SafePromiseRace([
+      PromisePrototypeThen(work, () => true, () => true),
+      timeout.promise,
+    ]);
+  } finally {
+    clearTimeout(timer);
+  }
+}
+
 function createAbortError(signal) {
   return new AbortError(undefined, { __proto__: null, cause: signal.reason });
 }
@@ -86,6 +101,7 @@ function createTimeoutError(benchmark) {
 }
 
 class Harness {
+  #abortReason = null;
   #autoRun;
   #buildPromises = [];
   #duplicateErrors = new SafeMap();
@@ -581,6 +597,14 @@ class Harness {
   }
 
   async #executeSuite(suite) {
+    if (this.#abortReason !== null) {
+      await this.#completeSubtree(suite, this.#abortReason);
+      suite.finished = true;
+      suite.completion.resolve();
+      if (!suite.isRoot) suite.emitDestroy();
+      return;
+    }
+
     if (suite.buildError !== null) {
       await this.#diagnostic(suite.buildError, suite.loc, 'error', suite);
       await this.#completeSubtree(suite, suite.buildError);
@@ -614,7 +638,7 @@ class Harness {
       }
     }
 
-    if (active) {
+    if (active && this.#abortReason === null) {
       const failure = await this.#runSuiteHooks(suite, 'after');
       if (failure !== null) {
         await this.#diagnostic(
@@ -711,6 +735,13 @@ class Harness {
       } catch {
         // Preserve the error that stopped benchmark execution.
       }
+      if (!await settlesPromptly(work) && this.#abortReason === null) {
+        this.#abortReason = new AbortError(
+          'The benchmark run was aborted because asynchronous work did not ' +
+          'settle after cancellation',
+          { __proto__: null, cause: error },
+        );
+      }
       throw error;
     } finally {
       if (timer !== undefined) clearTimeout(timer);
@@ -833,6 +864,7 @@ class Harness {
       return;
     }
 
+    if (this.#abortReason !== null) forcedError = this.#abortReason;
     if (forcedError !== undefined) {
       this.success = false;
       this.counts.failed++;

test/parallel/test-bench-errors.js

@@ -27,14 +27,21 @@ bench('invalid operations', options, (b) => {
 bench('throws', options, () => {
   throw new Error('benchmark failure');
 });
-bench('timeout', { samples: 1, timeout: 10 }, async () => {
-  await new Promise(() => {});
-});
+let lateTimeoutActive = false;
 bench('late timeout', { samples: 1, timeout: 5 }, async (b) => {
-  b.start();
-  await setTimeout(30);
-  b.end(1);
+  lateTimeoutActive = true;
+  try {
+    b.start();
+    await setTimeout(30);
+    b.end(1);
+  } finally {
+    lateTimeoutActive = false;
+  }
 });
+bench('after late timeout', options, common.mustCall((b) => {
+  assert.strictEqual(lateTimeoutActive, false);
+  complete(b);
+}));
 
 const signal = AbortSignal.abort(new Error('stop'));
 bench('aborted', { samples: 1, signal }, () => {});
@@ -48,6 +55,11 @@ function complete(b) {
 bench('duplicate', { samples: 1, params: { value: 1 } }, complete);
 bench('duplicate', { samples: 1, params: { value: 1 } }, complete);
 bench('continues', options, complete);
+bench('timeout', { samples: 1, timeout: 10 }, async () => {
+  await new Promise(() => {});
+});
+bench('after unsettled timeout', options, common.mustNotCall());
+bench.skip('skipped after unsettled timeout', options, common.mustNotCall());
 
 const completions = [];
 const sampleNames = [];
@@ -57,13 +69,13 @@ stream.on('bench:complete', (result) => completions.push(result));
 stream.on('bench:sample', (sample) => sampleNames.push(sample.name));
 stream.on('bench:summary', (result) => { summary = result; });
 stream.on('end', common.mustCall(() => {
-  assert.strictEqual(completions.length, 13);
+  assert.strictEqual(completions.length, 16);
   assert.deepStrictEqual(summary.counts, {
     __proto__: null,
-    completed: 2,
-    failed: 11,
-    skipped: 0,
-    total: 13,
+    completed: 3,
+    failed: 12,
+    skipped: 1,
+    total: 16,
   });
   assert.strictEqual(summary.success, false);
 
@@ -92,12 +104,18 @@ stream.on('end', common.mustCall(() => {
                      'ERR_OPERATION_FAILED');
   assert.strictEqual(byName.get('late timeout')[0].error.code,
                      'ERR_OPERATION_FAILED');
+  assert.strictEqual(byName.get('after late timeout')[0].error, undefined);
   assert.strictEqual(byName.get('aborted')[0].error.code, 'ABORT_ERR');
 
   const duplicates = byName.get('duplicate');
   assert.strictEqual(duplicates[0].error, undefined);
   assert.match(duplicates[1].error.message, /duplicate benchmark identity/);
   assert.strictEqual(byName.get('continues')[0].error, undefined);
+  const unsettled = byName.get('after unsettled timeout')[0].error;
+  assert.strictEqual(unsettled.code, 'ABORT_ERR');
+  assert.strictEqual(unsettled.cause.code, 'ERR_OPERATION_FAILED');
+  assert.strictEqual(
+    byName.get('skipped after unsettled timeout')[0].skip, true);
   setTimeout(40).then(common.mustCall(() => {
     assert.strictEqual(sampleNames.includes('late timeout'), false);
   }));