perf(core): hash each context block once per turn, not once per step#732
Draft
macanderson wants to merge 1 commit into
Draft
perf(core): hash each context block once per turn, not once per step#732macanderson wants to merge 1 commit into
macanderson wants to merge 1 commit into
Conversation
The last named follow-up from #725. Each tool result was JSON-serialized and SHA-256'd twice per step and each block's content hashed twice more, all unconditionally, all over the whole transcript: - `decompose` derived every block's `block_id` AND `content_digest` on every step — two SHA-256 passes over overlapping preimages (`kind_tag ‖ NUL ‖ content` and `content`), which cannot share a hash state — plus a full character count for `token_cost`, plus a `serde_json::to_string` of every tool call and tool result just to obtain the preimage. - `snapshot_result_identities` derived every uncompacted tool result's identity on every step, each one a serialization plus a hash. The manifest has to NAME every block each step; the bytes behind an unchanged block do not change, so naming it again is the only part that has to repeat. Both are now memoized by `(message_index, ordinal)`. A position is a stable name for bytes only while nothing rewrites what is already there, so `TranscriptRevision` counts in-place rewrites as distinct from appends. It is bumped where the mutation happens — compaction's pure passes, and the overflow summarizer, which is bumped whenever it RUNS rather than only when it splices, because over-invalidating costs one turn's memo on a rare path while under-invalidating serves a digest for bytes that moved. `run_compaction_pass` reports that through a `#[must_use] CompactionPass` rather than a `&mut` out-param: it was already at clippy's argument limit, and making the return impossible to discard turns "remember to invalidate" from a convention into a compile error. Also drops a copy nothing needed: `BlockDraft::content` was an owned `String` for gap kinds, so the system prefix — the largest gap block there is — was re-cloned on every step to hand to a `BlockRegistered` that had already fired. It borrows now, and copies at the one site that sends it. Measured, with a counter at the single choke point every hash in the module funnels through. Doubling a turn's steps takes hashing from 24 to 44 passes (linear); before it was 64 to 188 (superlinear). An 8-step turn drops from 188 hashes to 44, and the gap widens with turn length. Five witnesses, each verified to FAIL when the thing it guards is removed — which mattered, because two of them did not at first: - `per_step_hashing_grows_with_the_turn_not_with_its_square` — fails with the memo disabled (ratio 2.94 vs 1.83). - `a_rewritten_block_is_never_served_from_the_digest_memo` — the ledger's contract; fails with invalidation disabled. - `compaction_mid_turn_invalidates_the_receipt_ledgers_digest_memo` — the WIRING. My first attempt at this asserted the wrong thing and passed even with the driver's signal deleted, meaning the memo could have shipped serving stale digests on every compacting turn with a green suite. It now drives a real turn over a tight budget and fails when `revision.rewritten()` is removed. - `every_rewrite_compaction_performs_is_recognized_as_compacted` — pins the cross-module invariant the identity memo's soundness actually rests on (see below). Also got this wrong first: the fixture evicted its aged results, so the elision marker never survived to be checked and the test passed either way. It now ages without evicting, and fails if `is_compacted_output` stops recognizing aging. - `a_step_walks_the_transcript_to_estimate_it_at_most_twice` (from #725) still holds. Stated rather than glossed: clearing the identity memo is belt-and-braces, not load-bearing. Every rewrite leaves content `is_compacted_output` recognizes, and the sweep skips a recognized result before deriving anything, so a stale entry there is unobservable today. That is why disabling its clearing breaks no test, and it is documented at the memo alongside the invariant test, rather than left to look like missing coverage. Structural: the driver's loop-detection evidence (`ResultIdentities`, `snapshot_result_identities`, `recent_call_records`, `comparable_output`, `call_identity_key`) moves to `driver/loop_evidence.rs`. That is the file-size gate asking for a module instead of a raised baseline, and it takes driver.rs from 2256 to 2078 lines — below its recorded ceiling, so no baseline moves. Still not done, and conditional rather than per-step: `compaction::compact` recomputes every tool result's pre-mutation `block_id` when it runs. Sharing the ledger's memo would couple a pure module to receipt state, and it only fires on an over-budget step, so it stays as it is.
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The last named follow-up from #725. Each tool result was JSON-serialized and SHA-256'd twice per step, and each block's content hashed twice more — all unconditionally, all over the whole transcript:
decomposederived every block'sblock_idandcontent_digeston every step. Those hash overlapping preimages (kind_tag ‖ NUL ‖ contentandcontent) and so cannot share a hash state. Add a full character count fortoken_cost, and aserde_json::to_stringof every tool call and tool result just to obtain the preimage.snapshot_result_identitiesderived every uncompacted tool result's identity on every step — another serialization plus another hash, again transcript-wide.The manifest has to name every block each step. The bytes behind an unchanged block do not change, so naming it again is the only part that has to repeat. Both are now memoized by
(message_index, ordinal).A position is a stable name for bytes only while nothing rewrites what is already there, so
TranscriptRevisioncounts in-place rewrites as distinct from appends. It is bumped where the mutation happens — compaction's pure passes, and the overflow summarizer. The summarizer bumps whenever it runs rather than only when it splices: over-invalidating costs one turn's memo on a rare path, under-invalidating serves a digest for bytes that moved.run_compaction_passreports that through a#[must_use] CompactionPassrather than a&mutout-param — it was already at clippy's argument limit, and making the return impossible to discard turns "remember to invalidate" from a convention into a compile error.Also drops a copy nothing needed:
BlockDraft::contentwas an ownedStringfor gap kinds, so the system prefix — the largest gap block there is — was re-cloned on every step to hand to aBlockRegisteredthat had already fired. It borrows now, and copies at the one site that sends it.Refs #546
The witness
main, passes here)Measured at the single choke point every hash in the module funnels through. Doubling a turn's steps takes hashing from 24 → 44 passes (linear); before it was 64 → 188 (superlinear). An 8-step turn drops from 188 hashes to 44, and the gap widens with turn length.
Five witnesses, each verified to fail when the thing it guards is removed — which mattered, because two of them did not at first:
per_step_hashing_grows_with_the_turn_not_with_its_squarea_rewritten_block_is_never_served_from_the_digest_memocompaction_mid_turn_invalidates_the_receipt_ledgers_digest_memorevision.rewritten()deleted ✗every_rewrite_compaction_performs_is_recognized_as_compactedis_compacted_outputstops recognizing aging ✗a_step_walks_the_transcript_to_estimate_it_at_most_twice(#725)The gate
cargo fmt --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace— 61 test binaries, 0 failuresmake checkgreen; no baseline moves (driver.rs went 2256 → 2078, below its ceiling)Ground-rule check
stella-core; no new depsAnything reviewers should know?
Two of my five tests were wrong in ways that would have shipped the bug. Both are worth knowing about because they're the failure mode this whole series is about — a green suite that proves nothing.
One claim I am deliberately not making. Clearing the identity memo is belt-and-braces, not load-bearing, and disabling it breaks no test. That is not missing coverage: every rewrite leaves content
is_compacted_outputrecognizes (three stubs, or the aging marker), and the sweep skips a recognized result before deriving anything — so a stale entry there is unobservable today. Rather than let that look like a gap, the cross-module invariant it depends on is pinned by its own test, and the reasoning is documented at the memo. A future pass that rewrote a result into something unrecognized would make it start serving pre-rewrite identities; the clearing is what keeps that a caught bug instead of a silent one.Still not done, and conditional rather than per-step:
compaction::compactrecomputes every tool result's pre-mutationblock_idwhen it runs. Sharing the ledger's memo would couple a pure module to receipt state, and it only fires on an over-budget step, so it stays.Structural churn is the file-size gate, not preference. The driver's loop-detection evidence (
ResultIdentities,snapshot_result_identities,recent_call_records,comparable_output,call_identity_key) moves todriver/loop_evidence.rs— the gate asking for a module instead of a raised baseline. It takes driver.rs from 2256 to 2078 lines, so unlike #725 this PR moves no baseline at all.Process note: this is rebased onto
006fec3e(#729), which landed while I was working. Worth a look that the loop-evidence extraction and #729's retrieval-plane restructuring don't overlap — they shouldn't, this PR touches onlystella-core.