perf(tui): stop deep-copying the session twice per debounced save (#6214 T3) - #6273
Conversation
T3) Every debounced flush deep-copied the whole session history three times: 1. `save_session` -> `let mut durable_session = session.clone();` 2. `storage_compatible_copy` -> `journal.to_messages()` 3. `storage_compatible_copy` -> `let mut copy = self.clone();` Two of the three are pure waste. `flush_inner` already **owns** each `SavedSession` — it does `std::mem::take(&mut pending.sessions)` — and then handed out `&session` only for the callee to clone it straight back. And `compact_for_persistence_queue` has already emptied `messages` on the queued path, so the session being cloned in (3) is journal-only and is about to be overwritten anyway. So: - `storage_compatible_copy(&self) -> Option<Self>` becomes `make_storage_compatible(&mut self)`, doing the same fixup in place. On the queued path that is zero clones instead of two. - `serialize_saved_session` takes the session by value. - `save_session` / `save_checkpoint` each split into an owned implementation plus a one-line borrowing wrapper, so the ~150 existing `&session` call sites are untouched. The persistence actor's three hot sites call the owned forms. Net: three full-history deep copies per write become one. The remaining one is `journal.to_messages()`, which the on-disk schema genuinely requires — `SavedSession` carries both the journal and a `messages` compat projection. The behavioural contract is byte-identical JSON on disk, and the sharp edge is the two no-op cases. The old helper returned `None` for "no journal" and for "messages already equals the journal's active branch", and the caller then serialized the *original* — leaving a `metadata.message_count` that disagrees with `messages.len()` exactly as it was. The in-place version must return before recomputing that count, or every save silently edits live data. The design review flagged that nothing in the suite would catch it, so a test now does. Explicitly NOT in this slice: - **T2 is deferred, and not because of effort.** `Event::SessionUpdated` has exactly one runtime consumer, and it *moves* the `Vec<Message>` into `App::api_messages` — a `Vec` mutated in place by push/pop/truncate/clear and referenced across 45 files. An `Arc` in the event would just relocate the same copy into a `to_vec()` at the consumer, and force the engine to rebuild the Arc on every `AppendLog::push`. Making T2 a real win means reshaping `App::api_messages` itself, which is not one reviewable slice. - `create_saved_session_with_id_mode_and_stamps`'s double `to_vec()`: it costs 2N clones in any form, because the struct holds two representations of the same history. Removing it is a schema change and deserves its own issue. - `update_session`'s element-wise compare: not on the debounced path (its callers are `/save`, `/fork` and the Runtime API), and the compare is the append-vs-rebranch branch decision, i.e. correctness-load-bearing. Verification (macOS aarch64, source 21a02f1): cargo check -p codewhale-tui --all-features --locked --all-targets (clean) cargo fmt --all -- --check (clean) python3 scripts/check-blocking-calls-budget.py blocking-call budget: 626 sites across 181 files, within budget sh scripts/with-hermetic-test-home.sh cargo test -p codewhale-tui --lib \ --all-features --locked -j 5 -- --test-threads=2 \ storage_compatible_tests session_manager::tests persistence_actor:: test result: ok. 120 passed; 0 failed; 2 ignored; 0 measured; 12693 filtered out The byte-identity test was confirmed to fail without the early return — dropping it and recomputing `message_count` unconditionally gives test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 12813 filtered out Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 2m 46s —— View job Review complete
SummaryThis is a clean, well-scoped perf fix. I traced the ownership refactor by hand against the old Correctness
Minor nit (non-blocking)
Design
No correctness, safety, or test-coverage issues found that need fixing before merge. |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 2 file patches, sha256:564c984412a0747d80e33971a51fb465249825824f9e0cb2ae47c065c654a72e.
Pass 1: Refactor of the session-save path: SavedSession::storage_compatible_copy(&self) -> Option<Self> becomes an in-place make_storage_compatible(&mut self), serialize_saved_session takes the session by value, and save_session/save_checkpoint are split into owned impls (save_session_owned, save_checkpoint_owned) plus borrowing wrappers, with the persistence actor's three hot sites switched to the owned forms. I reconstructed the old helper's two None paths (no journal; messages already equal to the journal's active branch) and confirmed the new early returns occupy exactly those cases, so the byte-identity contract (including a deliberately stale metadata.message_count) is preserved; the two Some paths (empty messages rehydrated from the journal, divergent branch rebranched + leaf_id refreshed + count rewritten) also map 1:1. The remaining defect I can demonstrate is a small perf regression on rejected writes.
Findings
- [INFO] Borrowing wrappers deep-copy the whole session before any validation or admission check (
crates/tui/src/session_manager.rs:1834)
pub fn save_session(&self, session: &SavedSession)now unconditionally evaluatessession.clone()before delegating tosave_session_owned. Previously the clone happened inside the closure passed towith_session_write_admission(aftervalidated_session_pathand afterarchive_before_first_graph_write). Consequences on paths that reject the write before any I/O: (a) an invalid/unsafesession.metadata.idmakesvalidated_session_pathreturnErr— previously no clone was taken, now the full history (and journal) is cloned and immediately dropped; (b) a retired session makeswith_session_write_admissionskip the closure and returnOk(None)(turned intoretired_session_write_error) — previously the skipped closure meant no clone either, now every such call pays one full deep copy of the history for nothing. The same applies tosave_checkpoint(&self, session: &SavedSession)at line 1877. This is directly counter to the PR's stated goal, though the affected paths are error/retired paths rather than the debounced happy path, so the practical cost is small.
Suggestions
crates/tui/src/session_manager.rs:1834— To keep the ~150 borrowed call sites free of a wasted clone on rejected writes, validate the session id (and, if cheaply possible, the admission/retired check) before materialising the session. The minimal shape is to keep the shared body in an impl taking&SavedSessionfor the rare paths that must not clone, and have the owned entry point clone/consume only after the id and admission checks have passed. This is a judgement call about which rejection path matters; if the clone-on-error cost is acceptable, no change is needed.
Assessment
Pass 1: Source inspection only: no build, test, fmt or runtime check was executed for this review, and the reported verification output in the PR description was treated as untrusted evidence, not as a result I confirmed. Only crates/tui/src/tui/persistence_actor.rs was supplied as supplementary context; session_manager.rs was unavailable beyond the diff, so I could not inspect several things the change depends on and am explicitly not asserting anything about them: (1) the trait bound of with_session_write_admission — the new closure in save_session_owned/save_checkpoint_owned moves the owned session into its own body, which requires an FnOnce-style bound (the comment claiming a non-move closure works relies on that), and I could not verify the signature; (2) the behaviour of rebranch_active_messages and compact_for_persistence_queue, whose contract the in-place function assumes; (3) whether the new sibling #[cfg(test)] mod storage_compatible_tests can see create_saved_session (and Message/Role) through use super::*, which holds only if those items live at crate::session_manager scope rather than inside mod tests; (4) the two updated test call sites now clone (serialize_saved_session(saved.clone())), which is test-only. Within the diff I could inspect, the Option-to-in-place conversion is behaviour-preserving on both the no-op and the fixup paths, the mem::take/restore around rebranch_active_messages leaves messages populated on all non-panicking paths, and the actor's three call sites only ever dropped their SavedSession immediately after the old borrowed call, so moving them into the owned impls does not change observable state. One further unverified nit: the new doc on the public save_session links [Self::save_session_owned] to a pub(crate) item, which is the shape rustdoc's warn-by-default private_intra_doc_links lint reports; whether that matters depends on whether this workspace builds docs with warnings denied, which I could not check.
Advisory review by Codewhale (codewhale review --pr 6273 --post, head 2b04559f7db3804a69bdecb06d128cf36229a497). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| pub fn save_session(&self, session: &SavedSession) -> std::io::Result<PathBuf> { | ||
| let path = self.validated_session_path(&session.metadata.id)?; | ||
| self.with_session_write_admission(&session.metadata.id, || { | ||
| self.save_session_owned(session.clone()) |
There was a problem hiding this comment.
[INFO] Borrowing wrappers deep-copy the whole session before any validation or admission check
pub fn save_session(&self, session: &SavedSession) now unconditionally evaluates session.clone() before delegating to save_session_owned. Previously the clone happened inside the closure passed to with_session_write_admission (after validated_session_path and after archive_before_first_graph_write). Consequences on paths that reject the write before any I/O: (a) an invalid/unsafe session.metadata.id makes validated_session_path return Err — previously no clone was taken, now the full history (and journal) is cloned and immediately dropped; (b) a retired session makes with_session_write_admission skip the closure and return Ok(None) (turned into retired_session_write_error) — previously the skipped closure meant no clone either, now every such call pays one full deep copy of the history for nothing. The same applies to save_checkpoint(&self, session: &SavedSession) at line 1877. This is directly counter to the PR's stated goal, though the affected paths are error/retired paths rather than the debounced happy path, so the practical cost is small.
| pub fn save_session(&self, session: &SavedSession) -> std::io::Result<PathBuf> { | ||
| let path = self.validated_session_path(&session.metadata.id)?; | ||
| self.with_session_write_admission(&session.metadata.id, || { | ||
| self.save_session_owned(session.clone()) |
There was a problem hiding this comment.
To keep the ~150 borrowed call sites free of a wasted clone on rejected writes, validate the session id (and, if cheaply possible, the admission/retired check) before materialising the session. The minimal shape is to keep the shared body in an impl taking &SavedSession for the rare paths that must not clone, and have the owned entry point clone/consume only after the id and admission checks have passed. This is a judgement call about which rejection path matters; if the clone-on-error cost is acceptable, no change is needed.
The waste
Every debounced flush deep-copied the whole session history three times:
save_session→let mut durable_session = session.clone();storage_compatible_copy→journal.to_messages()storage_compatible_copy→let mut copy = self.clone();Two of the three are pure waste.
flush_inneralready owns eachSavedSession— it doesstd::mem::take(&mut pending.sessions)— and then handed out&sessiononly for the callee to clone it straight back. Andcompact_for_persistence_queuehas already emptiedmessageson the queued path, so the session cloned in (3) is journal-only and about to be overwritten anyway.The change
storage_compatible_copy(&self) -> Option<Self>→make_storage_compatible(&mut self), same fixup in place. Zero clones instead of two on the queued path.serialize_saved_sessiontakes the session by value.save_session/save_checkpointeach split into an owned impl plus a one-line borrowing wrapper, so the ~150 existing&sessioncall sites are untouched. The persistence actor's three hot sites call the owned forms.Three full-history deep copies per write become one. The survivor is
journal.to_messages(), which the on-disk schema genuinely requires.The sharp edge
The contract is byte-identical JSON on disk. The old helper returned
Nonefor "no journal" and for "messages already equals the journal's active branch", and the caller then serialized the original — leaving ametadata.message_countthat disagrees withmessages.len()exactly as it was.The in-place version must return before recomputing that count, or every save silently edits live data. The design review flagged that nothing in the suite would catch this, so a test now does, and it was verified to fail without the early return:
T2 is deferred, and not because of effort
Event::SessionUpdatedhas exactly one runtime consumer, and it moves theVec<Message>intoApp::api_messages— aVecmutated in place by push/pop/truncate/clear and referenced across 45 files. AnArcin the event would just relocate the same copy into ato_vec()at the consumer, and force the engine to rebuild the Arc on everyAppendLog::push.Making T2 a real win means reshaping
App::api_messagesitself. That is not one reviewable slice, and doing half of it would be worse than not starting.Also deliberately out of scope:
create_saved_session_with_id_mode_and_stamps's doubleto_vec()(costs 2N clones in any form — the struct holds two representations of the same history; removing it is a schema change), andupdate_session's element-wise compare (not on the debounced path, and it is the append-vs-rebranch decision).Verification (macOS aarch64, source
21a02f1f0)No-Issue: #6214 T3 only; T2 is deferred with reasons above, so the issue stays open.