Consolidate benchmark config into a single generated registry - #33
Conversation
Replace three hand-maintained copies of benchmark config (backend configs.js, frontend BenchmarkRunner.js, frontend machineProfiles.js) with one canonical shared/benchmark-registry.json plus a generator script that emits backend/benchmarks/configs.generated.js (CJS) and src/generated/benchmarkRegistry.js (ESM). Backend and frontend now import the generated data instead of hand-rolled arrays/switches, webgpu_dispatch* configs are present on both sides, and `npm run config:check` (wired into CI) fails the build on drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014jeFBxMgkwAb3pZ5GHwZQA
📝 WalkthroughWalkthroughChangesBenchmark configuration centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/config-check.yml:
- Line 13: Update the actions/checkout@v4 step to set persist-credentials to
false, preventing the job from storing the GITHUB_TOKEN in local Git
configuration.
In `@backend/benchmarks/configs.js`:
- Around line 19-23: Update runConfig’s result dispatch so webgpu_dispatch uses
the dispatch-overhead metrics path rather than the generic GPU/WebGL labeling
path; keep webgpu_compute on its existing handling and preserve current behavior
for other configurations.
In `@shared/benchmark-registry.json`:
- Around line 79-84: Update the GPU-only selection logic in BenchmarkRunner to
identify configurations by compilation.family === "gpu" rather than requiring
IDs to include "compute". Ensure webgpu_dispatch is included while non-GPU
configurations remain excluded.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 331aca81-f13f-436f-bfb7-a0516afefb61
⛔ Files ignored due to path filters (2)
backend/benchmarks/configs.generated.jsis excluded by!**/*.generated.*src/generated/benchmarkRegistry.jsis excluded by!**/generated/**
📒 Files selected for processing (8)
.github/workflows/config-check.ymlCLAUDE.mdbackend/benchmarks/configs.jspackage.jsonscripts/generate-benchmark-config.jsshared/benchmark-registry.jsonsrc/components/BenchmarkRunner.jssrc/utils/machineProfiles.js
| config-check: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For actions/checkout@v4, does persist-credentials default to true, and does setting persist-credentials: false prevent the GitHub token from being stored in local Git configuration?
💡 Result:
For actions/checkout@v4, the persist-credentials input defaults to true [1][2][3]. Setting persist-credentials to false does prevent the GitHub token (or SSH key) from being configured in the local Git configuration [1][4][5]. When persist-credentials is set to true (the default), the action automatically configures the local Git settings with the provided token, which allows subsequent steps in the job to run authenticated Git commands [1][4]. Setting this to false opts out of this behavior, ensuring the token is not stored in the local Git configuration [1][5].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/README.md
- 2: https://github.com/actions/checkout/blob/v4/action.yml
- 3: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 4: https://github.com/actions/checkout
- 5: Always emit
persist-credentials: falsewith actions/checkout github/gh-aw#2641
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/config-check.yml
printf '%s\n' '--- package scripts ---'
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log(JSON.stringify({ scripts: pkg.scripts }, null, 2));
JS
printf '%s\n' '--- Git-related references in the config-check path ---'
rg -n -i 'git|github|token|credential|checkout|config:check' package.json .github/workflows . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: ford442/benching_machine
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/config-check.yml
printf '%s\n' '--- config:check script ---'
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
console.log(pkg.scripts && pkg.scripts['config:check']);
JS
printf '%s\n' '--- files referenced by config:check ---'
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
const script = pkg.scripts && pkg.scripts['config:check'];
console.log(script || '(missing)');
JS
rg -n --glob 'package.json' --glob '.github/workflows/config-check.yml' --glob '*.js' --glob '*.mjs' --glob '*.cjs' --glob '*.ts' --glob '*.yml' --glob '*.yaml' \
'config:check|git (clone|fetch|pull|push)|actions/checkout|GITHUB_TOKEN|github\.token|persist-credentials' .Repository: ford442/benching_machine
Length of output: 870
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- generator outline and size ---'
wc -l scripts/generate-benchmark-config.js
ast-grep outline scripts/generate-benchmark-config.js
printf '%s\n' '--- Git, token, and credential references in generator ---'
rg -n -i 'git|github|token|credential|exec|spawn|https?://|fetch\(' scripts/generate-benchmark-config.js || true
printf '%s\n' '--- generator entry points and file operations ---'
rg -n 'process\.argv|readFile|writeFile|existsSync|readdir|path\.|require\(' scripts/generate-benchmark-config.jsRepository: ford442/benching_machine
Length of output: 1811
Disable persisted Git credentials.
Set persist-credentials: false because this job does not require Git authentication. This prevents actions/checkout@v4 from storing the GITHUB_TOKEN in local Git configuration.
Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/config-check.yml at line 13, Update the
actions/checkout@v4 step to set persist-credentials to false, preventing the job
from storing the GITHUB_TOKEN in local Git configuration.
Source: Linters/SAST tools
| async function runConfig(configId) { | ||
| const m = getMultiplier(configId); | ||
| const supportsWasmThreads = ['wasm_threads','wasm_simd','wasm_max','wasm64','wasmfs'].includes(configId); | ||
| const supportsOpenMP = ['wasm_openmp', 'wasm_max'].includes(configId); | ||
| const isGPU = ['webgl_compute', 'webgpu_compute'].includes(configId); | ||
| const isGPU = ['webgl_compute', 'webgpu_compute', 'webgpu_dispatch'].includes(configId); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a dispatch-specific result path.
Line 23 classifies webgpu_dispatch as GPU. The GPU branch only handles webgpu_compute separately. The remaining GPU path reports "Matrix Mult (WebGL)". Backend results for WebGPU dispatch are therefore mislabeled. Return dispatch-overhead metrics for webgpu_dispatch instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/benchmarks/configs.js` around lines 19 - 23, Update runConfig’s
result dispatch so webgpu_dispatch uses the dispatch-overhead metrics path
rather than the generic GPU/WebGL labeling path; keep webgpu_compute on its
existing handling and preserve current behavior for other configurations.
| { "id": "webgpu_dispatch", "name": "WebGPU Dispatch Overhead", "desc": "JS ↔ GPU Binding Stress", "color": "#e67e22", | ||
| "compilation": { "family": "gpu", "toolchain": "WGSL→GPU driver", "backend": "GPU (compute shader)", "language": "WGSL", "optLevel": "driver", "flags": [], "postProcess": [], "status": "real" }, | ||
| "multiplier": 10.0 }, | ||
| { "id": "webgpu_dispatch_wasm", "name": "WebGPU Dispatch (WASM)", "desc": "Real C++ via Emscripten + Dawn (experimental)", "color": "#c0392b", | ||
| "compilation": { "family": "wasm", "toolchain": "emcc→Dawn glue", "backend": "Emscripten/WebGPU", "language": "C++", "optLevel": "O3", "flags": ["-O3", "-s USE_WEBGPU=1"], "postProcess": [], "status": "experimental" }, | ||
| "multiplier": 2.5 } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include webgpu_dispatch in the GPU-only run.
This new GPU configuration does not match id.includes('compute') in src/components/BenchmarkRunner.js Line 393. The GPU Benchmarks action skips it. Select GPU configurations from compilation.family === 'gpu' instead of an ID substring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/benchmark-registry.json` around lines 79 - 84, Update the GPU-only
selection logic in BenchmarkRunner to identify configurations by
compilation.family === "gpu" rather than requiring IDs to include "compute".
Ensure webgpu_dispatch is included while non-GPU configurations remain excluded.
2 participants
Summary
Benchmark configuration used to live in three hand-maintained places (
backend/benchmarks/configs.js,src/components/BenchmarkRunner.js,src/utils/machineProfiles.js), which had already drifted —webgpu_dispatch/webgpu_dispatch_wasmonly existed on the frontend, andgetMultiplier()was duplicated as an inline switch in the frontend fallback simulation.This PR introduces a single hand-edited config source and generates the rest:
shared/benchmark-registry.json— canonicalconfigurations[](with per-configmultiplier),machineProfiles, andwasmLoaderConfigs.scripts/generate-benchmark-config.js— emitsbackend/benchmarks/configs.generated.js(CommonJS) andsrc/generated/benchmarkRegistry.js(ESM) from the registry. Supports--checkfor CI drift detection.backend/benchmarks/configs.js— now only holds the mock benchmark-running logic (runConfig,runConfigsSequential,generateResult); config data is imported from the generated module.src/components/BenchmarkRunner.js— inlineconfigurations[]array and duplicated multiplierswitchremoved; both now import from the generated registry.src/utils/machineProfiles.js— re-exportsmachineProfilesfrom the generated registry instead of duplicating the object.webgpu_dispatch/webgpu_dispatch_wasmare now present in both the backend and frontend generated configs.npm run config:generate/npm run config:checkadded topackage.json;.github/workflows/config-check.ymlruns the check on push/PR to fail CI on drift.CLAUDE.mdupdated: directory structure, "How to Add a New Compilation Config" workflow, and "Common Pitfalls" now reflect the single-source-of-truth model.Test plan
npm run config:checkpasses against the committed generated filesnode backend/benchmarks/configs.jsloads correctly andrunConfigsSequentialproduces results (viabackend/server.js's existing/api/configurationsroute)node backend/cli.js liststill works (CLI benchmark path untouched)CI=true npx react-scripts buildcompiles cleanly with no new warningsnpm run web) — run/GPU benchmark buttons and machine-profile playbackGenerated by Claude Code
Summary by CodeRabbit
New Features
Documentation