perf(tui): hand tool-output rows back as a shared handle (#6213 T1)

[Hmbown]

Summary

Part of #6213 (item T1). The points below are what changed and what did not.

No-Issue: partial work on #6213 — T1's shared-handle half only; the id-keying half is not implemented, so that issue must stay open.

What changed

The tool-output rows cache exists so a finalized tool cell is not re-wrapped on every frame. A cache hit still deep-copied every String and styled span in the payload — the one thing the cache was supposed to make free, on the 120 FPS path the module's own docs describe.

The rows now live behind an Arc<Vec<OutputRow>>, and a hit hands back a refcount bump instead of a copy. The cache still stores one owned copy per (content_hash, width); only the hand-off changed.

The single production caller derefs unchanged: all_lines.iter(), &all_lines[idx], and &all_lines into selected_output_indices all still resolve through Arc's Deref to Vec to slice, so crates/tui/src/tui/history/tool_output.rs needed no edit at all. The existing cache tests also pass untouched, including cache_hit_returns_cached_rows, which asserts the second call does not recompute — the property this change is about.

What deliberately did not change

T1's second half — keying the cache by tool-result id so the per-frame hash_str(output) over the whole output disappears — is not here. It needs the tool-result id plumbed down to the render call, and doing it half-way would leave two keying schemes in the same cache. I am not claiming T1 is done.

Verification

cargo check -p codewhale-tui --all-targets --all-features --locked   (clean)
test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 12799 filtered out
    (tui::output_rows_cache)
test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 12800 filtered out
    (tui::history::tool_output)

Local only, on macOS/aarch64 at ecbec3e44. No hosted CI claim. No benchmark was run, so the size of the win is argued from the removed allocation, not measured.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 1m 57s —— View job

---

Review: PR #6265 — hand tool-output rows back as a shared handle

Todo list

• Gather context (read diff, read file, check CLAUDE.md/AGENTS.md guidance)
• Understand request (code review of the diff)
• Review for correctness bugs
• Review for provider/model/route safety concerns
• Review for reuse/simplification
• Review for test coverage
• Review for security
• Post final review

Summary

Scoped, well-documented change. crates/tui/src/tui/output_rows_cache.rs now stores CacheEntry.rows as Arc<Vec<OutputRow>> and hands back Arc::clone on a hit instead of Vec::clone (deep copy of every String/styled span). This is exactly the fix the PR describes, and it matches what T1 asks for: same cache key and eviction policy, only the hand-off is cheaper.

I verified the PR's central claim myself: crates/tui/src/tui/history/tool_output.rs:373-413 (the only production caller) does need no edits — all_lines.iter(), &all_lines[idx], and &all_lines passed into selected_output_indices(&[OutputRow]) all still resolve through Arc<Vec<T>>'s Deref/unsized coercion to &[T]. No bug there.

Findings

1. Correctness — none found. The Arc::new(compute()) / Arc::clone(&rows) sequencing in get_or_compute_rows (lines 129-139) is correct: the entry stored in by_key and the value returned to the caller share the same allocation, and the early-return-on-hit path (Arc::clone(&entry.rows)) doesn't fight the borrow checker the way returning a &mut reference would have. OutputRow already derives PartialEq, Eq, so Arc<Vec<OutputRow>>'s PartialEq impl (and the assert_eq!(a, b) in the existing test) still typecheck.

2. Provider/model/route safety — not applicable; this diff touches only the render-side rows cache.

3. Reuse/simplification (minor) — GLOBAL_CACHE is a thread_local! RefCell, and the module comment directly above it (lines 176-179) says a !Sync cache is sufficient because the render loop is single-threaded. Given that, Rc<Vec<OutputRow>> would deliver the identical "handle instead of deep copy" win with non-atomic refcounting, which is strictly cheaper on the 120 FPS hot path this PR is optimizing — Arc's atomic increment/decrement is real, if small, overhead that buys thread-safety nothing here uses. Not a blocker (Arc is still correct, and it's a much bigger win than what it costs), just a candidate for a follow-up if T1's second half (id-keying) also touches this file.
Consider Rc instead of Arc →

4. Tests — existing tests (cache_hit_returns_cached_rows, capacity_evicts_oldest, etc.) cover the "second call does not recompute" property and still pass unmodified against the new return type, which is reasonable evidence for this change. One optional strengthening: cache_hit_returns_cached_rows currently only asserts calls == 1 and a == b (value equality) — it doesn't assert Arc::ptr_eq(&a, &b), so it wouldn't catch a regression that returned an equal-but-freshly-cloned Arc (e.g. via a .clone() that accidentally cloned the Vec instead of the Arc). Not required given how narrow the change is, but a one-line assert!(Arc::ptr_eq(&a, &b)) would directly pin the "handle, not copy" property the PR is about.

5. Security — not applicable.

Notes on the PR description

Accurate and appropriately scoped — it correctly states what's in vs. out of T1 (id-keying is explicitly deferred, not silently dropped), and the "no benchmark, win argued from the removed allocation" framing matches AGENTS.md's evidence policy (claims should match the evidence actually gathered).

No blocking issues. The Arc vs Rc point above is the only thing I'd consider before merge, and it's optional.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 1 file patches, sha256:00519d3473a66731069bf2b4c8f45ef8d2ccb4fd7846a7d7c2f0fe15e06826b1.

Pass 1: Single-file change in crates/tui/src/tui/output_rows_cache.rs switching CacheEntry::rows and the public get_or_compute_rows* returns from Vec<OutputRow> to Arc<Vec<OutputRow>>. By inspection of the supplied lines the change is semantics-preserving: the miss path constructs one Arc, stores one clone and returns the original (previously one deep clone() was stored), and the hit path returns Arc::clone(&entry.rows) instead of entry.rows.clone(). No mutation of the cached rows exists on either path (get_or_compute_indices only mutates the separate selected_by_limit map), so shared immutable aliasing introduces no behavioral change. No defect was demonstrated in the changed lines.

Assessment

Pass 1: I inspected every changed line and the surrounding unchanged control flow that is supplied (lines 94–174 for OutputRowsCacheInner, 176–236 for the thread-local wrapper and public entry points). The refactor is a pure ownership/aliasing change: (1) the hit path Arc::clone(&entry.rows) is a refcount bump equivalent to the old deep clone for all read-only uses; (2) the miss path let rows = Arc::new(compute()); let entry = CacheEntry::new(Arc::clone(&rows)); ... rows stores and returns handles to the same single allocation, exactly as the old rows.clone() + return-original did (in fact it removes the only remaining copy); (3) nothing in the module mutates CacheEntry::rows after insertion — get_or_compute_indices touches only selected_by_limit — so Arc's immutable sharing is safe and the #[derive(Clone)] on CacheEntry becoming shallow does not change any visible behavior (no clone site of CacheEntry exists in the supplied source). Arc rather than Rc is slightly redundant given the cache is thread-local, but the atomic refcount is negligible and the return type is public, so I do not treat it as a defect. Open items I could not verify here (no build, no tests, and the files were not supplied): the production consumer crates/tui/src/tui/history/tool_output.rs and the test modules of both files are outside the provided context, and the PR's cargo check/test results are untrusted, unreproduced claims. A return-type change from an owned Vec to Arc<Vec<...>> would fail to compile for any consumer that mutates the rows in place or writes assert_eq!(rows, vec![...]) (no PartialEq<Vec<T>> for Arc<Vec<T>>); auto-deref keeps rows.iter(), &rows[idx], and &rows → &[OutputRow] coercion working, so read-only callers are unaffected. That residual risk is a compile-time one in code I was not given, not a defect I can assert from the diff, and should be confirmed by the reported cargo check --all-targets on the head commit.

---

Advisory review by Codewhale (codewhale review --pr 6265 --post, head 6447e6342ba2b1617335cd6182032a02652a8507). 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.