perf(collectors): stop reading the same files twice in one cycle - #243
Merged
Conversation
`Application.collect` calls `spec.collect` and then `spec.usage` back to back. Both reach `analyze_codex_transcript`, which tail-reads up to `tail_bytes` and `json.loads` every line it finds. They overlap on any rollout that is both active and among the eight newest the quota reader samples, which is the ordinary case for whichever session is running right now. `codex_analysis` is a cached sibling keyed on `(st_mtime_ns, st_size)`, the same shape `codex_instruction` and `codex_plan` already use. A sibling rather than a `state` parameter on the analyzer: it is called as a `functools.partial` over its config in test_transcripts and positionally in test_codex, so widening its signature would be a contract change for a caching detail. What this saves is the intersection, not the whole read. `collect` analyzes only active rollouts; `usage` analyzes the eight newest unconditionally. The dedup removes exactly the files in both sets, at most eight per cycle. On a store where only three of the eight newest are inside the window it saves three reads. Do not read the ticket's 11.1ms as the delta. `heapq.nlargest` replaces `sorted(files, reverse=True)[:8]`, which ordered 516 entries to keep 8. Left alone: the two full-store globs. Both do sort - `glob_stores` delegates per root to `glob_under`, which is `sorted(glob.glob(...))` - so the ticket's count of two sorts was right even though it put them in one place. Neither is removable without changing `glob_under`, which every collector shares. The design doc's cache arithmetic is corrected in the same commit. Its `state.py` row counts the caches and classifies them by validity rule, and nothing in CI checks that count, so a nineteenth cache would have made it quietly wrong. Implements DRC-4276. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
`io.glob_under` is `sorted(glob.glob(...))`. Five collectors called it, or `glob_stores` over it, purely as a truthiness test, and `Application.collect` runs `spec.discover` in the guard immediately before `spec.collect`, which walks the identical tree again in the same pass. So the store was globbed and sorted twice to publish one row. `any_glob_under` and `any_glob_stores` sit beside the readers they mirror and stop at the first match. The four collectors that already had the cheap idiom show the shape: claude, codex and copilot use `any_store_dir`, goose uses `existing_stores`. Five, not the four the ticket named. `collectors/opencode.py` has the same `bool(glob_stores(...))` in its own `discover`; the ticket missed it, and a fix that left one behind would have been the next one to come back. `pi.py` is not a candidate: its discovery goes through `_session_paths`, which needs the paths. The cost is not measured here and the ticket says so. Droid, Gemini, Antigravity, Cursor and OpenCode stores are all absent on this machine, so the duplicate walk costs nothing locally. What is provable by reading is the duplication itself; on a machine with real history it doubles whatever that walk costs, and the two-level Claude glob measures 11.3ms for 3,805 files as a sense of scale. The contract test is the point of the change, more than the milliseconds. It AST-walks every collector's `discover` and fails if one reaches for a sorting reader, so the idiom cannot return in the next collector. Verified by restoring the old droid body and watching it fail. CONTRIBUTING.md told a new collector's author that discovery may be "a `glob_under()` call", which is how this got in. It now names the probes. Implements DRC-4277. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
Contributor
CoverageThreshold: |
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.
Implements DRC-4276 — The Codex store is globbed and stat-swept twice per collect, and the newest tails are parsed twice.
Implements DRC-4277 — Four discover() bodies use a full sorted glob as a boolean, then collect() repeats the same glob.
Two tickets, one PR, because they share a theme (stop the collectors doing the same filesystem work twice per cycle) and, more practically, they are the two claimants on the same design-doc table.
AGENTS.md§ Calibrating Effort owns PR grouping — "PRs are units of merge risk" — and this is the grouping that lets one author edit those two rows once instead of conflicting over them.DRC-4276 — Codex
Application.collectcallsspec.collectthenspec.usageback to back. Both reachanalyze_codex_transcript, which tail-reads andjson.loadses every line. They overlap on any rollout that is both active and among the eight newest the quota reader samples — the ordinary case for whichever session is running right now.codex_analysisis a cached sibling keyed on(st_mtime_ns, st_size), the same shapecodex_instructionandcodex_planalready use. A sibling, not astateparameter on the analyzer: it is called as afunctools.partialover its config intest_transcripts.pyand positionally intest_codex.py, so widening its signature would be a contract change for a caching detail.Measured, isolated (
bench_collect --repeat 5, reverting only the three changed files tomainfor the baseline):What it does not save. The dedup removes exactly
|{active} ∩ {8 newest}|tail reads per cycle, at most eight.collectanalyzes only active rollouts;usageanalyzes the eight newest unconditionally. On a store where three of the eight newest are inside the window it saves three reads. The ticket's 11.1 ms is the cost of the eight reads, not the delta.heapq.nlargestreplacessorted(files, reverse=True)[:8], which ordered 516 entries to keep 8.Left alone: the two full-store globs. Both do sort —
glob_storesdelegates per root toglob_under, which issorted(glob.glob(...))— so the ticket's count of two sorts was right even though it located them in one place. Neither is removable without changingglob_under, which every collector shares.DRC-4277 — discovery
Five collectors used a sorted match list as a boolean, and
collect()walked the same tree again moments later.Five, not the four the ticket named.
collectors/opencode.pyhas the samebool(glob_stores(...))in its owndiscover. The ticket missed it, and a fix that left one behind would have been the one to come back.pi.pyis not a candidate — its discovery goes through_session_paths, which needs the paths.The cost is not measured, and the ticket says so. Droid, Gemini, Antigravity, Cursor and OpenCode stores are all absent on this machine, so the duplicate walk costs nothing locally. What is provable by reading is the duplication; on a machine with real history it doubles whatever that walk costs.
So the contract test is the point, more than the milliseconds.
DiscoveryCostContractTestAST-walks every collector'sdiscoverand fails if one reaches for a sorting reader.CONTRIBUTING.mdtold a new collector's author that discovery may be "aglob_under()call", which is how this got in; it now names the probes.Verification
Written test-first, both red before and green after:
CodexReadBudgetTest.test_a_rollout_both_readers_want_is_tail_read_oncecountsio.read_tailcalls across onecollect+usagepair. Before:2 != 1on a two-element list. After: green.AnyGlobUnderTest, three cases, allAttributeErrorbefore the helpers existed. The primary countsiglobconsumption and asserts 1, so a naive "drop thesorted()" fix still fails at 3. It capturesreal_iglobbefore patching; resolvingglob.iglobinside the wrapper is infinite recursion and would have stayed red against a correct implementation.bool(glob_stores(...))body fails it withdroid.py discover() calls ['glob_stores']; restoring the fix passes.AGENTS.md§ Pre-PR Checks: 2,011 dashboard tests and 192 script tests green, coverage 90.3% against a threshold of 73. ruff,ruff format --check,mypy --strict(113 files),lint_embedded.py,validate_plugins.py,bump_version.py --currentall clean. No version field moved, checked against the merge base.sync-docsreconciliation: thestate.pyrow ofdocs/design-runtime-architecture.mdhard-codes a cache count and classifies each cache by validity rule, and nothing in CI checks it — a nineteenth cache would have made it silently wrong. Corrected 18→19, 6→7, 4→5. Theio.pyrow now distinguishes the sorting readers from the existence probes.For the reviewer
Two things worth the attention:
analyze_codex_transcriptkeeps its exact two-argument signature. That is deliberate and load-bearing —test_transcripts.pypartials over it andtest_codex.pycalls it positionally.