Skip to content

perf(stjm): keep STJ fast-path serialization by registering through the resolver chain - #213

Merged
egil merged 45 commits into
mainfrom
stjm/207-resolver-chain-registration
Sep 23, 2026
Merged

egil merged 45 commits into
mainfrom
stjm/207-resolver-chain-registration

Conversation

@egil

@egil egil commented Sep 21, 2026 •

Copy link
Copy Markdown
Owner

Closes #207.

What changed

AddJsonMigrationSupport() no longer adds a converter factory to options.Converters. It inserts JsonMigrationTypeInfoResolver (the renamed JsonMigratableConverterFactory) at the front of TypeInfoResolverChain, which returns a JsonMetadataServices.CreateValueInfo<T> contract wrapping JsonMigratableConverter<T> for [JsonMigratable] types and null for everything else. The exclusion clone used while building a type's converter wraps the inherited resolver (PlainContractResolver) and a per-options MigrationScope tells the migration resolver when to step aside for the type being built. Discovery of the registration (idempotency guard, union classifier, reflection fallback) walks the resolver structure without calling it (ResolverLeaves: chains are lists, STJ's WithAddedModifier wrapper keeps its source in a field), so decorated entries, replaced resolvers and copies are all handled and no user resolver is invoked during registration.

Why: STJ only uses a JsonSerializerContext's generated fast-path serializers when options.Converters.Count == 0 (JsonSerializerContext.IsCompatibleWithOptions), and JsonTypeInfo.Configure evaluates that per node through OriginatingResolver.IsCompatibleWithOptions. One entry in the list therefore put every type in the options on the metadata path. With a resolver, only the migratable contracts themselves (customized, so IsCustomized == true) and types whose property graph reaches them lose the fast path.

Behavior

  • Converter precedence unchanged: a converter in options.Converters registered before AddJsonMigrationSupport() still wins for a [JsonMigratable] type, one registered after still does not (the resolver snapshots the list at registration).
  • Options with no resolver keep reflection-based serialization (the resolver stands in for DefaultJsonTypeInfoResolver when it is the only chain entry, because STJ no longer populates it for a non-empty chain).
  • Changed: a custom IJsonTypeInfoResolver that serves a [JsonMigratable] type with its own converter used to win silently because it never consulted options.Converters. It now has to be inserted ahead of migration (TypeInfoResolverChain.Insert(0, ...) after AddJsonMigrationSupport()); otherwise migration owns the type and rejects the non-object contract. One test (Overridden_migratable_element_has_no_discriminator_route) was rewritten for this.
  • Changed: clearing TypeInfoResolverChain or assigning TypeInfoResolver after AddJsonMigrationSupport() removes migration support. One test (Copied_options_use_their_own_metadata_context_for_converter_creation) now swaps only the context.

Documented in docs/recipes/aot-source-gen.md ("Keeping the generated fast path"), the AddJsonMigrationSupport remarks, and the new docs/recipes/upgrading-to-v2.md.

Stored 1.x data

The 1.7 release's test suite, run unmodified against this branch, passes every payload test (199/200; the failing one is the resolver-chain-clear setup above). That run and review found read regressions introduced by the .NET 11 commit on main, fixed here: sources with a converter override were excluded from shape matching, and types 2.0 newly recognises (Guid, Half, ...) competed with the 1.x sources next to them. Scalar matching now runs in tiers: 1.x's TypeCode sources first (plain ahead of overridden), then the 2.0 types, then quoted numbers, then overridden 2.0 types, and the same order for first-element matching of collections. A source 1.x selected is selected again; a 2.0 source only where 1.x had no match.

Evidence

  • dotnet build Egil.SystemTextJson.Migration.slnx -c Release on SDK 11.0.100-rc.1: 0 warnings.
  • Tests: 253/253 on net10.0, 342/342 on net11.0.
  • New SourceGenFastPathTests read STJ's internal CanUseSerializeHandler through UnsafeAccessor: a type outside migration reports true, the migratable type and a wrapper containing one report false, the wrapper still writes the nested $type.

Benchmark tooling (first commit)

  • scripts/run-perf.ps1 -Label x: one pinned run (single SMT pair, fixed iteration budget) into a labelled folder.
  • scripts/compare-perf.ps1 -Baseline a -Candidate b: diff two labels.
  • scripts/perf-compare-refs.ps1 -BaselineRef main -CandidateRef HEAD: full before/after of two git refs on a dedicated machine. Temporary worktrees, the candidate's benchmark project for both refs (identical payloads, only the library differs), interleaved rounds, a baseline-vs-baseline drift check, and a single summary.md to send back. The active power scheme is left to BenchmarkDotNet, which runs under High Performance and restores the previous one; BDN does not touch processor boost (PowerManagementApplier only calls PowerSetActiveScheme), so the script sets the High Performance scheme's boost mode to Disabled for the run and restores the previous values afterwards, also on failure (-KeepBoost opts out; the summary records what was done).
  • Perf types got short TypeDiscriminator values; the bare attribute made $type the 65-char type name, so the Small serialize alloc ratio measured output length.

To run on the idle laptop (about an hour for two full rounds; -Filter '*SourceGen*' halves it):

git clone https://github.com/egil/framework.git; cd framework
git checkout stjm/207-resolver-chain-registration
.\Egil.SystemTextJson.Migration\scripts\perf-compare-refs.ps1 -BaselineRef main -CandidateRef HEAD

Framework defaults to net11.0 when an RC SDK is installed, else net10.0. Post the generated summary.md.

Benchmark result

Measured on an idle i7-13800H (P-core pair, net11.0, two interleaved rounds; see the comments for both runs). Source-gen serialize, migratable/plain: Large 1.32 → 1.06, Medium 1.71 → 1.37, Small unchanged at 2.0 (single object, metadata path by design). No consistent movement anywhere else. docs/perf/*.md and the README table are regenerated from that run.

🤖 Generated with Claude Code

egil and others added 2 commits September 21, 2026 12:24
scripts/run-perf.ps1 runs the BenchmarkDotNet scenarios pinned to one
SMT pair with a fixed iteration budget and stores the reports under a
label; scripts/compare-perf.ps1 diffs two labels and flags changes that
exceed three standard deviations of the noisier run.
scripts/perf-compare-refs.ps1 orchestrates a full before/after run of
two git refs on a dedicated machine: temporary worktrees, the
candidate's benchmark project for both refs, interleaved rounds, a
drift check between baseline rounds and one summary.md to send back.

The perf types now carry short TypeDiscriminator values like the
polymorphic guardrail does. With the bare attribute the discriminator
was the 65-character full type name, which made the Small serialize
allocation ratio (56 B vs 136 B) a measurement of output length rather
than of library overhead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he resolver chain

AddJsonMigrationSupport() no longer adds a converter factory to
options.Converters. System.Text.Json only uses a JsonSerializerContext's
generated fast-path serializers when that list is empty, so one entry
switched every type in the options to the metadata path, including
types that never take part in migration. Migration is now a resolver
inserted at the front of TypeInfoResolverChain. Types outside migration
keep their generated serializer; a [JsonMigratable] type itself, and
any type whose properties reach one, still serializes through the
metadata path because its contract carries the injected discriminator.

Precedence is unchanged for converters: one registered in
options.Converters before AddJsonMigrationSupport() still wins for a
[JsonMigratable] type, one registered after still does not. Options
without any resolver keep reflection-based serialization.

Two setups behave differently. A custom IJsonTypeInfoResolver that
serves a [JsonMigratable] type with its own converter used to win
silently because it never consulted options.Converters; it now has to
be inserted ahead of migration with TypeInfoResolverChain.Insert(0, ...)
after AddJsonMigrationSupport(), otherwise migration owns the type and
rejects the non-object contract. Clearing TypeInfoResolverChain or
assigning TypeInfoResolver after AddJsonMigrationSupport() now removes
migration support, since that is where it lives.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@egil egil added the stjm Egil.SystemTextJson.Migration scope label Sep 21, 2026
@egil

This comment has been minimized.

egil and others added 2 commits September 21, 2026 14:28
…marks [skip notes]

The default affinity mask chose the highest-numbered SMT pair, which on a
hybrid Intel part is two E-cores, so a whole comparison ran at E-core base
clock. Hybrid parts (core count times two differs from the thread count)
now get the second P-core pair; the summary also prints the mask in hex.
update-perf-docs.ps1 accepts -ResultsDir so the docs can be refreshed from
a perf-compare-refs.ps1 candidate folder.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…p notes]

Numbers now come from perf-compare-refs.ps1 on an idle i7-13800H (P-core
pair, boost off, 15 iterations) with the resolver-chain registration in
place. Source-generated serialization of a migratable type with nested
plain types is within 6% of plain STJ on the large profile and 1.37x on
the medium one, down from 1.32x and 1.71x before the change on the same
machine.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@egil

egil commented Sep 21, 2026

Copy link
Copy Markdown
Owner Author

P-core rerun on the same laptop (affinity 0xc, boost off, net11.0, 2 rounds × 15 iterations), main vs d8e75e1. Drift between baseline rounds is under 5 % for the source-gen class.

Source-gen serialize Baseline Candidate Plain STJ migratable/plain before → after
Large 6,389 / 6,328 ns 5,061 / 5,111 ns 4,800 ns 1.32 → 1.06
Medium 881 / 855 ns 698 / 693 ns 504 ns 1.71 → 1.37
Small 174 / 169 ns 168 / 169 ns 85 ns 2.0 (unchanged, metadata path by design)

Deserialize scenarios and the reflection class: no consistent movement. docs/perf/*.md and the README table are regenerated from candidate round 2 of this run.

@egil
egil marked this pull request as ready for review September 21, 2026 16:47
Copilot AI lite review requested due to automatic review settings September 21, 2026 16:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved resolver precedence and benchmark safety issues remain, along with missing source-generation coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Moves STJ migration registration into the resolver chain to preserve source-generated fast paths for unrelated types.

Changes:

  • Adds resolver-based migration contracts and precedence tests.
  • Updates union routing, copied-options behavior, and converter handling.
  • Adds benchmark tooling and refreshes performance documentation.
File Summary
Egil.SystemTextJson.Migration/​test/​Egil.SystemTextJson.Migration.Tests/​SourceGenFastPathTests.cs Fast-path and precedence coverage
Egil.SystemTextJson.Migration/​test/​Egil.SystemTextJson.Migration.Tests/​RegistrationAndTrackingTests.cs Copied-options metadata tests
Egil.SystemTextJson.Migration/​test/​Egil.SystemTextJson.Migration.Tests/​NumericSourceMigrationTests.cs Resolver override scenarios
Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​Migrations/​UnionCaseRouting.cs Resolver-backed union routing
Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​Migrations/​JsonMigrationTypeInfoResolver.cs Resolver-based migration contracts
Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​Migrations/​JsonMigratableTypes.cs Converter override detection
Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​JsonMigrationSerializerOptionsExtensions.cs Resolver-chain registration
Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​JsonMigratableUnionTypeClassifier.cs Migration resolver lookup
Egil.SystemTextJson.Migration/​scripts/​update-perf-docs.ps1 Benchmark documentation updates
Egil.SystemTextJson.Migration/​scripts/​run-perf.ps1 Labeled benchmark execution
Egil.SystemTextJson.Migration/​scripts/​perf-compare-refs.ps1 Cross-reference benchmark orchestration
Egil.SystemTextJson.Migration/​scripts/​compare-perf.ps1 Benchmark comparison
Egil.SystemTextJson.Migration/​README.md Updated performance results
Egil.SystemTextJson.Migration/​perf/​Egil.SystemTextJson.Migration.PerfTests/​MigrationScenarioBenchmarks.cs Short benchmark discriminators
Egil.SystemTextJson.Migration/​docs/​recipes/​aot-source-gen.md Resolver ordering guidance
Egil.SystemTextJson.Migration/​docs/​perf/​source-gen-benchmarks.md Refreshed source-generation report
Egil.SystemTextJson.Migration/​docs/​perf/​reflection-benchmarks.md Refreshed reflection report

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Egil.SystemTextJson.Migration/scripts/perf-compare-refs.ps1 Outdated
… read [skip notes]

perf-compare-refs.ps1 disabled processor boost even when powercfg output
could not be parsed, and the finally block then had no value to restore,
leaving the machine without boost. The script now aborts before touching
the power plan in that case.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 21, 2026 16:53
egil and others added 2 commits September 21, 2026 16:54
…skip notes]

Processor boost mode is a hidden power setting, so powercfg -query never
listed it and Get-BoostMode always returned null. Combined with the
previous restore logic that meant -DisableBoost turned boost off and never
turned it back on. The attribute hide flag is now cleared before the
query; without elevation the query stays empty and the script refuses to
change the plan.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s [skip notes]

BenchmarkDotNet already switches the active power scheme to High
Performance for a run and restores it afterwards, so the scripts' own
boost handling competed with it and edited whichever scheme was current
rather than the one the benchmarks run under. -DisableBoost and the
powercfg calls are gone; the docs say to disable boost on the High
Performance scheme by hand when a frozen clock is wanted.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical issues remain in duplicate resolver registration and PowerShell compatibility, with additional benchmark safety fixes requested.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Check powercfg exit codes before running benchmarks

Egil.SystemTextJson.Migration/​scripts/​perf-compare-refs.ps1:108

These native powercfg calls do not check $LASTEXITCODE. On an unelevated or otherwise unsupported machine, -DisableBoost can silently fail to change the plan while the script continues and reports a supposedly controlled benchmark, so check each exit code and throw before running the measurements.

Comment thread Egil.SystemTextJson.Migration/scripts/compare-perf.ps1 Outdated
Comment thread Egil.SystemTextJson.Migration/scripts/perf-compare-refs.ps1 Outdated
Copilot AI review requested due to automatic review settings September 21, 2026 16:57
egil and others added 2 commits September 21, 2026 17:01
…[skip notes]

The null-conditional member operator only exists in PowerShell 7, so the
script failed to parse in the stock Windows shell.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Calling AddJsonMigrationSupport() twice on the same options put two
migration resolvers in the resolver chain. The reflection fallback only
stands in while the migration resolver is the sole chain entry, so a
second call made options without any other resolver fail to serialize
plain types. A second call now keeps the first registration, which is
what the previous converter-factory registration did in effect because
System.Text.Json only consulted the first matching converter.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Fix the benchmark runner’s framework/build behavior and update the stale Stryker mutation path.

Review effort: Lite
Findings: None

Resolved since last review (3)

Copilot AI review requested due to automatic review settings September 21, 2026 17:04
run-perf.ps1 passed --no-build to dotnet run, so on a fresh checkout it
failed before running anything. It now builds the requested framework
first and keeps --no-build for the run itself.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Three moderate follow-ups remain in benchmark execution and mutation-test coverage.

Review effort: Lite
Findings: None

Copilot AI review requested due to automatic review settings September 21, 2026 17:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

…tring converter

1.x routed a JSON number to an enum source by TypeCode even when
JsonStringEnumConverter was registered, and that converter reads integers
by default. The 2.0 rule that excludes sources with a converter override
from shape matching therefore rejected stored numeric payloads whose only
source is such an enum. The number is now offered to an overridden enum
source when no other source matched, so those payloads migrate as they
did in 1.x while a plain numeric source still wins over the enum.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
egil and others added 21 commits September 23, 2026 10:51
…through it

Restoring a hidden registration by inserting its resolver at the front
of the chain took the migratable types away from the application-defined
wrapper that had been consulted first; the wrapper's own contracts for
those types no longer applied. The guard now leaves the chain as the
user shaped it: an application-defined resolver in the chain is assumed
to be wrapping the registered entry, and a chain made of STJ's own
resolvers is the only shape that counts as having removed it.

The union classifier used the strict structural check and reported
missing migration support behind such a wrapper; it now applies the same
rule and routes with the registration cached for the options.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…y they wrap

A copy of options whose migration entry sits behind an application-
defined wrapper carries no cached registration, and the chain walk
cannot see through the wrapper, so registering again on the copy put a
fresh registry in front of it and the union classifier reported missing
migration support.

Registration discovery now asks each application-defined resolver in
the chain for the contract of a marker type that only the migration
resolver answers, and takes the resolver stamped on the answer as the
one behind it. That replaces the assumption that any such resolver was
wrapping the entry: replacing the chain with one's own resolver and
registering again now registers again, as with STJ's resolvers.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A resolver that forwards only its own application's types never sees
the probe's marker, so its silence said nothing, yet the guard read it
as the registration having been removed and put a fresh registry in
front of it, losing the explicitly registered migrators.

A positive probe remains proof. A silent application-defined resolver
now keeps the cached registration, as the walk cannot tell it from a
wrapper. Copies of registered options, which carry no cached
registration, find the original's through the chain object STJ's copy
constructor shares with them until they change their own chain.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The union classifier routed with whatever registration was cached for
the options. A registration the options no longer use can stay cached
behind an application-defined resolver that answers no probe, and a
wrapper can send the cases to a resolver registered on other options;
in both cases the routes came from the wrong registry.

The classifier now reads the resolver from the migration converter a
case resolves to and routes with its registry, using the cached lookup
only for the exclusions of the options being resolved. A case that
resolves to its plain contract without a converter override is
reported as missing migration support rather than as a converter
ordering problem.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d route union cases per registry

A copy that extended its own chain gets a chain object of its own, so
the chain the original registered on no longer identified it; behind a
type-selective wrapper neither the walk nor the probe could recover the
registration and a fresh registry went in front. The copy's chain is
built from the entries of the chain it was copied from, so an
application-defined resolver it shares with a registered chain now
identifies that registration.

Union routing used one registry for every case, the first served case's,
while a type-selective wrapper can send cases to different migration
resolvers. Each case now routes with the registry that built its
converter; a case inside its own migration build keeps the scope's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nested case

The outer union forwarded a nested union's case discriminators and
sources from its own registry, while a type-selective resolver can
serve the nested case through another migration resolver whose
registry holds the external migrators for it. Old nested payloads then
went unclassified although the inner union could migrate them.

A nested migratable case now forwards its routes from the registry that
built its converter, as a direct case does.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… resolver

Matching any application-defined resolver shared with a registered
chain made independently created options that reuse a downstream
resolver inherit that chain's registration: their own registration
call returned without inserting a resolver or running its
configuration.

Registration puts the migration resolver at the front of the chain, so
the inference now requires the registered chain to no longer show its
resolver and the shared instance to be the application-defined entry
at its front, the one that took the resolver's place. A downstream
resolver shared with a registered chain says nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Discovery probed application-defined resolvers with a marker type, so
the first registration on options whose resolver wraps a
DefaultJsonTypeInfoResolver resolved through it and froze its
Modifiers, the side effect registration promises not to have.

The probe has no case left to serve: a copy finds its registration
through the chain object it shares with the original or the wrapper at
the front of that chain, a cached registration is kept while an
application-defined resolver is in the chain, and union routing reads
each case's registry from the converter the case resolves to. It is
removed; discovery walks STJ's chains and decorators and calls nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…egistered chain

A resolver at the front of a registered chain that no longer shows its
migration resolver looks the same whether it wraps that resolver or was
left behind when the resolver was removed, so independently created
options reusing it inherited a registration they never had and their
own registration call ran no configuration.

Registration carries over to a copy only by identity, through the chain
object STJ's copy constructor shares with it. A copy that changed its
own chain behind a wrapper registers like any other options, with the
configuration it passes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ison script [skip notes]

run-perf.ps1 defaulted to net10.0 while perf-compare-refs.ps1 and the
README's perf tables use the net11.0 build once an RC SDK is installed.
Both scripts now share that default.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… [skip notes]

A branch name given to perf-compare-refs.ps1 resolved to the local
branch, which on a machine used only for measuring is whatever was last
checked out there; a run meant for the current head measured a commit
nineteen library commits old. The script now fetches origin first and
refuses a local branch that is behind its origin counterpart.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…estore it [skip notes]

BenchmarkDotNet switches the active scheme to High Performance and
back, and nothing else: PowerManagementApplier only calls
PowerSetActiveScheme. That scheme ships with processor boost set to
Aggressive, so the turbo clock swung with load and heat, and the last
laptop run's reflection scenarios moved by up to 2x between rounds.

perf-compare-refs.ps1 now sets the High Performance scheme's boost
mode to Disabled for the run and restores the previous AC and DC values
in its finally block, also when the run fails or is interrupted. The
values are read from the registry, since the attribute is hidden and
unhiding it needs elevation while setting it does not. -KeepBoost opts
out; the summary records what was done.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… [skip notes]

When the AC value had been set and the DC one failed, the saved values
were discarded and the finally block skipped the restore. They are now
kept so both values are restored regardless.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…itted [skip notes]

A High Performance scheme that had never set its boost mode inherited
the setting's default, and the restore wrote that number back as the
scheme's own value. The read now records per value whether it was
explicit or inherited; the restore puts the number back and, for an
inherited value, removes the scheme's own entry again. That removal
needs elevation and is skipped without it, leaving the same number set
explicitly, which the summary and the restore message both state.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n entry keeps it [skip notes]

The AOT recipe and the upgrade guide said that assigning
TypeInfoResolver after registration removes migration support, while
decorating, combining or wrapping the entry keeps it working, as the
resolver chain tests cover. Both now distinguish replacing the entry
from delegating to it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…kip notes]

The non-object note said a source behind a converter override is
considered last, while the tier sentence after it, and the code, match
an overridden source of a type 1.x knew right after the plain ones of
those types and only overridden sources of 2.0's types last.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ck [skip notes]

The boost mode was disabled before the try block whose finally restores
it, so a failure in between would have left it disabled. The change now
happens inside the protected region; the saved values are initialized
before it so the finally block can read them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ip notes]

A laptop run spent 14 hours on four BenchmarkDotNet runs of about 26
minutes each: the gaps sat between runs, where the display timed out
and standby suspended the process. The script now holds an execution
state request for system and display for the whole run, released in
its finally block or when the process exits, and stamps its progress
lines with the time so a stall shows in the output.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kip notes]

A union routes a scalar payload to the case with that shape, and the
case's converter applies the source tiers, so a number stored through
a 1.x int source reaches it even when the case also migrates from Half.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… converter build

The union classifier re-rooted the options' scope to the resolver that
served the union's first case. Inside a converter build that scope is
the building registry, and the case being built has no migration
converter to ask, so it fell back to whichever registry served the
first case; a nested case then got that registry's discriminator and
migrators. Since each migratable case now reads its registry from its
own converter, the scope is kept as is, and the first case's resolver
stands in only when the options carry no registration at all.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r [skip notes]

Two summary blocks sat on CaseResolver, one describing the rectangle-only
resolver below it. RectangleResolver is now CaseResolver routing
RectangleV2, under one summary.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four unresolved review comments remain, including three moderate issues.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct diagnostics for resolver-provided converter overrides

Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​Migrations/​UnionCaseRouting.cs:103

When the case is served by a resolver override, HasConverterOverride is also true, so this branch tells the caller to call AddJsonMigrationSupport() before registering a converter even though the documented fix is to insert that resolver ahead of migration; the resolver may not be an options converter at all, and migration has already been added. Distinguish resolver-provided overrides from options.Converters/attributes in this diagnostic so the actionable ordering guidance is not misleading.

A union case served by a converter other than migration's was always
told to register its converters after AddJsonMigrationSupport(), also
when the override came from a resolver ahead of the migration entry or
from an attribute, where no converter registration is involved. The
diagnostic now distinguishes a converter in options.Converters from a
resolver or attribute override and gives the fix for each.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate findings remain in converter-order preservation, nested-union resolver discovery, and benchmark-script repeatability.

Review effort: Lite
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Two moderate findings remain unresolved; two additional nit findings were noted.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Recurse through nested union cases during migration classification

Egil.SystemTextJson.Migration/​src/​Egil.SystemTextJson.Migration/​JsonMigratableUnionTypeClassifier.cs:99

CanClassify recognizes unions recursively, but this fallback only inspects direct cases. If an options instance has no local migration scope and an application resolver forwards a nested union's migratable case to another migration resolver, the direct case is the nested union, so ServingResolver returns null and classification reports missing migration support even though the nested case resolves to IJsonMigratableConverter. Recurse through nested union cases and cover this no-local-registration shape.

Picking the converter branch whenever options.Converters held a
matching converter misattributed a resolver-supplied override when a
converter had been added after registration too. Which source served
the contract is not recorded on it, so the diagnostic now names all
three with the fix for each.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The critical numeric matching defect and three moderate issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

The quoted-number tier considered every numeric source at once, so a
target migrating from int and Half threw on "42" as ambiguous although
a plain 42 goes to int. The tier is now split, legacy numeric types
first and then the ones 2.0 added, for top-level payloads and for the
first element of a collection alike.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Five unresolved moderate findings affect resolver behavior, scope lifetime, and benchmark CPU-affinity handling.

Review effort: Lite
Findings: None

Resolved since last review (1)

egil commented Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Re-reviewed fba864efac49c7a36468fa6404de1b657b416d5a against the previously reviewed snapshot, accounting for the rebase.

  • Standards: no new actionable findings in the delta.
  • Behavior: the quoted-number legacy/widened precedence fix and union diagnostic changes look correct. No new findings. The existing P2 benchmark HEAD stale-ref issue remains open: perf-compare-refs.ps1 is unchanged from eedb1dc, and still compares HEAD against origin/HEAD rather than the checked-out branch's upstream. See the original finding.

Verification: ran dotnet test --solution Egil.SystemTextJson.Migration.slnx -c Release locally from an isolated checkout of this exact head: 766 passed, 0 failed, 0 skipped, covering net10.0 and net11.0. CI also passed its test, documentation, build and package-validation jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stjm Egil.SystemTextJson.Migration scope

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(stjm): register migration through the resolver chain so source-gen fast-path serialization survives

2 participants