Repository navigation
perf(tui): reuse unchanged partial tool input - #114
JensenChen28 wants to merge 2 commits into
Conversation
Signed-off-by: c24894 <chen.zhusen@h3c.com>
4be0957 to
2c5c1d6
Compare
asto18089
left a comment
There was a problem hiding this comment.
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:509uses.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-targetsfor exactly this reason. At review time nolint/testcheck-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) pinsls crates/tui/tests/*.rs | wc -lto 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 thetests/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_closersdeletes 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_withcheck (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_inputonly 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.
Signed-off-by: c24894 <chen.zhusen@h3c.com>
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 wasc08c8c022; the PR was subsequently rebased ontocd8fd0b65. 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.
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 -- --checkcargo clippy --workspace --all-targets --all-features --locked(warning-free under the CI allow list)cargo test --workspace --all-features --lockedAdditional completed checks:
cargo check -p codewhale-tui --lib --locked: passed.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.()in that extraction harness; the public Engine tests compile actual production types.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
Review follow-up
tests/integration/partial_input_engine.rsand registered the existing shared harness; no root-level test binary remains. The moved suite is unchanged apart from replacing.err().expect(...)with.expect_err(...).cargo fmt --all -- --checkand diff whitespace checks pass.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.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_constantspassed on Windows. This compiles and lints the consolidated test target; it is not a claim that every workspace target was linted locally.