perf: reduce Codex cost refresh metadata work#1430
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 6:32 AM ET / 10:32 UTC. Summary Reproducibility: not applicable. as a bug reproduction path for this PR review. The PR body reports profiling current main and the exact branch build on a real 60,607-file archive, but this read-only review did not rerun that benchmark. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused stat-based metadata/cache-validation patch after CI confirms the added regressions and maintainers accept the compatible-cache migration semantics. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction path for this PR review. The PR body reports profiling current main and the exact branch build on a real 60,607-file archive, but this read-only review did not rerun that benchmark. Is this the best way to solve the issue? Yes, based on source inspection this is a narrow maintainable solution: it targets the measured metadata/root-validation cost without adding a persisted cursor, memo, or new stateful artifact. The main remaining decision is whether to accept the prior producer key as parser-compatible. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 159d03ceb318. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Co-authored-by: pickaxe <54486432+ProspectOre@users.noreply.github.com>
7dceb10 to
e3b0597
Compare
Summary
fstatatpassCloses #1392.
Root cause
Profiling current
mainagainst a real 365-day archive showed the proposed SQLite priority memo was aimed at a minority cost. The archive contains 60,607 JSONL files (~43 GB);logs_2.sqliteis ~713 MB with ~430k rows. In the expired refresh trace, priority SQLite work was about 4% while per-file Foundation metadata/resource-identifier reads and repeated cached-path validation dominated.A direct 60,607-file benchmark measured:
The replacement keeps current refresh policy and cache correctness. It adds no cursor, persisted memo, overlap state, or new unbounded collection.
Exact candidate proof
All runs used the exact branch build and the same real archive. Candidate cache output was normalized by removing
updatedAt; expired and immediate-refresh JSON were byte-identical.main, expired cachemain, forced refreshThe candidate is roughly 4-5.5x faster on the recurring expired-refresh path than current
mainor the stacked alternatives, with no extra persisted artifact. Peak footprint remained ~736-737 MB; the remaining dominant work is the existing ~39 MB JSON cache decode/encode.Correctness contracts
fstatat(..., 0)follows the target, matching previous Foundation behaviorValidation
swift test --filter 'CostUsageCacheTests|CostUsageScannerTests'- 21/21 passmake check- SwiftFormat, SwiftLint, and generated parser hash cleanswift testrun twice - only existing parallel app-server timeout flakes; each affected suite passes alonegpt-5.5) - clean, no accepted/actionable findings, confidence 0.82git diff --check- cleanThis supersedes the larger stateful stack while retaining contributor credit from #1404, #1421, #1422, and #1423.