Repository navigation
perf(chat-store): avoid reprepare in contact and identity lookups - #288
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review Please review commit 3736097, especially Diesel query cache identity, optional first-row behavior, account isolation and unchanged PN/LID timestamp/LID tie ordering. This targets only the measured point lookups; bundled SQLite native counters, base/head CPU/walltime and separate C/Rust memory evidence are in docs/sqlite-point-lookup-performance.md. |
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: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
CI triage checkpoint: the shared web failure is a baseline issue from #282, reproduced on main 33108ce before any profiling patch. The daemon imports actual avatar/web.rs storage code, but its GUI decode helper references missing DecodePriority/gpui/crate::session types. Shared fix is isolated in #290, commit d21f091, with all storage code and browser tests preserved. Final local browser suites pass, 16 daemon + 32 session tests. The #288 native fuel timeout is a separate baseline flake. Main passes isolated in 8.47 s and fails under controlled CPU contention with the same timeout. Commit 0da555a in #290 adjusts only the infinite-loop fixture's fuel consumption efficiency; production budget, deadline and assertions remain unchanged. It passes 5/5 under identical contention; full plugin-host suite passes 135 tests, one ignored. Detailed reproduction/classification: docs/measurements/ci-288-289-triage.md in #290. Neither SQL optimization nor phone metadata changes are included there. #290 remains draft pending remote CI and requested Codex review; no merge performed. |
|
@codex review Please review final integration head 5e12350. It merges the exact #290 branch at 71130e9, preserving its commits. Before integration, #290 remained open and main was 33108ce. #290's full CI, Pages and Avatar restore workflows passed; its final Codex review had no findings. No PR merge was performed. Verified the whole chat-store tree, Cargo.lock, Cargo.toml and .cargo/config.toml are byte-identical to the original SQL optimization commit 3736097. The published merge tree exactly matches the locally tested candidate tree 3afb9d1463758f0a7e8396d7426778de20a8ae53. Revalidation: 596 native tests passed, eight existing ignored; 16 daemon + 32 session browser tests passed; chat-store/plugin-host all-target/all-feature Clippy with warnings denied and formatting passed. No SQL reimplementation or cherry-picked shared fixes. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Contact and nested LID identity reads recompiled cached SQLite statements because Diesel
.first()bindsLIMIT ?. Use a typed fixedLIMIT 1wrapper only in ChatStore::contact and both directions of counterpart_of, preserving filters, optional results and updated_at/LID ordering. No CAST expansion, page-query changes, production dependencies or schema changes.Synthetic release measurements show median paired CPU ratios of 0.519 for contact present/absent batches and 0.793 for LID reaction lookups; the unchanged PN control is 1.008. The exact bundled SQLite 3.53.2 probe records 20,000 to zero reprepare events with matching plans. C allocation traffic decreased; retained SQLite heap was unchanged. Variability, raw samples and measurement limits are in docs/sqlite-point-lookup-performance.md. These are synthetic read workloads, not application startup gains.
The branch includes the exact shared CI fixes from #290 through merge commit 5e12350, preserving their original commits. The chat-store tree, Cargo.lock, Cargo.toml and SQLite build flags remain byte-identical to original SQL commit 3736097. The merge tree matches the validated candidate. Integration validation passed: 596 native tests, eight existing ignored tests, all 16 daemon and 32 session browser tests, Clippy with warnings denied and formatting. Synthetic data only.
Final head 5e12350 is qualified: CI, Pages, Avatar restore and Release artifacts all completed successfully. Codex reviewed this exact head without findings; no unresolved review threads or merge conflicts. Ready for user review and merge. No PR merge was performed.