Skip to content

perf(tui): reuse unchanged partial tool input - #114

Open
JensenChen28 wants to merge 2 commits into
pinvou3-cleanfrom
codex/perf-tool-input-reuse
Open

JensenChen28 wants to merge 2 commits into
pinvou3-cleanfrom
codex/perf-tool-input-reuse

Conversation

@JensenChen28

@JensenChen28 JensenChen28 commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

No-Issue: This performance optimization was requested and validated directly through the A/B investigation and this PR, rather than an issue-reported defect.

Summary

Streaming long tool arguments currently reparses the entire accumulated prefix on every JSON delta. Keep a per-tool, suffix-only proof and reuse the previous parse result only while a valid unfinished string and the selected raw fallback segment are unchanged. Preserve both the owned partial input and the original Some/None disposition used by logging.

All uncertain or unsupported input continues through the existing parser. Invalidate before block-stop finalization and schema normalization, and initialize fresh state for every tool block, including reused provider IDs. Parser repair, final argument selection, permission handling, tool execution and retry policy remain unchanged. The module documents the per-delta evaluation it replaces.

This targets pinvou3-clean. The original App-consumed baseline was c08c8c022; the PR was subsequently rebased onto cd8fd0b65. The App gitlink is unchanged.

Performance evidence

Local synthetic release measurements of the extracted production handler/parser/finalizer, with production-locked serde_json 1.0.154 and thiserror 2.0.21 on Windows MSVC. Nine samples alternate baseline/candidate order and assert output for every sample; these are parameter-processing times, not whole-turn or model latency.

Content / 64-byte fragments Baseline P50 Candidate P50 Reduction
64 KiB plain text (65,557 serialized bytes) 396.858 ms 0.439 ms 99.89%
64 KiB source text (72,525 serialized bytes) 907.310 ms 315.243 ms 65.26%
256 KiB source text (290,041 serialized bytes) 12,965.456 ms 5,416.613 ms 58.22%

Separate 101-sample fallback confirmations at approximately 16 KiB show 20.332→20.480 ms for fenced input and 22.637→22.903 ms for arrays. Unsupported inputs therefore retain baseline cost with a small measured overhead. Mirror reads remain immediate; no queued replay is introduced.

Testing

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features --locked (warning-free under the CI allow list)
  • cargo test --workspace --all-features --locked

Additional completed checks:

  • cargo check -p codewhale-tui --lib --locked: passed.
  • Before harness consolidation, cargo test -p codewhale-tui --test partial_input_engine --locked -- --nocapture --test-threads=1: 6 passed; 0 failed on the unmodified baseline and 6 passed; 0 failed after forcibly rebuilding the candidate package. Dep-info and binary source markers confirm the candidate scanner was compiled; baseline and candidate binary hashes differ.
  • Native scenarios cover fragmented/interleaved tool calls and history, malformed final input, cancellation followed by a fresh same-ID call, transparent initial retry, headless transport recovery, and interactive partial-tool error feedback.
  • A private exact-source extraction harness runs the committed production differential tests plus the original repair tests: 19 passed; 0 failed after the review follow-up. Only unused caller metadata is represented as () in that extraction harness; the public Engine tests compile actual production types.
  • Removing raw-segment stability or cached Some/None handling causes differential assertion failures; restoring the implementation passes.
  • Diff whitespace checks and specification/code-quality self-review passed.

Full workspace tests and Clippy were not run locally. The original monolithic lib-test baseline failed with memory allocation failure on this desktop; focused native tests used one Cargo job without debug info or incremental compilation. Broad CI validation remains required before merge.

Compatibility notes

The existing interactive stream-error path may send its retained partial mirror through ordinary tool validation/feedback before surfacing the original stream error. The regression scenario preserves that behavior: the synthetic incomplete writer fails content validation without creating a file, the complete follow-up reader succeeds, and the original stream error remains terminal. This optimization does not claim to harden partial-tool dispatch or change that policy.

Checklist

  • This PR adds a private module; it names the repeated per-delta parsing it replaces.
  • Updated explanatory comments and ownership/invalidation obligations.
  • Added differential and public Engine lifecycle tests.
  • TUI manual verification: not applicable; no UI changes.
  • Harvested/co-authored credit: not applicable; no external patch harvested.

Review follow-up

  • Folded the six public Engine scenarios into tests/integration/partial_input_engine.rs and registered the existing shared harness; no root-level test binary remains. The moved suite is unchanged apart from replacing .err().expect(...) with .expect_err(...).
  • Added assertions for reuse of both parser dispositions, changed-segment fallback, stale length/suffix rejection and explicit invalidation. Added adversarial chunk boundaries for fence stripping and escape-run merging.
  • Five private mutations (never reuse, stale-buffer guard removed, backtick rule removed, invalid-escape rule removed and invalidation removed) all fail assertions; the restored exact-source harness passes 19 tests.
  • cargo fmt --all -- --check and diff whitespace checks pass.
  • The consolidated native Engine test command is now cargo test -p codewhale-tui --test integration partial_input_engine --locked -- --nocapture --test-threads=1. The local Windows rebuild failed with an LLVM out-of-memory error while compiling the TUI library, before running tests. This is not reported as a local test pass.
  • On the preceding head 2c5c1d6bb, Linux fork CI reported 14,855 tests passed and 15 skipped. Those receipts do not cover this follow-up commit; its new CI result must be checked separately.

Targeted follow-up lint: cargo clippy -p codewhale-tui --test integration --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or -A clippy::collapsible_if -A clippy::assertions_on_constants passed on Windows. This compiles and lints the consolidated test target; it is not a claim that every workspace target was linted locally.

Signed-off-by: c24894 <chen.zhusen@h3c.com>
@asto18089
asto18089 force-pushed the codex/perf-tool-input-reuse branch from 4be0957 to 2c5c1d6 Compare October 20, 2026 14:55

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh round-1 review — rebased onto latest pinvou3-clean

Rebased onto the new base cd8fd0b65 (#112 landed after this PR was opened). Zero conflicts; the result is patch-identical to the original head (per-file ±line multisets equal on all 7 files; changed-line bytes md5-equal). Rebased head pushed as 2c5c1d6bb (tree 460aa3a55, byte-exact against the local rebuild). 12-shard fresh review (8 analysis + 3 adversarial/fuzz + coordinator), mutation-tested.

Verdict: CHANGES_REQUESTED — two mechanical gate/policy items (M1, M2). The optimization itself is sound, useful, root-caused and cleanly built; everything else below is non-blocking.

The four questions

1. Useful / complete / root-caused — yes. The baseline O(n²) is real: every InputJsonDelta ran parse_tool_input on the full accumulated prefix, and the May 2026 arg-repair ladder (c018afdf8/1489cfbb5) raised the per-delta constant to ~7 serde attempts + ~8 O(n) allocations. I independently replicated the base cost (256 KiB source text × 64-byte deltas → 2.32 s on arm64 release; the author's 12.9 s P50 on Windows MSVC is order-consistent). The fix removes the dominant cost for the object-string case (replicated scanner feed: 159 µs per 256 KiB, ~0.007% of base) and honestly discloses what stays at baseline (arrays/fenced/double-encoded; backtick content caps at ~60%). The engine has exactly one per-delta tool-JSON parse site; all decoders (chat.rs / responses.rs / acp_server) accumulate and parse once — the claim is complete, nothing left unfixed in this class.

2. Elegant — yes. The suffix-only certificate is the minimal sound design: reuse fires only in the one state (ends inside an unfinished string) where every repair rung provably fails, so the disposition is a pure function of the raw balanced segment, whose selection the scanner mirrors byte-for-byte (including the root-over-array precedence and the no-array-fallback-on-parse-failure subtlety). Placement follows dependency direction (core/engine/ beside dispatch); the third quote/escape tracker is justified (it must mirror extraction semantics, not repair semantics); every doc-comment line I checked matches the code. Nothing reinvents an existing wheel.

3. Zero smuggled changes. 15/15 hunks classified: 8 feature / 4 test-file / 3+… mechanical (mod decls, field inits, log-variable swap). Whole-diff deletions are exactly the 3 replaced parse lines. No Cargo/CI/doc/submodule changes; all new symbols confined to the 7 files; DCO matches author; PR-body claims verified (serde_json 1.0.154 / thiserror 2.0.21 in Cargo.lock; "16 passed" = 3 committed differential + 13 arg_repair in the private harness — accurately described; 6 integration tests real and non-trivial).

4. No feature or performance bugs found. Three independent adversarial campaigns against the certificate found zero counterexamples: ~7.2M randomized differential events (dispositions and mirror values at every prefix), ~2.3M more over exhaustive depth-4 alphabets plus 8 targeted mutants, and the committed ~9,400-checkpoint harness. The soundness proof is subtler than the module docstring suggests — see m1 — but it holds. Behavior parity vs base verified: identical input values over time, identical log sequence, final_tool_input/input_parse_error untouched, stream-error partial-mirror flow matches the Compatibility notes.

MAJOR

  • M1 — the PR's own clippy gate fails on PR-added code. crates/tui/tests/partial_input_engine.rs:509 uses .err().expect(..) → clippy::err_expect. Reproduced locally with the exact CI command (cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings -A uninlined_format_args -A too_many_arguments -A unnecessary_map_or): error: called '.err().expect()' on a 'Result' value … help: try: 'expect_err'. Note the ci.yml lint job explicitly lints --all-targets for exactly this reason. At review time no lint/test check-runs existed on the head, so this has not surfaced on GitHub yet — it will fail the required gate. One-line fix.
  • M2 — new root-level test binary violates the documented harness-consolidation invariant. crates/tui/tests/README.md (lane codewhale-hq#5247) pins ls crates/tui/tests/*.rs | wc -l to 0: every root-level binary was a full-TUI-graph link job, which is the build-time cost codewhale-hq#5247 deleted. This PR re-adds one (tests/partial_input_engine.rs). Fix: fold the 6 tests into the tests/integration/ harness as a #[path] module (the README documents the pattern and per-module filtering), or extend the README with a justified exception in the same PR. Mechanical.

MINOR

  • m1 — the two load-bearing soundness facts are exactly where the differential fixtures are thinnest. "Repair cannot close a valid unfinished string" is not trivially true: strip_excess_closers deletes closers quote-unaware, and a deletion between backslash runs merges them and flips parity ({"k": "\]\" → \\" closes the string → repair succeeds with a different value than the segment fallback). What saves the certificate is the invalid-escape disabler — proven load-bearing by mutant counterexample, as is the backtick rule (fence-line between a closed raw root and a trailing open string: {"a": "}"}\n```z"). Neither tight shape is in the fixture list (differential_tests.rs:57-78). Recommend adding both fixtures so the certificate's actual foundations are pinned, not just its periphery.
  • m2 — the reuse() stale-buffer guard is unpinned. Disabling the length/ends_with check (mutation: all 14 tests stay green) is invisible repo-wide. Correct-by-audit today (single append site at turn_loop.rs:6101), and the doc-comment honestly states the equal-length-prefix blind spot — but a future refactor that violates append-only would not be caught by any test.
  • m3 — both invalidation calls are currently unreachable-effective. Removing invalidate_input_parse_hint() at turn_loop.rs:6180 and ~2743 (mutation) leaves all tests green: nothing reads or refreshes the hint after those points (refresh_partial_input only runs from the delta handler; stop-time finalization re-parses the full buffer unconditionally). Harmless defensive enforcement of the documented obligation — but as written the PR body's invalidation clauses are satisfied by code no test can distinguish. A one-line comment or a targeted pin would make the intent observable.
  • m4 — effectiveness is unguarded. A hint that never fires (or over-disables) ships green (mutation: pure-fallback hint passes everything, only runtime changes). Differential testing structurally can't do better, but a cheap parse-counter assertion in the harness would pin that the optimization engages.
  • m5 — the one surviving per-delta O(n) in this path is the verbose-gated whole-buffer format! at turn_loop.rs:6110-6114 (pre-existing, disclosed in the in-code comment). Out of scope here; flagging since this PR is the one that made the neighboring O(n²) visible.

NIT (compact)

Sticky backtick disable permanently reverts a block to base cost (conservative; the source-text numbers already reflect it). Segment::Root(usize) vs Array(usize, usize) asymmetry is correct (root start pinned by starts_object) but unexplained — one comment would stop a future "symmetry fix". The non-{ early return leaves self.bytes stale (harmless: disabled short-circuits first). is_ascii_whitespace ⊃ serde's whitespace set (\x0C) — verified harmless. Duplicated mock boilerplate in the integration file; T2's comment slightly overstates in-harness reused-ID coverage (actually covered by integration test 3); PR body's "current head is c08c8c0" sentence is now stale (superseded by this rebase).

Pre-existing on the new base — not this PR's, for the record

Under current stable clippy with the exact CI flags, the base cd8fd0b65 itself fails collapsible_if (turn_loop.rs:5409) and cloned_ref_to_slice_refs (repo_law.rs:638) — both verified present without this PR (likely newer-clippy extensions firing on old code). The full-lib flaky set (remote_control family ± forkguard_model_operation_auxiliary_retry_keys) fails in parallel runs and passes isolated on both base and PR — known environmental, not PR-attributable.

Verification summary

Gates on the rebased tree: cargo fmt --all -- --check clean; differential 3/3 + integration 6/6 green; full lib 12,2xx green modulo the environmental set above; clippy fails only on M1 (PR) + the two base-attributed lints. Mutations personally run: segment-stability removal → differential red (teeth); stale-buffer guard off → all green (m2); invalidations off → all green (m3). Soundness: hand-proofs of ladder-cannot-close-string + segment-mirror equivalence, plus the three fuzz campaigns above.

Comment thread crates/tui/tests/partial_input_engine.rs Outdated
Comment thread crates/tui/tests/integration/partial_input_engine.rs
Comment thread crates/tui/src/core/engine/partial_input_differential_tests.rs
Signed-off-by: c24894 <chen.zhusen@h3c.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants