fix(serialization): polymorphic object read — no JsonDocument + UTF-16 copy per nesting level (#5555 HEAPSTEP allocator) - #5767
Conversation
…m the reader — no JsonDocument + string copy per nesting level The [HEAPSTEP] sampler (#5665), live on ci.9408, names the same allocation mix in every heap step on both portals: String 53-56%, Byte[] 25-31%, JsonDocument 2-4%. That is one JSON value held as UTF-8 and again as UTF-16. ObjectPolymorphicConverter.ReadObject did exactly that for every object-typed value: JsonDocument.ParseValue (a byte[] copy), GetRawText() (a UTF-16 string), then Deserialize(string). Every nested object member re-enters the converter, so it repeated once per nesting level. - $type first + registered type + no reference metadata: deserialize from the reader directly (probe on a reader copy; declining leaves the reader untouched). - every other shape: the general path, now deserializing from UTF-8 instead of a string (JsonElementNormalizer.Deserialize replaces GetNormalizedRawText, also for the read-only collection converters). PolymorphicReadAllocationTest: one read of a value nested 8 levels with a 400,000-byte leaf allocated 4,006,624 bytes before, 402,024 after. Refs #5555, #4824 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the outstanding polymorphic metadata, duplicate $type, regression coverage, and immutable lookup concerns.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR optimizes polymorphic JSON deserialization by reducing intermediate allocations and adding allocation regression coverage.
Changes:
- Adds direct reader-based deserialization for registered
$typevalues. - Replaces string-based normalization with UTF-8 deserialization.
- Adds allocation tests and documents production findings.
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/PolymorphicReadAllocationTest.cs |
Verifies allocation reductions and fallback behavior. |
src/MeshWeaver.Messaging.Hub/Serialization/ReadOnlyCollectionConverterFactory.cs |
Uses UTF-8 normalization for collections. |
src/MeshWeaver.Messaging.Hub/Serialization/ObjectPolymorphicConverter.cs |
Adds the optimized polymorphic read path. |
src/MeshWeaver.Documentation/Data/Architecture/WhichKindOfSilence.md |
Documents heap findings and remediation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| foreach (var metadata in MetadataPropertyNames) | ||
| if (probe.ValueTextEquals(metadata)) | ||
| return false; | ||
| if (!probe.Read() || !probe.TrySkip()) | ||
| return false; |
There was a problem hiding this comment.
Added in 319acde: TypeFirstWithReferenceMetadata_IsStrippedAndTyped is a theory over $id, $ref, $values and $defs. Each case is a $type-first object that must come back typed, with the metadata stripped. Negative control: with the probe's metadata check disabled, all four cases go red (they come back as untyped JsonElement). With the check restored, all 7 tests in the class pass.
| return cleanedElement.Clone(); | ||
| } | ||
|
|
||
| private static readonly string[] MetadataPropertyNames = ["$id", "$ref", "$values", "$defs"]; |
There was a problem hiding this comment.
Fixed in 319acde: the static array is gone. The four names are now inline ValueTextEquals comparisons in the probe, so no collection is held at process scope.
…irst + reference metadata to the stripping path Negative control: with the probe's metadata check disabled, all four TypeFirstWithReferenceMetadata_IsStrippedAndTyped cases go red. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What the instrument named
[HEAPSTEP](#5665) is live on ci.9408 on both portals. It read 43 lines on memex-cloud and 88 on memex over 8 h, none cut (Ops/Actions/logs-{memexcloud,memex}-20260926-heapstep-5555). Every step past warm-up has the same mix:System.String53–56 %System.Byte[]25–31 %System.Text.Json.JsonDocument2–4 %String ≈ 2 × Byte[] plus a
JsonDocumentmeans one JSON value was held twice: once as UTF-8 and once as UTF-16.The steps are churn, not retention. On
…6c7669df84-nbrdx, tick 314 readheap=4.44GiB. Tick 315 ran 2 gen-0, 1 gen-1 and 1 gen-2 collections and read 3.03 GiB.The defect
ObjectPolymorphicConverter.ReadObjecthandled everyobject-typed value in three steps:JsonDocument.ParseValue, which copies the value into a byte[].GetRawText(), which builds a UTF-16 string.Deserialize(string)on that string.Every nested
objectmember re-enters the converter, so this repeats once per nesting level.The fix
$typefirst, a registered type, no$id/$ref/$values/$defs: the converter deserializes straight from the reader. It probes on a copy of the reader, so declining leaves the reader untouched. When a registered type's JSON no longer fits, the value is still preserved as raw JSON and a warning is logged, as before.JsonElementNormalizer.DeserializereplacesGetNormalizedRawText, and the read-only collection converters use it too.Repro
PolymorphicReadAllocationTestmeasures one read of a value nested 8 levels deep with a 400,000-byte leaf string:$typelast, general pathLocal Release runs: MeshWeaver.Messaging.Hub.Test 527/527, MeshWeaver.Data.Test 555/555.
-warnaserroris clean on Messaging.Hub, both test projects and Documentation.Not established
The sampler names types, not call sites, so this PR does not show that this converter is the dominant caller in production. The falsifier is the
Stringshare in[HEAPSTEP]on the first image carrying this change. If it stays above 50 %, some other caller holds the JSON as text, and #5555 stays open for it.Refs #5555, refs #4824. Both are severity issues, so they close on post-roll verification, not on this merge.
Pairs-with: none — only internal members changed (
JsonElementNormalizerisinternal); no public surface removed.🤖 Generated with Claude Code