Skip to content

fix(ci): correct Bencher reporting and benchmark failures - #176

Merged
jerome-benoit merged 3 commits into
masterfrom
fix/bencher-reporting
Sep 12, 2026
Merged

jerome-benoit merged 3 commits into
masterfrom
fix/bencher-reporting

Conversation

@jerome-benoit

@jerome-benoit jerome-benoit commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

PR Checklist

  • Conventional Commit title and change description.
  • Construction and worker failure scenarios verified locally; no test file included in this PR.
  • Historical unit mismatch explained in this PR; README unchanged.
  • RCA: latest failure, dependency-age failure.

Does this PR introduce a breaking change?

  • Yes
  • No public library API change. Benchmark latency output is corrected from milliseconds to nanoseconds; see the historical unit warning below.

RCA

  • The latest report uploaded successfully, then failed on two legitimate threshold crossings for DynamicThreadPool / FAIR_SHARE / no queue: latency +70.5% and throughput -39.5% against their historical baselines. This does not establish which code/environment change caused the slowdown.
  • The previous three failed runs were blocked by Deno’s 24-hour dependency age policy. Bencher then obscured the original failure with a missing report error.
  • Tinybench latency statistics are milliseconds, but the Bencher latency measure is nanoseconds. The exporter also swallowed pool failures and omitted errored tasks.

Changes

  • Separate benchmark execution and publication; propagate failures and reject incomplete results.
  • Convert latency mean and bounds to nanoseconds. Keep throughput in ops/s.
  • Retain the existing self-hosted testbed. Historical latency values are milliseconds labeled as nanoseconds; without migrating that history, corrected nanosecond reports will trigger artificial latency regressions. This PR does not migrate or delete stored data. Throughput units are unchanged.
  • Keep the existing t-test thresholds and error-on-alert behavior unchanged.
  • Align Deno with latest; delay npm updates in Deno manifests by one day using strict Renovate age filtering.
  • Non-master dispatches use Bencher dry-run; remove PR commenting from this push/dispatch-only workflow.

Verification

  • Real local benchmark: 24 scenarios / 48 finite metrics; latency/throughput unit invariant checked.
  • Bencher 0.6.12 CLI accepted the generated report with the workflow thresholds in dry-run.
  • The full suite passed before removing the temporary regression file; format and lint also pass after its removal.
  • Format, lint and Renovate config validator passed.
  • Self-hosted workflow validation passed with the retained self-hosted testbed in dry-run, without altering production results.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-12T15:22:12.793073Z 455cc2a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@jerome-benoit

Copy link
Copy Markdown
Contributor Author

Self-hosted validation passed: run 34700974127. Both the original failure and the validation ran on faust with Deno 2.9.6. The validation used Bencher dry-run and did not create a report or baseline.

For the previously alerted DynamicThreadPool / FAIR_SHARE / no queue scenario:

Metric Original run Validation Original alert boundary
Latency (ms, normalized) 1.235013 0.649441 upper 1.140855
Throughput (ops/s) 995.652 1633 lower 1035.730

The slowdown did not reproduce in this execution with the same runner and runtime. This is evidence of run-to-run variability, not proof of a performance fix or identification of the external cause. The PR changes reporting/error handling, not pool scheduling, benchmark workloads, or alert thresholds.

The new testbed is required for the ms-to-ns reporting migration, not to dismiss the old alert. Historical reports remain available.

@jerome-benoit

Copy link
Copy Markdown
Contributor Author

Updated as requested: the commit no longer contains tests/benchmarks-utils.test.mjs, and BENCHER_TESTBED remains self-hosted. This supersedes the new-testbed migration described in my earlier comment. No stored Bencher data has been changed. The description and documentation now explicitly warn that the retained history uses milliseconds while corrected reports use nanoseconds; without a historical migration, this produces artificial latency alerts.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 455cc2a474

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread benchmarks/benchmarks-utils.mjs Outdated
bmfResults[task.name] = {
latency: {
// Tinybench reports milliseconds; Bencher latency uses nanoseconds.
value: task.result.latency.mean * 1e6,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reset incompatible latency history before scaling samples

When this runs against the existing master/self-hosted Bencher history, all prior latency samples were published directly in Tinybench milliseconds, while this line makes only new samples one million times larger. The production path in .github/workflows/internal-benchmark.yml continues publishing to that same branch, testbed, and measure and uses --err, so valid runs will be classified as enormous regressions and fail until the incompatible history ages out. Migrate/reset the historical latency data or publish the corrected series under a fresh testbed before enabling this conversion.

Useful? React with 👍 / 👎.

@sonarqubecloud

Copy link
Copy Markdown

@jerome-benoit
jerome-benoit merged commit a25ef9e into master Sep 12, 2026
15 checks passed
@jerome-benoit
jerome-benoit deleted the fix/bencher-reporting branch September 12, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant