fix(bench): make builtin work dominate each builtins A/B sample - #6117
Conversation
Two sources of noise hid the vendored builtins' cost: - `$(seq 200)` in the loop spawned an external process every sample; the loop now counts with `while`/`[`/`$(( ))` only. - Every Shell.run pays a fixed ~90ms (on this host) for the descendant baseline scan of the whole process session, on 01511ff and dev alike (sampled: all time in DescendantBaseline::capture -> processes_in_session). An empty `:` costs 90ms on both sides. Looping 10000 times makes the builtin work 200-450ms per sample. Confidence: high Scope-risk: narrow Tested: builtins.ts --json --strict (all 8 measured, 289-552ms per sample); native-bench-ab.test.ts
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e19a9a1502
ℹ️ 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".
|
|
||
| try { | ||
| await runAbSuite("builtins", cases, 10); | ||
| await runAbSuite("builtins", cases, 5); |
There was a problem hiding this comment.
When the builtins suite is launched through scripts/native-bench-ab.ts without an explicit iteration override—the normal path in native-bench-ab.yml—the runner still defaults to 200 and always forwards --iterations 200, so this new default of 5 is never used. With each sample now taking roughly 289–552 ms, one 15-block A/B round performs 30 adapter invocations of 8 × 203 calls and cannot fit within the workflow's 120-minute limit (and individual adapters may hit the runner's 10-minute timeout). The suite-specific reduced count therefore needs to be honored by the runner, rather than only by direct adapter execution.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head e19a9a1, gajae-reviewer on behalf of probepark)
CI: gate pending: everything that ran passed except Merge approval bootstrap, which waits on the needs-human verdict. Affected path validation / install-methods is still pending.
Scope: +10 / -6, 1 file, packages/natives/bench/builtins.ts (bench adapter only).
Conventions: no changelog fragment (bench-only change, not user-facing). No generated files. No labels. The diff digest matches the PR body (865ff97b…).
Notable:
packages/natives/bench/builtins.ts:106:runAbSuite("builtins", cases, 5)sets only the adapter's own default.scripts/native-bench-ab.ts:10setsDEFAULT_ITERATIONS = 200, andrunBenchAdapter(scripts/native-bench-ab.ts:316-325) always forwards--iterations <n>..github/workflows/native-bench-ab.yml:112takes that path whenever theiterationsinput is empty. So the default of 5 never applies in the A/B run. (The Codex P1 comment reports the same thing; I confirmed it at this head.)packages/natives/bench/builtins.ts:22setsLOOP = 10000. By the PR's own numbers (289–552 ms per sample), one adapter invocation on the runner path does 8 cases × (3 warmup + 200) samples, roughly 470–900 s.runJsonCommandkills the adapter after10 * 60_000ms (scripts/native-bench-ab.ts:303), and the runner reports that asAdapterError. Even when an invocation fits, one 15-block round is 30 invocations, which is several hours againsttimeout-minutes: 120(native-bench-ab.yml:29). The INCONCLUSIVE retry doubles the block count, making it worse. Before this change a sample was about 90 ms plus 200 builtins, so the suite fit easily. This change makes the defaultbuiltinsA/B dispatch unrunnable.- Checked and clean: the
while [ "$__builtins_ab_i" -lt 10000 ]counter uses only builtins and does not collide with the case bodies (B08 usesn/sum).expectedStdout.repeat(LOOP)still matches, andtimeoutMs: 30_000perShell.runleaves headroom over the ~0.5 s samples.
Blocking: item 1 and item 2 (one defect). The reduced sample count has to reach the runner. Options: a per-suite iterations in SUITES that runBenchAdapter uses when no --iterations is given, or omitting --iterations unless the user passed it. Either way, the default builtins dispatch should fit under the 10-minute adapter timeout and the 120-minute job.
Body verdict line is owned by pending; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:865ff97bf0974cf41349083b4425fe642fc2e59ff12b9a742800e895356cb5df reviewer:critic reviewer-id:gajae-reviewer evidence:runner-forces-iterations-200;loop10000-exceeds-adapter-10min-timeout;digest-verified
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:865ff97bf0974cf41349083b4425fe642fc2e59ff12b9a742800e895356cb5df reviewer:critic reviewer-id:gajae-reviewer evidence:runner-forces-iterations-200;loop10000-exceeds-adapter-10min-timeout;digest-verified
… runner probepark (#6117 at e19a9a1): the adapter's default of 5 never applied because native-bench-ab.ts always forwards --iterations (DEFAULT_ITERATIONS 200). With the 10,000-iteration loop that is ~8-15 min per adapter call, over the 10-minute adapter timeout and the 120-minute job. Add a per-suite defaultIterations (builtins: 5) used when --iterations is not given; an explicit value still wins. ~35 s per invocation, ~18 min per 15-block round.
|
Fixed the blocker at e19a9a1 in c06dd9b. The adapter's own default of 5 was never reaching the runner, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c06dd9b6b1
ℹ️ 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".
| builtins: { adapter: "packages/natives/bench/builtins.ts", actualSuite: "builtins", cases: ["B01", "B02", "B03", "B04", "B05", "B06", "B07", "B08"] }, | ||
| // Each builtins sample loops the builtin 10,000 times (~0.3-0.55 s), so 200 | ||
| // samples per case would blow the 10-minute adapter timeout and the job budget. | ||
| builtins: { adapter: "packages/natives/bench/builtins.ts", actualSuite: "builtins", cases: ["B01", "B02", "B03", "B04", "B05", "B06", "B07", "B08"], defaultIterations: 5 }, |
There was a problem hiding this comment.
With the normal builtins invocation, this reduces every block to five timings, while summarizeBlock() still computes quantile(samples, 0.95). For five values that is always the block maximum; across blocks, the median of those maxima corresponds to roughly the 87th percentile, so a regression confined to the slowest 5–10% of samples will usually be absent from most blocks and can pass the advertised p95 gate. Keep enough per-block observations to estimate p95, or change the aggregation/gate explicitly rather than applying the five-sample default.
Useful? React with 👍 / 👎.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR replaces the builtins benchmark's external seq iteration with an in-shell loop, increases repetitions to 10,000, and sets a builtins-specific default of five samples per block. The changed default is scoped to this suite and explicit --iterations still overrides it. I found no verified merge-blocking defects at reviewed head c06dd9b6b10f5867e1e450707b6d31405e4765f5.
Findings / Required Changes
No blocking or actionable findings.
Non-blocking Observations
- The loop controller calls
[once per iteration, including for B03, which itself benchmarkstestand[. Because the complete command is timed, B03 includes controller work; differences in the target builtin can therefore influence both the fixture and loop-control time. This is limited to B03's attribution and does not invalidate the entire suite. Consider isolating that controller cost while preserving supported-shell compatibility. - With five samples per block, the block p95 is the maximum observation and feeds the latency verdict. That is a lower-resolution, more maximum-sensitive estimate than the former 200-sample default; no applicable minimum sample-count contract was identified, so this is a methodology consideration rather than a merge blocker.
CI / Verification
At the reviewed head, the affected-path test for scripts/native-bench-ab.test.ts and the affected-path/plan validation checks succeeded. The check-run set contained 25 successes and four skipped checks; PR contract bootstrap failed because authorized merge-verdict evidence was unavailable, not because benchmark code or tests failed. The native benchmark workflow is manual and no result artifact bound to this head was present, so I could not independently validate the reported benchmark measurements.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Suite-specific default and explicit override agree with runner behavior; no contract requiring 200 samples for every suite. |
| A2 — Architecture / Correctness / Failure | APPROVED |
No verified runtime/failure defect; five-sample p95 sensitivity is noted above as non-blocking. |
| A3 — Security / Privacy / Trust | APPROVED |
No new trust boundary, attacker-controlled execution path, or sensitive-data flow identified. |
| A4 — Verification / Tests / CI | APPROVED |
Added default/override test passes in affected-path CI; adapter behavior has no retained head-bound benchmark artifact. |
| A5 — Context / Compatibility / Platform | APPROVED |
Runner and macOS workflow honor the per-suite default; B03 loop-control overlap is a non-blocking attribution concern. |
Limitations
The benchmark itself was not independently run during this review; measurement-related conclusions are based on the exact-head/base source and CI metadata.
|
@probepark snowykr has approved the current head c06dd9b, and CI is green apart from the approval gate. The only thing blocking merge is your CHANGES_REQUESTED on e19a9a1. Its blocker (the runner forcing |
Review of #6117: with 5 samples per block, quantile(samples, 0.95) is the block maximum, so the p95 gate would miss a regression confined to the slowest samples. The builtins suite now defaults to 20 samples per block and halves the in-command loop to 5000 iterations so one A/B round keeps about the same wall time; builtin work (~100-230ms) still dominates the ~90ms Shell.run floor. The SUITES doc states the >=20 floor. Confidence: high Scope-risk: narrow Tested: native-bench-ab.test.ts (13 pass); builtins.ts --json --strict (all 8 measured, 160-325ms per sample)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61c1fea5d
ℹ️ 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".
| builtins: { adapter: "packages/natives/bench/builtins.ts", actualSuite: "builtins", cases: ["B01", "B02", "B03", "B04", "B05", "B06", "B07", "B08"] }, | ||
| // Each builtins sample loops the builtin 10,000 times (~0.3-0.55 s), so 200 | ||
| // samples per case would blow the 10-minute adapter timeout and the job budget. | ||
| builtins: { adapter: "packages/natives/bench/builtins.ts", actualSuite: "builtins", cases: ["B01", "B02", "B03", "B04", "B05", "B06", "B07", "B08"], defaultIterations: 20 }, |
There was a problem hiding this comment.
When a builtins comparison remains INCONCLUSIVE through the first two rounds, the retry loop at scripts/native-bench-ab.ts:648-655 runs 15 + 30 + 60 blocks, or 210 base/head adapter invocations. With this default, each invocation performs 8 × (3 warmups + 20 samples); at the commit's reported 160–325 ms per sample, the three rounds require roughly 103–209 minutes before setup/build overhead, while .github/workflows/native-bench-ab.yml:29 kills the job after 120 minutes. Thus the third-round path can never produce its intended verdict for normal measured timings; use a suite-specific block/retry budget or extend the workflow deadline.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head d61c1fe, gajae-reviewer on behalf of probepark)
CI: green — 29 pass, 6 skipped (opt-in/Windows lanes), only Merge approval bootstrap failing (gate waiting on the verdict). Approve gate ALLOW; plan covered check:@gajae-code/natives.
Scope: +33 / -9, 3 files — packages/natives/bench (builtins adapter), scripts/native-bench-ab.ts + its test
Conventions: CHANGELOG none (bench tooling only), generated files none, labels none
Notable:
- Delta since the last reviewed head
c06dd9b:0d5f358(LOOP 10000→5000, builtinsdefaultIterations5→20 in bothscripts/native-bench-ab.ts:86andpackages/natives/bench/builtins.ts:106, test expectation updated) plus a merge oforigin/dev(#6042 files only; they are not in thebase...headdiff). The earlier blocker frome19a9a1(suite default never reaching the runner) stays fixed:parseNativeBenchOptionsresolvesiterations ??= SUITES[suite].defaultIterations ?? DEFAULT_ITERATIONS(scripts/native-bench-ab.ts:131) before the positive-integer check, andrunBenchAdapterpasses--iterationsexplicitly (:336). Budget: 8 cases x 20 samples x ~0.2-0.32 s ≈ 30-50 s per adapter call, well inside the 10-minuterunJsonCommandtimeout (:315). scripts/native-bench-ab.ts:84— the comment still says "loops the builtin 10,000 times (~0.3-0.55 s)", butLOOPis now 5000 (~100-230 ms of builtin work perbuiltins.ts:17). Stale comment only; non-blocking.
Blocking: none
Body verdict line is owned by pending; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:d11dd9f996ab82f61e6f076070b28ce36e7fc20077911d01f035809ec1f0930b reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;suite-default-iterations-reach-runner;adapter-budget-checked
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:d11dd9f996ab82f61e6f076070b28ce36e7fc20077911d01f035809ec1f0930b reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;suite-default-iterations-reach-runner;adapter-budget-checked
A-PI-BUILTINS-BASE, C-PTY, C-POWER, C-APPEARANCE, C-PROF, B-CRASH-HANDLER and C-ISO were held in-progress because their listed benchmarks did not reach their native code. The surface suites from #6062 (with #6069, #6089 and #6117 fixing what they found) now PASS against 01511ff: pty, power, appearance, prof, crash, iso and builtins, the last with byte-identical stdout for eight builtins as the per-builtin parity evidence. A tag-build-verify rehearsal on dev 35f2808 (run 36538316951) is green. Confidence: high Scope-risk: narrow Tested: verify-rust-porting-inventory (schema valid, adopted-row checks); verify-rust-porting-inventory.test.ts
3 participants
What
builtinsA/B loop now counts using only builtins (while/[/$(( ))) instead of$(seq …).Why
The suite was still INCONCLUSIVE everywhere with equal medians, locally and on the quiet runner (run 36513103797). Two things hid the builtins' cost:
$(seq 200)added noise. It spawned an external process on every sample, which put fork/exec variance into each measurement.Shell.runhas a fixed ~90 ms floor. On this host, a:command costs about 90 ms on01511ffd5fand on dev alike. Asampleprofile puts essentially all of that time inpi_shell::process::DescendantBaseline::capture→processes_in_session, a scan of the whole process session per command. This is pre-existing, not a regression. It does swamp a microsecond builtin, though.At 5000 iterations the builtin work is about 100–230 ms per sample, so the measurement reflects the vendored builtins.
Testing
bun packages/natives/bench/builtins.ts --json --strict: all 8 cases measured, at 160–325 ms per sample.bun test scripts/native-bench-ab.test.tspasses.Risk classification
low-riskregression-riskhigh-riskGJC verdict
devpackages/<pkg>/changelog.d/(if user-facing)