Repository navigation
perf: cross-hub patch ships a string SPLICE, not the whole value (chat half of #1284/#1351) - #1414
Merged
Merged
Conversation
Closes the chat half of the quadratic-write class (#1284, #1351). The activities half (#1398/#1399/#1400) moved growing log content to satellites; chat cannot take that treatment — a streaming cell must show all the text so far, and Thread.Messages already holds only ids (~10 kB of head at 200 messages, two orders of magnitude off being the problem). Chat's cost is the STREAMING WRITE. ThreadExecution streams through Sample(100 ms) into PushToResponseMessage, which rewrites the response cell's whole content per tick. Cross-hub that means ComputeMergePatchDiff emits the entire new text AND MeshNodePatchMerge.ExtractBaseValues emits the entire previous text, so a round costs O(ticks x final length) — measured 3.84 MB for a 20 kB answer, all of it LOH traffic on writer and owner both. A changed string leaf >= 1 kB whose change is smaller than the value now travels as patch { "$sd": [start, removedLength, "inserted"] } BaseValues { "$sdb": [baseLength, "fingerprint"] } Both halves are required: splicing only the patch leaves the base re-shipping the previous value every tick and the round stays quadratic. The owner applies a splice ONLY when the fingerprint (length + truncated SHA-256) proves its live text is the text the writer diffed against — then the result is byte-identical to what the whole-value patch would have written. On mismatch it REFUSES: keeps the newer live value and NACKs Conflict, which re-runs the writer's lambda against fresh state and re-diffs. So two mirrors splicing the same string concurrently cannot interleave into corrupted text; the loser re-diffs. The cost is that a DISJOINT concurrent edit to a big string, which the full-value three-way merge rebased and landed, now costs a round trip — a rare extra hop against a constant per-tick, per-viewer megabyte. Unchanged below 1 kB and for arrays/scalars: arrays in particular still need their full base (MergeArray consumes it), so that door stays shut as #1351 established. Measured, real runs, two sizes (StreamingCellWriteByteCountTest): 5,000 chars / 50 ticks 20,000 chars / 200 ticks before wire bytes 257,788 4,031,152 after wire bytes 23,794 69,881 before bytes/char 51.6 201.6 (grows 4x) after bytes/char 4.8 3.5 (falls) 57.7x less at 20 kB, and the per-character cost no longer grows with length. Correctness, since chat is the most user-visible surface: the final text is byte-identical (asserted at 20 kB / 200 splices and on a real streamed round), every intermediate state a viewer sees is a prefix of the final answer and never shrinks, and a late subscriber reads the whole text. Rolling deploys: the marker rides inside the patch, so a new writer reaching a not-yet-upgraded owner produces a merged node that fails to deserialize — NACKed and never committed, and self-healing because the mirror never advanced. The alternative (a sibling property an old owner ignores) would instead have it ACK a write it only partly applied — the acked-write-loss class of #648. A visibly stalled field beats a silently lost one. Not addressed here, and stated rather than implied: the owner->subscriber fan-out (JsonSynchronizationStream.CreateSingleObjectPatch, RFC 6902) still replaces the whole string per tick per subscriber, and #1351's MessageDelivery.Package double materialisation is a ~2x transport constant. Both are separate wire formats with separate consumers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces cross-hub streaming write amplification for large string fields by encoding big-string changes as compact “splices” instead of re-sending whole string values (and whole base values) on every tick—targeting the chat/streaming-cell path behind #1284/#1351.
Changes:
- Emit large-string diffs as
PatchStringSplicemarkers fromComputeMergePatchDiff, and extract compact base fingerprints for those leaves. - Resolve splice markers owner-side during three-way merge (
MeshNodePatchMerge) and on the base-less RFC7396 fallback apply path (MergePatchRecursive) so markers are never persisted verbatim. - Add focused unit + integration tests for splice correctness and streaming-wire-cost shape; document behavior in Architecture + What’s New.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Threading.Test/StreamingCellWriteByteCountTest.cs | Measures cross-hub PatchDataRequest payload size during simulated streaming writes to ensure cost scales ~linearly. |
| test/MeshWeaver.Threading.Test/StreamedTextIntegrityTest.cs | End-to-end streamed round through ThreadExecution asserting prefix monotonicity, final-byte identity, and late-subscriber correctness. |
| test/MeshWeaver.Data.Test/PatchStringSpliceTest.cs | Unit coverage for splice encoding/decoding, refusal semantics, base extraction fingerprinting, and base-less fallback behavior. |
| src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs | Produces splice markers for eligible large-string leaf changes in RFC7396 merge patches. |
| src/MeshWeaver.Data/Serialization/PatchStringSplice.cs | New codec for $sd splice markers + $sdb base fingerprint markers (length + truncated SHA-256). |
| src/MeshWeaver.Data/Serialization/MeshNodePatchMerge.cs | Applies splice markers safely (fingerprint-verified) and emits compact base fingerprints for spliced leaves. |
| src/MeshWeaver.Data/DataExtensions.cs | Ensures base-less merge apply path decodes splice markers instead of writing marker objects into string fields. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-long-answer-no-longer-costs-more-than-it-says.md | User-facing release note describing the reduced cost of streaming long answers. |
| src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md | Architecture doc update explaining splice wire shape, refusal semantics, and rolling-deploy behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
…space
Both from the Copilot review, both worth taking.
PatchWireRecorder called its metric "bytes" while summing string.Length — a UTF-16
char count. For this test's ASCII prose the two coincide, so the conclusion never
moved, but the payload travels as UTF-8 and a char count silently understates
anything non-ASCII, which real chat is full of. Now Encoding.UTF8.GetByteCount, and
both columns were re-measured under it:
5,000 chars / 50 ticks 20,000 chars / 200 ticks
before wire bytes 257,788 4,031,144
after wire bytes 23,795 69,893
before bytes/char 51.6 201.6 (grows 4x)
after bytes/char 4.8 3.5 (falls)
StreamedTextIntegrityTest created its thread against Mesh.Address. That is the root
mesh hub; the majority of the thread tests (ThreadCreationTest, the Orleans thread
suite, DelegationCompletionTest) target the context namespace, which is also what a
real client does and what keeps the test representative. Switched to
new Address(ContextPath).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Contributor
Author
|
Both taken — thanks, the first one in particular was a real precision bug in the measurement's own metric. UTF-8 byte counts.
Thread creation target. Correct that Both in |
Contributor
Contributor
This was referenced Aug 30, 2026
rbuergi
added a commit
that referenced
this pull request
Aug 30, 2026
… the first (#2654 owns the residual) (#2744) AcceptsStringSplice — the one negotiated capability of the owner→subscriber fan-out (#1284/#1414) — was set at exactly ONE of the three sites that post a SubscribeRequest: the initial subscribe. Resubscribe (owner recycle / StreamEndedEvent) and SynchronizationStream.RequestFreshSnapshot (the frame-loss resync) both omitted it. When the owner cannot match a re-subscribe to a live server-side stream it BUILDS a fresh one and reads the capability off THAT request — which is exactly what an eviction on the router's TargetUnserved verdict (#2620), an owner recycle or a torn-down sync hub produces. From that cycle on, the owner fans out whole-string `replace` frames to a mirror that understands splices: the per-subscriber quadratic #1284/#1414 removed, reinstated silently and permanently, on precisely the paths that fire during a resync storm. Nothing fails — a replace is a shape every subscriber understands — so no assertion on the FRAMES could catch it; the only place it is visible is the request. Fixed at the root, not with a third literal: JsonSynchronizationStream.MintSubscribeRequest is now the one place a C# subscriber mints a SubscribeRequest, because a capability is a property of the applier this assembly ships, not of the moment one post was written. Nothing else changes — the resync still posts with no identity and the same Subscriber, the recycle re-ask still with SystemUserId. ResubscribeKeepsNegotiatedCapabilitiesTest drives the real resync (the deterministic one-frame wire loss StreamFrameLossResyncTest already uses) and asserts every posted SubscribeRequest declares the capability. Verified RED on the unfixed tree, GREEN with the fix; MeshWeaver.Data.Test 394/394. Docs: DataSyncAndCrdt §6 now documents the BasedOnVersion chain and states how to read `[SYNC_STREAM] Frame loss detected` — a RESYNC counter, not a data-loss counter. The raw total diagnoses nothing; the per-stream count and whether a Full follows do, and the driver is almost always upstream (a latched stream attach #2633/#2692 → #2645, the #2620 eviction cycle, an owner recycle). That is the recorded disposition of #2641's 847-lines-in-30-min. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes the chat half of the quadratic-write class — #1284, #1351. The activities half shipped in #1398/#1399/#1400 (bounded window + segment satellites).
Chat is a different shape, and the satellite migration does not apply
Thread.Messagesalready holds only ids (~26 B each): a 200-message thread is ~10 kB of head against the activity log's 88.7 kB at 500 — two orders of magnitude from being the problem. Message bodies are alreadyThreadMessagesatellites. And a streaming cell must show all the text so far, so its value legitimately grows; no satellite can change that.The cost is the streaming write, not the content shape.
ThreadExecutionstreams throughSample(100 ms)intoPushToResponseMessage, which rewrites the cell's whole content per tick. Cross-hub that is a double whammy:ComputeMergePatchDiffemits the entire new text, andMeshNodePatchMerge.ExtractBaseValuesemits the entire previous text next to it.So one round costs
O(ticks × final length). Measured below: 3.84 MB for a 20 kB answer, all of it Large-Object-Heap traffic on the writer and the owner. WideningSample(100 ms)or dropping ticks would be the band-aid; the patch simply does not need to carry text the owner already has.The change
A changed string leaf at least
PatchStringSplice.MinSpliceLength(1 kB) long, whose change is smaller than the value, now travels as a splice:{ "$sd": [start, removedLength, "inserted"] }BaseValues, same leaf{ "$sdb": [baseLength, "fingerprint"] }Both halves are load-bearing — splicing only the patch leaves the base re-shipping the previous value every tick and the round stays quadratic.
Three files carry it: the producer (
ComputeMergePatchDiff), the base extractor + owner-side resolver (MeshNodePatchMerge), and the base-less RFC 7396 fallback (MergePatchRecursive, so a marker can never be written verbatim into a string field on any apply path). New codec:PatchStringSplice, next to the existingStringDelta/StringDeltaPatchit shares the$sdmarker with.Below 1 kB, and for arrays and scalars, nothing changes. Arrays in particular still ship their full base —
MergeArraygenuinely consumes it, so that door stays shut exactly as #1351 established.Measured: bytes serialised per streaming round
Real runs of the production cross-hub write path (
GetMeshNodeStream(path).Update→ shared stream cache →PatchDataRequest→ owner merge), counting the actualPatchDataRequestpayloads as the owning node hub receives them, at two answer sizes. The "before" column is the same test againstmain's code, not a model — it fails there and passes here.The shape assertion is the important one: quadrupling the answer must not quadruple the cost per character. Before, it does exactly that (51.6 → 201.6); after, the per-character cost falls as the per-tick constant amortises.
Correctness — chat is the most user-visible surface in the product
The owner applies a splice only when it can prove the offsets still address the right characters. The fingerprint (exact length + truncated SHA-256) is compared against the owner's live text:
Conflict— which re-runs the writer's update lambda against fresh state and re-diffs, the same self-healing route a refused scalar already takes.Concurrent splices therefore cannot corrupt the string. A splice is never applied at an offset the fingerprint did not vouch for, so two mirrors splicing the same text do not interleave: the first lands whole, the second is refused whole and re-diffs. Stated plainly rather than left implicit — this is not "last splice wins on that field"; it is "the loser never lands, and is told to start again from current".
The cost is honest and worth naming: a disjoint concurrent edit to a big string, which the full-value three-way merge would have rebased and landed in one hop, now costs a round trip. For streaming cells, markdown bodies and prerendered html that trades a rare extra hop against a constant per-tick, per-viewer megabyte.
Tests:
PatchStringSpliceTest(15) — exactness against the true base; 200 sequential splices reproducing the whole streamed text; fingerprint mismatch refuses rather than splicing blind; two mirrors concurrently; non-string live value; no base signal; a full base carried with a splice still gets the three-way rebase; the strict decoder; and both fallback-path cases.StreamingCellWriteByteCountTest— the measurement above, plus a byte-identity assertion on the settled cell at 20 kB / 200 splices.StreamedTextIntegrityTest— a real streamed round throughThreadExecutionwith a 6 kB answer over 24 chunks: the finished text is byte-identical to what the model streamed, every observed state is a prefix of it and never shrinks, and a late subscriber on a fresh client reads the whole text. (The framework's short pre-stream placeholders are exempt by a length bound, never by matching their strings — they are localizable, and a length rule cannot excuse a long corrupted state.)Rolling deploys — why the marker rides inside the patch
During a rollout a new writer can reach a per-node hub still on the old image. That owner does not decode
$sd, merges the marker into a string-typed field, and the merged node then fails to deserialize — the write is NACKed and never commits. Loud, non-destructive, and self-healing: the mirror never advanced, so the next splice is computed against the same base and lands the whole missing span once the owner is upgraded.The obvious alternative — carrying splices in a new sibling property an old owner would simply ignore — degrades instead into an owner that acks a write it only partly applied, i.e. the acked-write-loss class of #648. A visibly stalled field beats a silently lost one, so the in-patch marker is the deliberate choice.
Deliberately out of scope, stated rather than implied
JsonSynchronizationStream.CreateSingleObjectPatchstill emits an RFC 6902replacecarrying the whole string, per tick, per subscriber. Different wire format, different consumers (including the JS clients) — its own change.MessageDelivery.Packageserialises the payload to a string on the root-mesh route and the Orleans codec transcodes it back to UTF-8 through a doublingPooledByteBufferWriter. Real waste, but a transport concern rather than a patch-shape one; folding it in here would have made this measurement ambiguous. With the payload now ~58× smaller it is no longer material for chat, and it should be fixed where it matters — large payloads generally.Thread.SessionStateis declared (src/MeshWeaver.AI/Thread.cs:173) and never read or written anywhere — the only hit insrc,test,clients,samplesandcontentis the declaration itself. Left in place rather than deleted: it is a public property on a content record, and in-mesh C# is invisible to the compiler (AGENTS.md), so removing it belongs in a change that can own that sweep.Verification
Local,
-c Release -warnaserroron every touched project and their test assemblies:MeshWeaver.Data.TestMeshWeaver.Graph.TestMeshWeaver.Threading.TestMeshWeaver.Hosting.Monolith.TestMeshWeaver.Documentation.Test🤖 Generated with Claude Code