fix(bench): combine the providers into one document, and run them in parallel - #207
Merged
Merged
Conversation
…parallel Three things, all from reading the last run's output. BENCHMARKS.md was the two per-provider reports concatenated, so it had two `# Benchmarks` titles and two Context blocks - the redis half read as a stray section of the memory report, and comparing one scenario across providers meant scrolling between two tables. It now builds from the per-provider JSON, with the providers as columns: speedup vs the cache-disabled baseline first, then requests per second and latency. Providers no longer run serially. That was on the theory they would compete for the same CPU, but matrix legs get a runner each and never shared one - the last run put memory on runner 1000003579 and redis on 1000003580, back to back, for no benefit and twice the wall clock. Absolute throughput is not comparable across the legs either way, which is why each scenario is measured against a baseline taken in its own job. Node version and core count are recorded into the JSON at measurement time rather than read when the document is rendered, because that now happens in a different job, where process.version is the assembler's Node. Regenerated from the artifacts of run 31762785893, so this needs no new run. Co-Authored-By: Claude <noreply@anthropic.com>
The measurements were taken on ee75158, which is the code that ships as 5.1.0. The generator reads the version out of package.json, and package.json still says 5.0.1 because release-please bumps it on merge of the release PR, not before - so the generated value named the previous release for code that is not in it. Hand-edited rather than bumped at the source: release-please owns those version fields, and editing them to make a generator agree would desync the manifest from the tags. Regenerating after the release PR merges produces 5.1.0 on its own. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All three of these came from reading the output of run 31762785893.
1. One document, not two stacked
BENCHMARKS.mdwas the two per-provider reportscat'd together, so it had two# Benchmarkstitles and two## Contextblocks. The redis half read as astray section of the memory report — which is exactly why it looked like the
memory run had "a reference to redis" in it. (It didn't: the memory half is
Provider: memorywith zero redis mentions. It was the second document.)It now builds from the per-provider JSON, with the providers as columns:
then requests/sec and latency in the same shape. Scenario rows come from the
reports themselves, so adding one to
benchmark.mjscannot silently drop itfrom the doc.
2. Providers now run in parallel
They were serialised on the theory that they'd compete for the same CPU. They
never could: matrix legs get a runner each. The last run put memory on
GitHub Actions 1000003579and redis on1000003580, back to back —— for no benefit and twice the wall clock. Absolute throughput isn't comparable
across the two legs either way, which is why each scenario is measured against a
cache-disabled baseline taken inside its own job. The workflow-level concurrency
group still prevents two runs overlapping.
3. Node version recorded at measurement time
It was read from
process.versionwhen rendering. Rendering now happens in aseparate job, so that would have reported the assembler's Node rather than the
one the measurements were taken on. Node version and core count go into the
JSON instead.
Verified
Ran the assembler against the real downloaded artifacts from run 31762785893 —
that's what the committed
BENCHMARKS.mdis. Also injected a syntheticunexpectedNon2xxto confirm the untrustworthy-results path still fires:Because the doc is regenerated from those artifacts, this supersedes #206 and
needs no new benchmark run.
Note on the version field
It reads
Plugin version: 5.0.1, which is correct — that is what is onmainat the benchmarked commit. It becomes
5.1.0once #200 merges and thebenchmarks are re-run. Worth not hand-editing: release-please owns those version
fields, and editing them would conflict with #200.
🤖 Generated with Claude Code