perf: avoid decoding usage events for Codex activity - #3528
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
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. |
|
Codex review: needs maintainer review before merge. Reviewed September 10, 2026, 12:37 AM ET / 04:37 UTC (Revision 4). ClawSweeper reviewWhat this changesThe PR reads cached Codex activity from daily totals without decoding individual events, simplifies optional returns, and adds regression coverage and documentation. Merge readiness✅ Ready for maintainer review This remains a useful optimization absent from current main and v0.58.0; source review and reported benchmark results support landing it, with no actionable correctness findings. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s Usage & Spend dashboard reads locally cached Codex session activity from SQLite. The changed read path supplies daily token totals while detailed spending reports continue loading individual pricing events. flowchart TD
A[Usage and Spend dashboard] --> B[Account and date request]
B --> C[Cached activity reader]
D[SQLite session cache] --> C
C --> E[Read daily aggregates]
E --> F[Check scope and scan coverage]
F --> G[Daily token totals or unavailable]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep activity reads aggregate-only while preserving scoped coverage checks and full event detail for spending reports. Do we have a high-confidence way to reproduce the issue? Yes, from source: current main requests event-bearing reports for an activity calculation that consumes daily aggregates only. Supplied before-fix counters corroborate the overhead; this review did not execute tests. Is this the best way to solve the issue? Yes. Extending the existing read-purpose projection avoids a parallel cache implementation and leaves report pricing, persistence, and account filtering intact. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 33161afe7f2c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles) |
Read scoped daily aggregates while preserving coverage, retries, and account boundaries. Return typed optional cached results directly and document the lighter activity projection. Co-authored-by: Nikolai Berezovskii <nikolai@brzv.sk>
8729fc1 to
4b9ee5e
Compare
|
Landed as 5c0d7b4. Thanks @brzvsk for the focused projection fix and release-core measurements. The activity reader now loads scoped daily aggregates without materializing usage-event payloads. Account/root, time-zone, coverage, incomplete-scan, and retry checks remain in place, and detailed reports retain their row-level history. The nearby optional-return cleanup preserves nil/error behavior. Verification:
Swift validation used Keychain suppression and Codex/session-file isolation through the test wrapper. Native UI/provider probing was unnecessary for this cache-read-only change. The contributor’s benchmark remains their measurement of this one read path; #3247 stays open for its broader CPU and WebKit symptoms. Documentation and the 0.58.1 Unreleased notes include the change and credit. |
Cached Codex token activity now reads scoped daily aggregates instead of loading and decoding every usage-event payload. Account/root and time-zone validation, coverage, pending scans, retained retries, identity reconciliation, and detailed monetary/project/session reports keep their existing behavior. The nearby cached-read methods also return typed optional executor results directly, removing redundant double-optional wrapping.
This addresses one verified refresh-time CPU and memory hotspot related to #3247. It does not claim to resolve that report’s separate WebKit or whole-app energy symptoms. No parser hash, schema, pricing, refresh cadence, or credential-ownership change is introduced.
Maintainer proof:
make checkpassed with zero violations; the Codex parser hash remains4969a789db679c93.make test: all 1,057 selections across 89 groups passed on the first attempt, with zero retries or timeouts (1,008.5 seconds). Exact-head CI remains the final pre-merge gate.The contributor’s release-core benchmark on independent private-cache copies measured median activity-call CPU of 1.385110 seconds before and 0.234250 seconds after, with complete output parity and unchanged stored-record fingerprints. Those are contributor measurements of this read path, not a prediction of whole-app battery savings.
Includes documentation and an Unreleased performance entry. Thanks @brzvsk!