镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Fix immutable sorted dictionary deserialization and simplify converter setup - #137036

Merged
eiriktsarpalis merged 2 commits into
mainfrom
eiriktsarpalis-stj-collection-deserialization
Nov 6, 2026
Merged

eiriktsarpalis merged 2 commits into
mainfrom
eiriktsarpalis-stj-collection-deserialization

Conversation

@eiriktsarpalis

Copy link
Copy Markdown
Member

Immutable sorted dictionary deserialization currently accumulates entries in a Dictionary<TKey, TValue> before invoking the immutable factory. This applies equality/hash semantics before the destination's ordering comparer, which can collapse comparison-distinct keys or produce the wrong duplicate-key behavior. It also introduces hash-based accumulation for a comparison-based destination.

Changes

  • Add a dedicated ImmutableSortedDictionary converter backed by SortedDictionary<TKey, TValue>. Duplicate detection follows the destination key comparer: allowed duplicates remain last-write-wins; disallowed duplicates throw JsonException, including when the values are equal.
  • Preserve equality-based buffering for ordinary immutable dictionaries and leave already-safe sorted dictionary/set buffering unchanged.
  • Move reflection-only configuration onto the corresponding sealed converters and remove the four redundant WithReflection subclasses. Source-generated metadata continues to supply constructor/add delegates directly; reflection setup retains its method-level trimming and dynamic-code annotations.
  • Add public API regressions for equality/comparison disagreement, forbidden key hashing, alternate key representations, nested values, and small-buffer streaming. Add a source-generated trimming/AOT app covering the affected converter families with reflection disabled and analysis-warning suppression disabled.

Validation

All validation used Windows x64 and the repository's Release runtime/library builds.

Check Result
.\build.cmd clr+libs -rc Release -lc Release Passed before and after reflection-wrapper consolidation
.\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -c Release -p:RuntimeConfiguration=Release Passed for net12.0, net11.0, net10.0, netstandard2.0, and net462
Full reflection, Roslyn 4.4/3.11 source-generation integration, FSharp, and both generator-unit suites 135,290 cases, zero failures, 17 existing skips; net12.0/net481 as applicable
ILLink command below All 22 apps published and executed successfully before and after consolidation
NativeAOT command below Both supported apps published and executed successfully before and after consolidation
.\dotnet.cmd msbuild .\src\libraries\System.Text.Json\tests\System.Text.Json.Tests\TrimmingTests\System.Text.Json.TrimmingTests.proj -p:TestTrimming=true -p:Configuration=Release -p:RuntimeConfiguration=Release -p:TargetOS=windows -p:TargetArchitecture=x64
.\dotnet.cmd msbuild .\src\libraries\System.Text.Json\tests\System.Text.Json.Tests\TrimmingTests\System.Text.Json.TrimmingTests.proj -p:RunNativeAotTestApps=true -p:Configuration=Release -p:RuntimeConfiguration=Release -p:TargetOS=windows -p:TargetArchitecture=x64

Every linker/AOT app returned 100, with no linker/AOT diagnostics. The comparer regressions were also demonstrated failing against the original implementation before applying the fix. Non-Windows execution was not performed.

Performance tradeoff

Tree buffering fixes comparison semantics and removes serializer-introduced key hashing, but adds comparison work. BenchmarkDotNet measurements of the dedicated sorted-converter implementation, before the final reflection-hook-only consolidation, showed the following for 1,024 entries with duplicates allowed:

Keys Baseline Sorted buffer Allocated memory
Integer 157.7 us 183.4 us 212,816 B -> 189,320 B (-11%)
String 731.8 us 1,243.5 us 258,248 B -> 213,896 B (-17%)

The string-key case was approximately 70% slower. This is a correctness/hash-resilience change, not a throughput optimization. The final reflection-only consolidation does not change allocation or insertion logic, but those measurements were not rerun after that cleanup. The changed duplicate-key treatment and exception type are observable behavioral corrections.

Note

This pull request was authored with GitHub Copilot.

Use a dedicated SortedDictionary-backed converter so immutable sorted dictionary deserialization preserves comparison-based key semantics without hash staging. Keep duplicate rejection comparer-based and preserve last-write-wins when duplicates are allowed.

Consolidate reflection-only configuration hooks onto their converters and remove redundant WithReflection types. Add public comparer regressions and strict source-generated ILLink/NativeAOT coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Moving reflection-only configuration off the WithReflection constructors removed the IL2026 warning sites in these factories. Their old suppressions now trigger IL2121 during the whole-framework linker build.

Remove only the two unused suppressions; retain required reflection-dataflow suppression and reflection-hook annotations. Validated with the CI whole-framework linker target using warnings-as-errors and public collection/constructor tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@eiriktsarpalis
eiriktsarpalis merged commit 6cf1b61 into main Nov 6, 2026
87 checks passed
@eiriktsarpalis
eiriktsarpalis deleted the eiriktsarpalis-stj-collection-deserialization branch November 6, 2026 11:22
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants