Avoid per-element patch-path allocations in C# collection serializers - #11947
Jorge Rangel (jorgerangel-msft) with Copilot wants to merge 22 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
There was a problem hiding this comment.
🔵 Needs a closer look
Nested collection and dictionary serializers still construct indexed patch paths without the collection-level guard.
Pull request overview
This PR reduces per-element patch-path allocations in generated C# collection serializers and ActiveItems() helpers.
Changes:
- Adds descendant-aware patch guards for collections, nested arrays, spans, and dictionaries.
- Preserves child-model root-removal checks.
- Updates generated baselines and adds allocation/patch-semantics tests.
File summaries
| File | Reviewed changes |
|---|---|
packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/NullableDynamicModel.Serialization.cs |
Updates generated collection serializers. |
packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/DynamicModel.Serialization.cs |
Updates generated serializers and helpers. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteRequiredCollectionDoesNotDuplicatePatchedKey.cs |
Updates collection serialization baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteReadOnlySpanProperty.cs |
Updates span serialization baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteNestedArrayPrimitiveProperties.cs |
Updates nested-array baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteNestedArrayDynamicModelProperties.cs |
Updates nested dynamic-model baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteNestedArrayDictionaryProperties.cs |
Updates nested dictionary baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/WriteArrayProperties.cs |
Updates array serialization baselines. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/TestData/DynamicModelSerializationTests/PropagateModelListPropertyHelperMethods.cs |
Updates active-item helper baseline. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/ModelReaderWriterValidation/TestProjects/Sample_TypeSpec/DynamicModelTests.cs |
Tests allocation reduction and child-removal semantics. |
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs |
Generates collection-level guards; nested and dictionary indexed patch-path construction remains ungated. |
Review details
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:167
- This guard only short-circuits
patchIsRemovedCondition; nested serializers still unconditionally build indexed paths forwriteToPatchStatementafter each inner loop (for example,Patch.WriteTo(... Encoding.UTF8.GetBytes($"$.propertyWithNestedArray[{i}]"))). The list-of-dictionaries path is likewise built for each key. Consequently an unpatched nested collection still allocates despitehasPatchbeing false; gate all indexedPatch.WriteTo/Patch.Containsconstruction with the prefix guard, including the dictionary path.
hasPatchDeclaration,
- Files reviewed: 9/11 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot validate if we are correctly guarding on nested elements in dictionaries + lists |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
... Not fully before this follow-up. Fixed in
Regression tests cover list/list, list/dictionary, dictionary/list, and dictionary/dictionary with empty or unrelated patches. Previously these allocated 28–37 KB; they now pass the <1 KB budget. Supported dictionary patch additions/removals and nested child-root removals also pass. Full regeneration, the normal build, 4,196 .NET tests, and 240 emitter tests passed. Independent review found no significant issues. Validation limits: ESLint was unavailable, Cop was not run, automated review was unavailable, CodeQL skipped the oversized database, and explicit .NET Standard compilation encountered Separately, pre-fix probes exposed existing mixed nested-patch merge failures (remove-plus-append and dictionary-of-dictionaries descendant updates). This allocation fix does not change those semantics. |
Copilot this introduced additional complexity and duplication. Revert the previous changes / commit |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:125
- Because every recursive call passes the same root
serializedName, this guard becomes true for every nested collection whenever any sibling under the root property has a patch. For example, a patch under$.propertyWithNestedDictionary["patched"]still makes the loop for"unpatched"build an encoded path for every entry, so the allocation guard is not effective for nested collections with sibling patches. Base the guard on the current nested collection prefix (or carry a per-parent result) and add a sibling-patch regression case.
var hasPatchDeclaration = Declare(
"hasPatch",
typeof(bool),
patchSnippet.Contains(LiteralU8("$"), LiteralU8(serializedName.Split('.')[0])),
out var hasPatch);
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:125
- The new nested-list and read-only-span branches are covered only by generated-source snapshots. The runtime allocation test uses empty inner lists and no span collection, so it would pass even if the
hasPatch0or span path mishandled indexed patches; add a populated nested/span runtime case that asserts patch merging as well as allocations.
var hasPatchDeclaration = Declare(
"hasPatch",
typeof(bool),
patchSnippet.Contains(LiteralU8("$"), LiteralU8(serializedName.Split('.')[0])),
out var hasPatch);
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:630
BuildActiveItemsMethodis validated only byTypeProviderWritertext comparison; no runtime test invokes the generatedTryResolve.../Active...path after an unrelated parent patch or an indexed removal. Add a populated direct dynamic-list test that exercises both cases, otherwise this new short-circuit can regress while the snapshot still passes.
var serializedName = GetJsonSerializedName(property.WireInfo!);
var hasPatchDeclaration = Declare(
"hasPatch",
typeof(bool),
_jsonPatchProperty!.As<JsonPatch>().Contains(LiteralU8("$"), LiteralU8(serializedName.Split('.')[0])),
out var hasPatch);
- Files reviewed: 12/14 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Top-level dictionaries lack a propagated patch guard, allowing unnecessary per-entry path allocations.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 13/15 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved path-escaping and nested-dictionary guard issues remain, along with related coverage gaps.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:111
- Only nested collections reached from a parent list receive an inherited guard. Top-level dictionaries enter this
parentHasPatch == nullbranch, so a nested list or dictionary still emits its interpolatedPatch.WriteTopath unconditionally and allocates once per entry when the parent has no patch. Add a dictionary-level descendant guard and pass it intoCreateDictionaryItemSerialization/child serializers, analogous to the list path.
if (parentHasPatch == null)
{
dictionarySerialization = patchedStatements;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:153
- The allocation test covers only the top-level
Childrenlist, so it never exercises the new inherited-guard branch for nested lists/dictionaries or the read-only-span andActiveItemspaths. Add runtime no-patch/unrelated-patch cases for representative nested and helper scenarios; otherwise a regression in those allocation guards can pass because the remaining coverage is snapshot-only.
if (parentHasPatch == null)
{
hasPatchDeclaration = Declare(
"hasPatch",
typeof(bool),
- Files reviewed: 13/15 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A moderate JSON-path escaping issue and two follow-up findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:762
- The bracket form interpolates
propertySerializedNamedirectly into a JSON-path string. A valid wire name such asfoo".bar(or one containing\\or control characters) therefore produces a malformed or altered path, so the generatedContains/IsRemovedchecks can no longer match patches for that property. Encode the name as a JSON path/string segment before applying C# literal escaping, and add a regression for quote/backslash names.
var jsonPath = propertySerializedName.Contains('.')
? $"$[\"{propertySerializedName}\"]"
: $"$.{propertySerializedName}";
return escapeForCSharpString
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:96
- The new inherited-guard path for a collection nested under a dictionary is not covered by the updated serializer tests:
WriteNestedArrayDictionaryPropertiesexercises array-to-dictionary nesting, whileWriteNestedDict*exercises dictionary-to-dictionary nesting; the only dictionary-to-array case is inPropagateMultipleDynamicProperties, which filters to propagation helpers rather thanJsonModelWriteCore. Add a generated-output or runtime serialization test forDictionary<string, List<T>>covering both no parent patch and a descendant patch.
MethodBodyStatement CreateDictionaryItemSerialization(KeyValuePairExpression item, ValueExpression? itemParentHasPatch)
{
List<ValueExpression> itemChildIndices = item.ValueType.IsCollection
? [.. parentIndices, item.Key]
: parentIndices;
return new MethodBodyStatement[]
{
_utf8JsonWriterSnippet.WritePropertyName(item.Key),
CreateElementSerializationWithPatch(
item.Value,
item.ValueType,
patchSnippet,
serializationFormat,
serializedName,
itemChildIndices,
itemParentHasPatch)
- Files reviewed: 16/18 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate unresolved issues remain in JSON-path escaping, quote handling, and per-key nested patch guarding.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:781
- This escape is applied before
FormattableStringExpressionparses the format, and that parser preserves{{/}}for the generated interpolated string. Replacing each brace with four braces therefore emits{{{{/}}}}; the runtime patch path contains two braces instead of the original one, and the new escaped-name assertion expects only doubled braces. Escape each literal brace only once at this stage.
return value.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("{", "{{{{").Replace("}", "}}}}");
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:103
- This callback is emitted only inside
if (!patchContainsNet8Var), so the current dictionary key has no patch descendants. Passing the outerhasPatch(which can be true because a sibling key is patched) makes nested lists/dictionaries under every unpatched key run indexed/key path checks and allocations; pass the per-key condition (orFalsehere) so path-dependent work stays disabled for that key.
CreateDictionaryItemSerialization(keyValuePair, hasPatch)
- Files reviewed: 16/18 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate and critical findings remain in nested no-patch generation and quote-collision path handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:126
- Passing
Falsehere still recursively invokesCreateElementSerializationWithPatch, so nested collections in the no-patch branch generate the full patch-aware serializer under constantif (false)blocks (the updated nested-dictionary baselines contain several of these). This avoids runtime work but duplicates every nested loop/path expression in generated clients; generate a separate no-patch element path, or otherwise omit the patch branch, instead.
var unpatchedForeachStatement = new ForEachStatement("unpatchedItem", dictionary, out KeyValuePairExpression unpatchedKeyValuePair);
// This branch only runs when the collection has no relevant patch, so serializers can skip path-dependent patch work.
unpatchedForeachStatement.Add(CreateDictionaryItemSerialization(unpatchedKeyValuePair, False));
- Files reviewed: 16/18 changed files
- Comments generated: 1
- Review effort level: Lite
| bool doubleQuoteCollides = propertySerializedName.Contains("\"]", StringComparison.Ordinal); | ||
| bool singleQuoteCollides = propertySerializedName.Contains("']", StringComparison.Ordinal); | ||
| char quote = doubleQuoteCollides && !singleQuoteCollides ? '\'' : '\"'; | ||
| return $"$[{quote}{propertySerializedName}{quote}]"; |
…ested collections Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical escaping defect and two moderate patch-path issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:120
patchContainsNet8Varis the descendant-aware check for the current dictionary key, so this!patchContainsNet8Varbranch already proves that no parent patch exists underitem.Key. PassingitemSuppressPatchLogic: falsenevertheless makes nested collections emit and execute their indexed patch paths for every unpatched dictionary entry (and produces an unreachable nestedelseinside the enclosinghasPatchbranch, as inWriteNestedDictPrimitiveProperties.cs:67-128). Pass the no-patch mode here so child-model root checks remain but nested collection path work is omitted.
CreateDictionaryItemSerialization(keyValuePair, hasPatch, itemSuppressPatchLogic: false)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.Dynamic.cs:834
- When both collision patterns occur, this still chooses a delimiter that is known to collide. A valid wire name such as
a"]b']cmakes both flags true, so the generated path uses"and the reader terminates at the embedded"]; because the reader has no escape syntax, patch lookups for this property cannot match. Handle or reject this case instead of selecting the colliding delimiter, and add a regression for it.
char quote = doubleQuoteCollides && !singleQuoteCollides ? '\'' : '\"';
- Files reviewed: 16/18 changed files
- Comments generated: 1
- Review effort level: Lite
| private static string EscapeForCSharpInterpolatedString(string value) | ||
| { | ||
| return value.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("{", "{{{{").Replace("}", "}}}}"); | ||
| } |
Generated collection serializers and
ActiveItems()allocate an interpolated string and UTF-8 byte array per element, even when no relevant patch exists.Contains(prefix, property)overload. Keep child-model root-removal checks independent of the parent guard.