perf: establish truthful performance baseline (Phase 1) - #28
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds enqueue performance guards, an open-loop scenario harness, workload models, reports, benchmark baselines, CI workflows, and performance planning documentation. Go Actions versions and coverage checks are also updated. ChangesPerformance benchmarking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant MakeGuards
participant Batcher
participant ScenarioMatrix
participant Report
CI->>MakeGuards: Run race and allocation guards
MakeGuards->>Batcher: Execute tests and enqueue benchmarks
CI->>ScenarioMatrix: Run scheduled scenario matrix
ScenarioMatrix->>Batcher: Submit scheduled workload
ScenarioMatrix->>Report: Write environment and metric results
Report-->>CI: Upload benchmark artifacts
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (4)
Makefile (2)
30-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
-shortfor the race lane.
go test -race ./...includestest/scenario.TestSmallWindowGivesNoOverloadProtectionoffers 500,000 items/s andTestReproducesInlineSlowProcessorInversionruns 1.5s windows twice. Both tests skip only whentesting.Short()is set. Under the race detector the memory and CPU cost multiplies, so the blocking guard becomes slow and OOM-prone on shared runners.♻️ Proposed change
guards-race: `@echo` "Running race detector..." - `@go` test -race ./... + `@go` test -race -short ./...🤖 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 `@Makefile` around lines 30 - 32, Update the guards-race target to pass Go’s short-test flag when running the race detector, so scenario tests that honor testing.Short() are skipped while standard race coverage remains enabled.
26-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the new Make targets as
.PHONY. Addguards,guards-race,guards-allocs,bench-enqueue, andbench-matrixto the existing declaration so matching files cannot suppress their recipes.🤖 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 `@Makefile` around lines 26 - 46, Add guards, guards-race, guards-allocs, bench-enqueue, and bench-matrix to the existing .PHONY declaration in the Makefile so filesystem entries with those names cannot suppress their recipes.Source: Linters/SAST tools
test/scenario/matrix_test.go (1)
87-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
strconvinstead of a hand-rolleditoa.The standard library already formats integers. The recursive version also loses precision:
itoa(1_500)returns"1k".formatCountinpkg/batcher/batcher_bench_test.gosolves the same problem withstrconv.Itoa.♻️ Proposed refactor
-func itoa(v int) string { - if v >= 1_000 { - return itoa(v/1_000) + "k" - } - - digits := "" - for v > 0 { - digits = string(rune('0'+v%10)) + digits - v /= 10 - } - - if digits == "" { - return "0" - } - - return digits -} +func itoa(v int) string { + if v >= 1_000 && v%1_000 == 0 { + return strconv.Itoa(v/1_000) + "k" + } + + return strconv.Itoa(v) +}Add
"strconv"to the import block.🤖 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 `@test/scenario/matrix_test.go` around lines 87 - 103, Replace the hand-rolled itoa function with strconv.Itoa in the relevant test formatting flow, adding the strconv import and removing the recursive digit-building implementation. Ensure values such as 1,500 retain their full decimal representation rather than being abbreviated or losing precision.docs/improvements/plan-perf.md (1)
156-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to the fenced blocks.
Markdownlint reports MD040 for these fences. Add an identifier such as
text,go, orshto each block.Example
-``` +```textAlso applies to: 200-200, 221-221, 276-276, 313-313, 367-367, 1100-1100
🤖 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 `@docs/improvements/plan-perf.md` at line 156, Add language identifiers to every fenced code block in plan-perf.md, including the blocks at the referenced locations, using the appropriate identifier such as text, go, or sh so all Markdown fences satisfy MD040.Source: Linters/SAST tools
🤖 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/go.yml:
- Line 69: Update the coverage exclusion regex used by
gwatts/go-coverage-action@v2 from \test\/.*$ to (^|/)test/.*$ so paths beginning
with or containing the test/ directory are excluded correctly.
In @.github/workflows/performance.yml:
- Around line 49-57: Update the “Record environment” step in the performance
workflow to include GOMAXPROCS, the Go runtime CPU count, and the CPU model in
environment.txt alongside the existing metadata. Use the same values or commands
already used by scenario-matrix.txt so the uploaded reports remain consistent.
- Line 12: Replace the broad permissions setting with a workflow-level
permissions block granting only contents: read, preserving the checkout
functionality while removing access to unrelated permission scopes.
- Around line 65-69: Update the scheduled benchmark workflow around the “Enqueue
benchmarks with benchstat input” step to either select an appropriate CI
baseline and run benchstat against enqueue-bench.txt, uploading the comparison
output as an artifact, or explicitly document that trend comparison is deferred
until a baseline exists.
- Line 22: Update both actions/checkout@v4 steps in the workflow jobs to set
persist-credentials to false, ensuring checkout credentials are not retained for
subsequent repository commands.
- Around line 38-42: Remove job-level continue-on-error from the Scenario matrix
job so setup, scenario, benchmark, and reporting failures remain visible. Keep
non-blocking behavior only for timing assertions, and update the Upload
performance artifacts step with if: ${{ always() }} so artifacts are uploaded
even when earlier steps fail.
- Around line 24-26: Update every actions/setup-go workflow reference in
performance.yml, go.yml, and bench.yml to `@v6`, including the current `@v4` and
remaining `@v3` references. Ensure the workflows use a runner version v2.327.1 or
later to satisfy setup-go@v6’s Node.js 24 requirement.
In `@docs/improvements/baselines/enqueue-darwin-arm64.txt`:
- Line 3: Remove the trailing duplicated darwin/arm64 value from the environment
line in the Darwin ARM64 baseline, preserving the platform information already
included in the go version output.
In `@docs/improvements/plan-perf.md`:
- Around line 507-509: Update the benchmark workflow reference in the
performance plan to point to the current alert-threshold and fail-on-alert
settings, replacing the stale line range with the setting names or their correct
location. Keep the existing recommendation to demote the post-merge 200%
threshold unchanged.
- Around line 115-117: Update the conclusion and related tests to use
“publication order” consistently as the FIFO contract, replacing any “admission
order” wording. Treat the successful publication event as authoritative, since
concurrent producer reservation order may differ from publication order, and
preserve the existing ordering behavior otherwise.
In `@docs/improvements/thresholds.md`:
- Around line 30-35: Change the Scenario recorder allocations per item threshold
in the blocking table from “no growth” to the intended exact numeric allocation
count, using “exactly 0” if no per-item allocations are allowed. Update
TestHarnessRecorderDoesNotAllocatePerItem to assert that same numeric threshold.
- Around line 44-50: Update the “Throughput gates (blocking)” section in the
thresholds documentation to match the current workflow: either rename it as
advisory/planned and retain the documented non-enforcing status, or implement a
performance.yml comparison step that uses benchstat and fails when the +10%
threshold is exceeded. Keep the section’s status consistent with the enforcement
actually present.
- Around line 11-19: Update the Go version value in the Reference environment
table to 1.22.4, matching the version selected by go.mod and performance.yml; do
not leave the broader “1.22 or later” range unless separate toolchain baselines
are added.
In `@test/scenario/arrival.go`:
- Around line 115-129: Update fixedRate to handle a zero gap after calculating
time.Second divided by ratePerSecond; return nil before computing count when gap
is zero, preserving the existing behavior for non-positive inputs and valid
rates.
In `@test/scenario/run.go`:
- Around line 279-302: Update the completion-wait logic around the deadline
timer to drain completed sends after timeout until the batcher has fully
stopped, without closing completed while the processor may still write; record
the timedOut state and remaining shortfall, then pass closeErr/timedOut through
summarise so the result reports the failure. Remove the no-op _ = err branch and
preserve normal completion behavior.
In `@test/scenario/scenario_test.go`:
- Around line 165-205: Make the overload scenario host-independent by
configuring Config.Producers to runtime.NumCPU() so the producer can approach
the intended rate; import runtime if needed. Compute the expected backlog
deficit using the run’s measured offering duration or equivalent result window
rather than the fixed duration constant, while preserving the existing overload
and queue assertions.
---
Nitpick comments:
In `@docs/improvements/plan-perf.md`:
- Line 156: Add language identifiers to every fenced code block in plan-perf.md,
including the blocks at the referenced locations, using the appropriate
identifier such as text, go, or sh so all Markdown fences satisfy MD040.
In `@Makefile`:
- Around line 30-32: Update the guards-race target to pass Go’s short-test flag
when running the race detector, so scenario tests that honor testing.Short() are
skipped while standard race coverage remains enabled.
- Around line 26-46: Add guards, guards-race, guards-allocs, bench-enqueue, and
bench-matrix to the existing .PHONY declaration in the Makefile so filesystem
entries with those names cannot suppress their recipes.
In `@test/scenario/matrix_test.go`:
- Around line 87-103: Replace the hand-rolled itoa function with strconv.Itoa in
the relevant test formatting flow, adding the strconv import and removing the
recursive digit-building implementation. Ensure values such as 1,500 retain
their full decimal representation rather than being abbreviated or losing
precision.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f93b90d-4f58-44d2-9a5a-5e1dfaf370f9
📒 Files selected for processing (17)
.github/workflows/bench.yml.github/workflows/go.yml.github/workflows/performance.ymlMakefiledocs/improvements/baselines/enqueue-darwin-arm64.txtdocs/improvements/plan-perf.mddocs/improvements/thresholds.mdpkg/batcher/allocation_test.gopkg/batcher/batcher_bench_test.gotest/scenario/arrival.gotest/scenario/matrix_test.gotest/scenario/processor.gotest/scenario/report.gotest/scenario/run.gotest/scenario/sample.gotest/scenario/scenario_test.gotest/scenario/summarise.go
This was referenced Aug 6, 2026
heynemann
marked this pull request as ready for review
August 6, 2026 20:02
Adds a phased plan to make sub-10ms batch windows practical, and to fix the correctness problems that currently make a smaller window unsafe or misleading. Measured findings that motivate the plan: - The processor runs inline in the aggregation loop, so the effective window is max(window, processor_duration). With a 50ms processor at 10k items/s, a 5ms window yields p50 120ms while a 100ms window yields p50 50ms: lowering the window today makes latency 2.4x worse. - A smaller window provides no overload protection. With a 2ms processor and a saturating producer, 100ms/10ms/1ms all accepted 1.8-2.4M items and grew the heap 2.2-4.3GB in two seconds. - Add can panic during shutdown, isClosed races under -race, and a partial batch with an interval beyond the close cap is silently discarded (50 accepted, 0 processed, Len reporting 50 phantom items). The plan specifies a validated admission and drain protocol: Batcher-owned queues that are never closed, a publisher gate with a coordinator self-signal to prevent lost wakeups, separate Pending and IntakePending accounting to avoid a worker-pool shutdown deadlock, and resumable Shutdown that never abandons a drain. Prototypes for the protocol were exercised under -race, including an n=4 worker pool and in-flight-at-shutdown cases. Delivery model: each phase is one pull request, and each milestone within it is an independently reviewable commit with its own acceptance criteria.
Milestone 1.1 of the performance plan. The previous suite could not support any performance claim: - BenchmarkBatcherBatchSize10_000 and BenchmarkBatcherBatchSize100_000 both called runBench(b, 100), so the published results for large batch sizes described a batch size of 100. - fmt.Sprintf ran inside the timed loop, so roughly 51 ns/op and 2 allocs/op of payload construction were attributed to Add. - Allocations were not reported and there was no multi-producer case. Each benchmark now configures the batch size in its name, builds its payload before the timed region, reports allocations, and has a RunParallel variant for producer contention. The suite is documented as enqueue overhead only, with a repeatable invocation, since it deliberately excludes batch completion and processor work. Measured effect: 0 allocs/op in the timed region, down from 2. benchstat over -count=10 separates the 100k case from the 100 case in bytes/op (238.5 B/op vs 176.0 B/op) while sec/op overlaps, which is expected because Add performs the same per-item work at any batch size. The plan's acceptance criterion is updated to state that bytes/op, not ns/op, is the discriminator.
Milestone 1.2 of the performance plan. Go benchmarks cannot answer the questions this project needs answered. ns/op is a mean over a closed loop, and a closed loop stops offering load exactly when the system slows down, hiding the failure mode that matters most: a slow processor under sustained arrivals. The harness is therefore open-loop. Arrival times are precomputed from a seeded schedule and are never gated on completion or capacity, so overload cannot suppress offered load (coordinated omission). It reports distributions from raw samples rather than means, separates admission blocking from queueing and processing, and marks a run invalid when the generator itself fell behind, so generator lag can never be reported as batcher latency. Recording uses preallocated atomic slots indexed directly by the item, so neither the producing nor the processing path allocates or locks while being measured. Heap and queue depth are sampled during the run, because reading after the drain reports a recovered process and hides the peak backlog. The suite reproduces both baseline findings independently of the ad-hoc probes used while writing the plan: - Inline slow-processor inversion: with a 50ms processor at 10k items/s, a 5ms window measures p50 170ms while a 100ms window measures p50 100ms. Lowering the window makes latency worse, because the inline processor bounds the effective window. - No overload protection from a smaller window: at 500k items/s offered against a 2ms processor, both 100ms and 1ms accept everything (~500k/s) while completing only ~240k/s, peaking at 76,000 queued items. The window does not bound queued work. Both are pinned as tests, so decoupling intake from processing in Phase 3 will make the inversion test fail, and that failure is the signal the fix worked.
Milestone 1.3 of the performance plan. CI previously ran benchmarks post-merge only, on mutable ubuntu-latest with Go "stable", alerting at a 200% regression. That threshold cannot catch a real regression: measured run-to-run spread within a single benchmark configuration has exceeded the difference between the configurations it compares. The test job also ran neither the race detector nor any allocation check. Splits performance CI into two lanes with different jobs: - performance.yml "guards" is blocking and gates only on signals that are stable on shared runners: go test -race and exact allocation counts. - performance.yml "matrix" is scheduled and informational. It runs the scenario sweep, records environment metadata, and uploads artifacts. It never fails a pull request on timing. Latency percentiles are deliberately not gated anywhere. They are reported and compared as a trend, because a p99 gate on a GitHub-hosted runner would fire on noise. Adds docs/improvements/thresholds.md with the predeclared numbers, and a stored local baseline. The doc states plainly that the local darwin/arm64 baseline is not the CI reference and that the ns/op gate stays advisory until a baseline is recorded from the reference runner. Adds the first allocation gate: Add must not allocate per call in steady state. Verified it actually catches a regression by temporarily injecting an escaping allocation into Add, which failed the test with 2 allocations; a non-escaping make() was optimised away and did not, so the test asserts on real behaviour rather than on a construct the compiler can elide. The historical bench.yml lane is kept for trend continuity but demoted to fail-on-alert: false, with a comment explaining not to lower its threshold expecting protection.
The scenario harness is test infrastructure, not shipped library code. Its reporting and arrival-shape helpers are exercised by the opt-in matrix lane, which CI skips by default, so counting them dragged reported coverage from 92.4% to 76.2% and would have failed the 80% gate. Excludes internal/scenario the same way internal/test is already excluded, in both the CI action and the Makefile coverage targets. This keeps the gate meaningful for pkg/batcher rather than inviting tests written only to satisfy a percentage.
…e assertions Two corrections to Phase 1. Location: internal/ is for library code, so the scenario harness belongs in test/scenario. Moving it also simplifies coverage configuration, because the pre-existing "test/" exclusions already cover it and the extra internal/scenario filters added earlier are no longer needed. CI correctness: the first run of the new guard lane failed, and the failures were real rather than flaky. Two assertions had absolute thresholds tuned to a developer machine: - TestSmallWindowGivesNoOverloadProtection required a backlog above 50,000 items. The GitHub runner could only offer 117k items/s rather than 500k, so the threshold was unreachable there. It now asserts that the peak backlog absorbs the run's own measured accept-minus-complete deficit, which holds on any hardware because it is derived from that run. - TestHarnessReportsLatenessAndInvalidatesBadRuns required a 5ms lateness budget to be achievable. That measures the host scheduler, not the harness: the runner showed 5.63ms of p99 sleep overshoot. It now probes the host's actual overshoot and asserts the guard accepts a budget above it and rejects one below it, so both directions of the validity guard stay covered. Verified under GOMAXPROCS=2 to approximate a constrained runner: p99 overshoot 5.94ms, and both windows still accumulate an ~82,000 item backlog, so the baseline finding is preserved.
Fixes 13 of 15 review findings; the other two are rebutted on their threads. Two real bugs, both reproduced before fixing: - fixedRate panicked with "integer divide by zero" for any rate above 1e9 items/s, which AtCapacity can produce: a 1µs processor with a 1000-item batch yields a service rate of exactly 1e9. It now refuses an unrepresentable schedule rather than substituting a 1ns gap, which would have allocated duration/1ns entries. - The scenario runner could leak a goroutine per timed-out run. The completion channel held 1024 entries, so once the waiter stopped reading, the processor parked on the send forever, Close could not drain, and the matrix sweep does over a hundred runs in one process. Replaced with an atomic counter plus a non-blocking latch, which cannot block the processor at all. The reviewer's drain-then-close suggestion was not used because closing a channel a live processor may still send on would panic, as they noted themselves. Result now reports OfferedFor, TimedOut and CloseErr, so a run that gave up says so instead of folding the shortfall into its completion counts. The overload test was host-dependent: it asked one producer for 500k items/s, which needs a 2µs sleep against tens-of-microseconds granularity, so the run stretched well past its 300ms window and the offered rate collapsed toward the service rate. It now spreads the schedule across runtime.NumCPU() producers and derives the deficit from the measured offered window rather than the configured duration, which were two different time bases. Measured after the fix: 499,988 items/s offered within 300ms, backlog 77,000. Workflow hardening: - permissions: read-all narrowed to contents: read; neither job writes. - persist-credentials: false on both checkouts, since both run repository code. - Removed job-level continue-on-error, which hid setup, scenario and benchmark failures, and added if: always() to the artifact upload so a failed step no longer skips the artifacts that are the point of the lane. - environment.txt now records GOMAXPROCS and CPU model, matching what the scenario report embeds, so an uploaded artifact is interpretable alone. - setup-go v3/v4 to v6 and checkout v3 to v4 across the touched workflows. - Documented that benchstat comparison is deferred until a scheduled run stores an ubuntu-latest baseline; comparing against a developer machine would be meaningless. Documentation corrections: duplicated darwin/arm64 in the baseline header, the conclusion saying "admission order" where the contract is publication order, a stale bench.yml line reference now naming the settings instead, the reference Go version stated as 1.22.4 to match go.mod, the throughput gate relabelled advisory to match what CI actually enforces, and a stale test name in the threshold table. The recorder allocation threshold is now numeric (≤ 1 and no growth with run length) rather than "no growth", and the test asserts both. It is not "exactly 0" because AllocsPerItem measures the whole pipeline including Batcher's own per-batch allocations; measured values are 0.03-0.04 per item.
The coverage action evaluates ignore-pattern with JavaScript new RegExp. The old pattern `\test\/.*$` was not a literal path separator followed by test: `\t` is a tab escape, so it matched no repository path at all. As a result, test/scenario was included in the coverage aggregate despite the workflow claiming it was excluded. Use `(^|/)test/.*$`, which matches a test directory at the path start or after a separator. Apply the same explicit form to internal/test: its old `\internal` pattern worked only by accident because JavaScript treats the invalid \i escape as a literal i. Verified with the same JavaScript RegExp semantics as gwatts/go-coverage-action: both test/scenario/run.go and internal/test/helpers.go match, while pkg/batcher/batcher.go does not.
The README told contributors to run `make bench` and showed BenchmarkBatcherBatchSize* output. Those benchmarks no longer exist under those names, and two of them were the mislabeled cases this phase fixed, so the section documented a suite that had been replaced. Contributing now carries the baseline policy, because that is where a contributor looks before opening a PR: - measure before changing, compare with benchstat rather than by eye, and only compare runs from the same machine; - pick the tool that matches the claim. The enqueue benchmarks measure producer-side cost only and cannot support a latency or throughput claim, which is the easiest way to be confidently wrong here; - expect TestReproducesInlineSlowProcessorInversion and TestSmallWindowGivesNoOverloadProtection to fail if you fix what they pin. They encode known defects, so a failure is likely the change working and the test should be updated rather than deleted; - report the numbers with the environment, since a result without its environment cannot be reproduced; - record a negative result rather than dropping it. Adaptive batch capacity was gated on exactly this kind of measurement, and an earlier estimator was rejected because the numbers were worse than doing nothing. A new "Developing batcher" section carries the reference material: what each tool measures, why the harness is open-loop, what it reports, and which CI lane blocks. Cross-linked rather than duplicated, so the workflow rules live in one place. Verified every command, path, and claim in the new text: make guards, make bench-enqueue piped to benchstat, make bench-matrix, the stored baseline file, both pinned tests, and all intra-document anchors.
No build, unit-test, coverage or lint check has actually run on a pull request since 2026-04-22. The Go workflow reports startup_failure on every branch, including main, and creates zero jobs, so GitHub rejects it before scheduling anything. Diagnosis: the workflow file is not the problem. actionlint reports no findings, every referenced action resolves, and the pinned tags exist. What is distinctive about this workflow is that it is the only one depending on third-party actions (super-linter, go-test-report, go-coverage-action, codeclimate-action); codeql.yaml uses only first-party actions and runs fine. That points at a repository or organisation Actions policy restricting third-party actions, which cannot be confirmed from the API while Actions is mid-outage. Rather than leave the checks dark, this reimplements the same coverage using only first-party actions/* steps and plain go commands: - build: go build ./... - unit tests: go test -v ./... - vet and formatting: go vet ./... plus a gofmt check that fails with the file list - coverage: go test -coverprofile with an 80% threshold, excluding test/ for the same reason internal/test is excluded — it is measurement infrastructure, not shipped code Two details worth noting. The pull_request trigger is deliberately not filtered by base branch: stacked PRs target the layer below rather than main, so a branches: [main] filter would skip every layer above the bottom one. And setup-go resolves from go.mod rather than a pinned version, so the tested toolchain cannot drift from the declared floor. Verified locally before committing: actionlint clean, and each step's exact command run by hand, including the coverage parse and threshold arithmetic (92.4% against the 80% gate).
Six findings from the Phase 1 adversarial review. PendingPeak was documented as a "queue depth proxy" and was not one. Len() counts accepted work that has not reached a terminal outcome, so it includes the batch accumulating and the batch inside the processor. At sub-saturating rates it is a sawtooth roughly one window deep even with zero backlog, and periodic sampling misses the peaks. Renamed PendingWorkPeak and documented as a sampled lower bound on in-system work rather than backlog, with a pointer to Stats().Queued in Phase 2 for true depth. The overload test's assertion is unchanged in strength but its comment now says what the number is: the rate comparison is load-bearing, the work-peak check is a deliberately weak check on a deliberately weak metric. HeapHighWater silently fell back to the post-drain reading when the sampler had not fired -- exactly the recovered-process number run.go says must not be used. Added HeapSampled so a caller can tell a real peak from the fallback. Added GCDuringRun, reported always, and DisableGCDuringRun, opt-in. Disabling the collector is not the default because the overload scenarios deliberately accumulate gigabytes in an unbounded queue; turning GC off there converts a scenario into an OOM. Reporting lets a tail-latency comparison flag a suspect run without that risk. Batch sizes were stored as time.Duration to reuse the duration distribution helper, so a mean batch of 455 rendered as "455ns". Added IntDistribution. Corrected the Producers doc: the round-robin partition keeps the set of offer times independent of producer count, but each producer sleeps gap x Producers, so the achievable rate is not. Points at Lateness/LatenessValid as the real guard. The allocation CI gate used `go test -run 'Alloc'`, a substring match that exits 0 when it matches nothing -- renaming either test would have dropped the gate with no CI error. It now requires TestAddAllocatesNothingPerCall and TestHarnessRecorderDoesNotAllocatePerItem to have actually passed. Verified by renaming one: the gate fails with "was it renamed?" instead of passing silently. Also noted in plan-perf.md that the gigabyte overload figures come from an unbounded exploratory probe while the pinned test runs at a smaller scale, so a reader comparing them does not read the difference as a contradiction.
heynemann
force-pushed
the
phase-1-performance-baseline
branch
from
August 7, 2026 13:53
8b6d33c to
d3e2059
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/improvements/plan-perf.md`:
- Line 164: Add the text language identifier to the seven fenced code blocks in
the performance plan document, including the blocks near the referenced
sections, while preserving their existing pseudocode, equations, and ASCII
dependency graph contents.
In `@Makefile`:
- Line 28: Add guards to the existing .PHONY declaration in the Makefile, and
include any other command-only targets that are not already marked phony so Make
always executes them.
- Around line 34-40: The allocation gate must cover Enqueue and the recovery
wrapper, or explicitly defer them. Update guards-allocs in Makefile to require
the corresponding allocation tests if they belong in Phase 1.3; otherwise update
docs/improvements/plan-perf.md at lines 488-490 to mark both checks as
future-phase additions, while preserving the existing allocation tests.
In `@test/scenario/run.go`:
- Around line 376-381: Move GoroutinesPeak sampling out of the post-wg.Wait path
so it runs while producers are active, reusing the existing sampler goroutine or
equivalent workload-monitoring loop. Update the peak value from
runtime.NumGoroutine during execution, while preserving the final offeredFor
measurement and producer synchronization.
- Around line 363-370: Update the admission timing flow around b.Add and the
admissionEnd/accepted stores so AdmissionEnd is recorded before inline
processing can begin. Ensure ProcessorStart cannot precede the admission
completion event, preventing negative QueueDelay and processor service time from
being included in AdmissionBlocking; preserve the existing batch acceptance
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 596ed86c-8304-4344-9857-b844d702a77c
📒 Files selected for processing (9)
.github/workflows/performance.yml.gitignoreMakefiledocs/improvements/plan-perf.mdtest/scenario/report.gotest/scenario/run.gotest/scenario/sample.gotest/scenario/scenario_test.gotest/scenario/summarise.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/scenario/summarise.go
- test/scenario/scenario_test.go
- .github/workflows/performance.yml
- test/scenario/report.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/improvements/plan-perf.md`:
- Around line 490-491: Update the “Scheduled/manual lane” benchmark instructions
in plan-perf.md to separate collection from comparison: use go test -bench=.
-count=10 to generate benchmark output files, then use benchstat old.txt new.txt
to compare them; remove the non-executable benchstat -count=10 usage.
In `@Makefile`:
- Around line 48-50: Update the bench-matrix target to pass Go’s -count=1 flag
to go test, ensuring each TestScenarioMatrix execution runs without cached
results while preserving the existing test selection, timeout, verbosity, and
scenario path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 068a22f5-f3e8-4a0e-be2d-9617069800e9
📒 Files selected for processing (9)
.github/workflows/performance.yml.gitignoreMakefiledocs/improvements/plan-perf.mdtest/scenario/report.gotest/scenario/run.gotest/scenario/sample.gotest/scenario/scenario_test.gotest/scenario/summarise.go
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- test/scenario/report.go
- test/scenario/summarise.go
- .github/workflows/performance.yml
- test/scenario/scenario_test.go
- test/scenario/sample.go
- test/scenario/run.go
Address three verified review findings. GoroutinesPeak was sampled after producer WaitGroup completion, so it could never include configured producers and understated concurrent scenarios. The existing in-flight sampler now records the maximum runtime.NumGoroutine every 2ms, seeded before load for runs too short to sample. A 32-producer probe now reports peak 42 from base 2; before this change it could only see the post-producer count. The performance-plan command said `benchstat -count=10`, but -count belongs to go test. Documented separate collection and comparison commands instead. bench-matrix now uses -count=1 so its report-only matrix is never served from Go's test cache. Marked every command-only Make target .PHONY. Verified by creating a file named guards: make still expands the guards recipe rather than reporting it up to date. Also labelled the seven text/pseudocode fences required by markdownlint MD040. The remaining timing comment was verified and rebutted rather than changed: Phase 1 uses a rill pipeline, not inline processing. With the reviewer's proposed BatchSize=1, 2ms processor setup, QueueDelay.min was positive (5.6us) and AdmissionBlocking.p50 was 167ns, so processor time does not overlap admission in this harness.
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
1 participant
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.
What
Phase 1 of the batcher performance plan (
docs/improvements/plan-perf.md). Itestablishes measurement we can trust. No library behaviour changes — only
tests, a harness, docs, and CI.
Five commits, one per milestone:
docs: add batcher performance and reliability plantest: fix and scope enqueue microbenchmarkstest: add open-loop scenario harnessci: add performance guard lanes and predeclared thresholdsci: exclude scenario harness from coverage thresholdsWhy
Our users run 100ms batch windows, so a request crossing several batching
services can accumulate roughly one window per hop. Before changing any default,
we need evidence — and the existing suite could not provide it:
BenchmarkBatcherBatchSize10_000andBenchmarkBatcherBatchSize100_000bothcalled
runBench(b, 100), so published results for large batch sizesdescribed a batch size of 100.
fmt.Sprintfran inside the timed loop, attributing ~51 ns/op and 2 allocs/opof payload construction to
Add.Join, so batch formation and processing were excluded,and the interval was 1s, so the timer path was never exercised.
ubuntu-latest.Measured spread within one configuration exceeded the difference between
configurations, so that gate could not fire on a real regression without also
firing on noise. The test job ran neither
-racenor any allocation check.What the harness found
The scenario harness is open-loop: arrival times are precomputed and never gated
on completion or capacity, so overload cannot suppress offered load
(coordinated omission). Recording uses preallocated atomic slots, so neither the
producer nor processor path allocates while measured, and heap/queue depth are
sampled during the run because reading after the drain hides the peak.
It independently reproduces two findings that shape the rest of the plan:
1. A smaller window currently makes latency worse. The processor runs inline
in the aggregation loop, so the effective window is
max(window, processor_duration). With a 50ms processor at 10k items/s:2. A smaller window gives no overload protection. At 500k items/s offered
against a 2ms processor, both windows accept everything while completing far
less:
Both are pinned as tests. When Phase 3 decouples intake from processing, the
inversion test should start failing — that failure is the signal the fix worked.
Validation
go test -race ./...clean.go vet ./...clean.pkg/batchercoverage 93.6%.Addmeasured at 0 allocs/op in the timed region, down from 2.non-escaping
make()injected intoAddwas optimised away and did notfail the test; only an escaping allocation did (failing with 2 allocations).
The gate asserts on real behaviour, not on something the compiler can elide.
Notes for reviewers
expected
benchstatto separate the mislabeled cases by ns/op; measurementshows separation appears in bytes/op (238.5 vs 176.0 B/op) while sec/op
overlaps within noise. That is expected, since
Addperforms identicalper-item work at any batch size, so the plan now names bytes/op as the
discriminator.
and exact allocation counts block a PR.
internal/scenariois excluded from the coverage gate as test infrastructure,matching how
internal/testis already treated. Counting it would have pushedreported coverage from 92.4% to 76.2% and invited tests written only to satisfy
a percentage.
darwin/arm64baseline is explicitly not the CI reference; thens/op gate stays advisory until a baseline is recorded from
ubuntu-latest.Dependency context
Bottom of the stack, targeting
main. Phase 2 (removingrillbehindcharacterization tests, then the admission/drain protocol) will stack on top.
Stack created with GitHub Stacks CLI • Give Feedback 💬
Follow-up correction
After the first CI execution, the blocking guard exposed two host-dependent
scenario assertions. They are fixed in
fb9be23:internal/scenariototest/scenario, because it istest infrastructure and
internal/is reserved for library code.accept-minus-complete deficit instead of requiring an absolute 50,000-item
backlog tuned to one machine.
overshoot instead of asserting that a fixed 5ms budget is schedulable.
The corrected Race and allocation guards job passed on GitHub Actions in
1m2s.
Summary by CodeRabbit
New Features
Documentation
CI Improvements