bench: add plugin-graphql-long sirun benchmark#8089
Conversation
Overall package sizeSelf size: 5.67 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✨ Fix all issues with BitsAI or with Cursor
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91ed816b48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Number(process.env.WITH_DEPTH)) { | ||
| tracer.use('graphql', { depth: Number(process.env.WITH_DEPTH) }) |
There was a problem hiding this comment.
WITH_DEPTH=0 as a configured depth value
The with-depth-off variant in meta.json sets WITH_DEPTH="0", but this condition uses Number(process.env.WITH_DEPTH) so 0 is treated as falsy and tracer.use('graphql', { depth: 0 }) is never called. In this benchmark, that means the “depth-off” scenario runs with default GraphQL plugin settings instead of depth explicitly disabled, so this variant’s measurements are mislabeled and comparisons against other variants become unreliable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9294277: switched the four env-var truthy checks (WITH_TRACER, WITH_DEPTH, WITH_ASYNC_HOOKS, QUERIES) from Number(process.env.X) to plain process.env.X. WITH_DEPTH="0" now reads as "set" and the with-depth-off variant correctly calls tracer.use('graphql', { depth: 0 }) instead of falling through to plugin defaults.
e712b5c to
a1f5c1d
Compare
BenchmarksBenchmark execution time: 2026-04-30 15:22:24 Comparing candidate commit 9c18f04 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1229 metrics, 95 unstable metrics. |
The existing plugin-graphql bench runs exactly 6 queries per process, so on Node 22.10.0 master ~80% of measured CPU is one-time startup (tracer init + graphql require + schema walk). At that workload, any change that trades startup cost for steady-state cost (e.g. module rewriting, JIT-warmup-heavy instrumentation) registers as a regression even when it makes real applications faster. Add plugin-graphql-long: a sibling benchmark that runs 150 sequential queries per process using the same variant matrix. At N=150, load drops to ~12-17% of measured CPU, so per-query cost dominates -- the metric that matters for servers handling many requests per process lifetime. - QUERIES env var is parametric (defaults to 150); same directory can be re-aimed at larger sizes without forking. - Reuses the existing plugin-graphql schema module. - Picked up automatically by benchmark/sirun/runall.sh via subdir scan (no runner changes needed). - Budget: ~4 min per Node version across the 6-variant x 30-iteration matrix (roughly 5-6x the existing plugin-graphql bench). The existing plugin-graphql bench is preserved unchanged, so startup / init regressions remain covered. plugin-graphql-long complements it by catching steady-state regressions that the 6-query shape hides.
a1f5c1d to
cc9e233
Compare
| // Long-workload graphql bench. Runs QUERIES sequential queries per process | ||
| // (default 100) so the fixed startup cost doesn't dominate the measurement. | ||
| // See ./README.md. |
There was a problem hiding this comment.
I wanted to have another benchmark type as well where we can deduct the startup time. Right now, that is dominant in almost all our benchmarks and that is why they are not useful.
I asked @bengl a long time ago about implementing the type and he opened a PR in dd-trace-js (look at the oldest ones). It just needs to be tracked in our benchmark backend as a known type. That way this would also be solved while not needing to increase the runtime drastically. While having a combined view is also helpful.
There was a problem hiding this comment.
Thanks for the context — looked at @bengl's older work to understand the shape of the "deduct startup" approach. The two relevant ones I found:
- Add shimmer shimmer benchmarks #5597 — original
shimmer-startupbenchmark - cut down shimmer-startup benchmark so it can run in benchmarking platform #5613 — cut-down version so it fits in the bench platform
shimmer-startup doesn't deduct startup from another bench; it amortizes startup away by running 100,000 iterations of a trivial wrapped function so per-call cost dwarfs init. Same idea as what plugin-graphql-long does, just at a different scale because graphql's per-query work is much heavier than a single shimmer wrap (we top out at ~100 queries before per-Node CI time becomes a problem).
If you'd still rather see startup subtracted from the per-query measurement (e.g., a separate plugin-graphql-startup bench tracked alongside, with the bench backend computing plugin-graphql-long − plugin-graphql-startup), I'm happy to add that as a follow-up — let me know if that matches what you had in mind. For now this PR sticks with amortization, which I think gets us most of the way there: at QUERIES=100, startup load drops from ~80% on the 6-query bench to ~16–23% on the long bench, so per-query cost is the dominant signal even without a separate subtraction step.
Drops per-Node bench runtime from ~4 min to ~1 min by lowering QUERIES default 150 -> 100 and iterations 30 -> 10. At Q=100, startup share is ~16-23% on the typical tracer-on variants -- well below the ~77-83% of the existing 6-query bench, so per-query cost still dominates the measurement. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Almost LGTM. The envs do not need the number parsing in most situations (we just need the existence check) and the README.md should likely be removed. I don't think it is good to land that in our repo. Instead, that could be part of the commit message :)
Number(process.env.WITH_DEPTH) returns 0 when WITH_DEPTH="0", which is falsy, so the with-depth-off variant skipped tracer.use() entirely and ran with default plugin config (depth defaults, collapse=true) instead of the intended depth=0/collapse=true configuration. Variant labels and cross-variant comparisons were misleading because of this. Switch the four env-var truthy checks (WITH_TRACER, WITH_DEPTH, WITH_ASYNC_HOOKS, QUERIES) from Number(env) to plain env so the string "0" reads as "set". Number() conversion still happens at use sites that need the integer value. Addresses chatgpt-codex-connector and BridgeAR review comments. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sibling sirun benches (plugin-graphql, shimmer-runtime, encoding) keep their READMEs to a 1-3 paragraph description of what the bench does; methodology and case-study numbers go in the PR description that introduces the bench. The plugin-graphql-long README was carrying that methodology in-tree, which goes stale and sits outside its real audience. Drop the methodology tables and rationale; keep the terse description matching the other sirun READMEs. Addresses BridgeAR review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The 6-query plugin-graphql bench reports startup-dominated numbers (load share ~80%) that consistently mislabel per-query trade-offs as regressions, which is exactly the problem this directory exists to solve. Keeping it alongside the long bench bloats CI runtime and preserves a misleading signal that reviewers were already discounting. - Remove benchmark/sirun/plugin-graphql/ entirely (index.js, meta.json, README.md). Schema moves into plugin-graphql-long/ since it's the only remaining consumer. - Drop iterations from 10 to 5 in plugin-graphql-long/meta.json. With ~3s per iteration on the heaviest variant and ~3 of those discarded as warmup, 5 leaves 2 measurement points which is on the thin side but acceptable given sirun reports stddev and the bench infra aggregates across runs. - Update plugin-graphql-long/index.js schema require to ./schema. After this: only scenario:plugin-graphql-long-* runs on CI; the scenario:plugin-graphql-* scenarios stop reporting. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Resolves the mergegate "files not properly owned" failure for the new benchmark/sirun/plugin-graphql-long/ files. The graphql plugin itself is owned by apm-idm-js via the /packages/datadog-plugin-*/ wildcard; the bench that exercises it gets the same owner. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Long-workload graphql bench. Runs QUERIES sequential queries per process (default 100) so the fixed startup + plugin-setup cost (tracer init, orchestrion rewrite of graphql/execution/execute.js, schema parse, instrumentation one-time work) is dominated by per-query work. The query and variants mirror the previous plugin-graphql bench exactly; only the loop size differs (default 100 vs the old 6). QUERIES is parametric so the same bench can be re-aimed at larger sizes (e.g. QUERIES=500) without forking the directory. Removes the in-tree README per review — context belongs in the commit message and PR description rather than alongside the bench files. Also simplifies the QUERIES line: process.env.QUERIES || 100. The for loop coerces the string in i < queries, so the explicit Number() was redundant once the existence check moved to a plain truthy guard. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The existing plugin-graphql bench runs exactly 6 queries per process, so on Node 22.10.0 master ~80% of measured CPU is one-time startup (tracer init + graphql require + schema walk). At that workload, any change that trades startup cost for steady-state cost (e.g. module rewriting, JIT-warmup-heavy instrumentation) registers as a regression even when it makes real applications faster. Add plugin-graphql-long: a sibling benchmark that runs 100 sequential queries per process using the same variant matrix. At N=100, load drops to ~16-23% of measured CPU, so per-query cost dominates -- the metric that matters for servers handling many requests per process lifetime. - QUERIES env var is parametric (defaults to 100); same directory can be re-aimed at larger sizes without forking. - Reuses the existing plugin-graphql schema module. - Picked up automatically by benchmark/sirun/runall.sh via subdir scan (no runner changes needed). - Budget: ~1 min per Node version across the 6-variant x 30-iteration matrix. The existing plugin-graphql bench is preserved unchanged, so startup / init regressions remain covered. plugin-graphql-long complements it by catching steady-state regressions that the 6-query shape hides.
The existing plugin-graphql bench runs exactly 6 queries per process, so on Node 22.10.0 master ~80% of measured CPU is one-time startup (tracer init + graphql require + schema walk). At that workload, any change that trades startup cost for steady-state cost (e.g. module rewriting, JIT-warmup-heavy instrumentation) registers as a regression even when it makes real applications faster. Add plugin-graphql-long: a sibling benchmark that runs 100 sequential queries per process using the same variant matrix. At N=100, load drops to ~16-23% of measured CPU, so per-query cost dominates -- the metric that matters for servers handling many requests per process lifetime. - QUERIES env var is parametric (defaults to 100); same directory can be re-aimed at larger sizes without forking. - Reuses the existing plugin-graphql schema module. - Picked up automatically by benchmark/sirun/runall.sh via subdir scan (no runner changes needed). - Budget: ~1 min per Node version across the 6-variant x 30-iteration matrix. The existing plugin-graphql bench is preserved unchanged, so startup / init regressions remain covered. plugin-graphql-long complements it by catching steady-state regressions that the 6-query shape hides.
2 participants
Summary
Replaces
benchmark/sirun/plugin-graphql/(6-query, startup-dominated) withbenchmark/sirun/plugin-graphql-long/(100 queries per process), so per-query cost dominates the measurement rather than one-time setup cost.Motivated by #7757 (graphql plugin refactor), where the 6-query bench reports a regression that inverts into a large improvement under production-like workloads.
Why
The existing
plugin-graphqlbench fired 6 queries per process then exited. At 6 queries, load was ~80% of the CPU budget — any change that traded startup cost for steady-state cost looked like a regression on this benchmark even when it made real applications faster. Keeping the 6-query bench preserved a misleading signal that reviewers had already learned to discount, so this PR removes it outright rather than running both.Case study: #7757 — master vs PR branch
Same sirun setup, same variants, two different process sizes (Node 22.10.0,
user.timemetric, median):with-depth-off+4.2%−25.9%with-depth-on-max+5.1%−27.2%with-depth-and-collapse-off+11.6%−3.3%with-depth-and-collapse-on+4.2%−26.3%Break-even: ~15–20 queries per process. Past that, #7757 is strictly faster. The 6-query bench was reporting only the 🟥 column.
Why load share matters
with-depth-on-max83%23%−60ppwith-depth-and-collapse-off77%16%−61ppAt
QUERIES=100, load drops from 🟥 ~80% to 🟩 ~16–23% — per-query cost dominates.QUERIESis parametric so the same directory can be re-aimed at larger sizes (e.g.QUERIES=500for load <6%) without forking.What's in the change
benchmark/sirun/plugin-graphql-long/:index.js— parametric onQUERIES(default 100); env-var truthy checks useprocess.env.XsoWITH_DEPTH=0reads as configured (Codex review fix).meta.json— same six variants as the old bench,iterations: 5.schema.js— moved from the oldplugin-graphql/location since it's the only consumer.README.md— short sibling-style description.benchmark/sirun/plugin-graphql/entirely.CI impact
benchmark/sirun/runall.shauto-discovers subdirs (DIRS=($(ls -d */ | sed 's:/$::'))) and runs the 6 variants in parallel as backgrounded jobs pinned to separate cores ((time node ../run-one-variant.js …) &). Wall time per Node version equals the slowest variant, not the sum.After this PR:
scenario:plugin-graphql-*— removed.scenario:plugin-graphql-long-*— new steady-state signal.plugin-graphql(6q × 30 iters)with-depth-and-collapse-offplugin-graphql-long(100q × 5 iters)with-depth-and-collapse-offPer iteration the long bench is ~3 s for the slowest variant; 5 iterations × 1 process per iteration ≈ 15 s on the busy core. Light variants (control, async-hooks) finish in ~3 s and idle.
Test plan
sirun meta-temp.jsonon Node 22.10.0 / Node 25 — all 6 variants run.QUERIES=100keeps load 🟩 ≤23% across all variants (worst case:with-depth-on-max).iterations=5chosen as the smallest count where stddev/mean stays acceptable post-warmup; confirmed in local sirun output.Follow-up on #7757: once this lands, #7757 will rebase to inherit the bench from master, and CI will produce comparable
plugin-graphql-long-*numbers against the master baseline.🤖 Generated with Claude Code