You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Migration now reads the source and calls the migrator through typed code instead of object-typed invokers. A value-type source or target is no longer boxed, which removes two allocations per struct migration. Reference-type migrations do the same work as before.
MigratorReference<TTarget> lets JsonMigratableConverter<T> read and migrate a source without knowing its type; MigratorReference<TSource, TTarget> reads through the source's typed converter and calls a typed invoker.
Static migrations call TTarget.TryMigrateFrom through the IMigrateFrom constraint. Inherited contracts with a derived-target overload, public or not, keep working through a cached typed delegate.
External migrators still resolve from the service provider on every call, then from a fallback constructed once.
Fix: a struct target that implements IJsonMigrationTracked now reports MigratedDuringDeserialization. The flag used to be set on a boxed copy.
Hot-path benchmarks for struct migrations and 1 to 8 registered versions. On .NET 10 they run out of process with five launches and automatic warmup; .NET 11 stays in process until BenchmarkDotNet 0.16, which can start a .NET 11 child process.
Characterization tests for discriminator matching across many sources and for contract compatibility, in the feature test classes.
The comparison runner starts both launch modes in the selected worktree so the .NET 10 out-of-process build benchmarks the requested revision. Normal and isolated one-case checks confirm baseline 72 B / candidate 24 B; these checks establish revision identity, not throughput.
Additional behavioral tests cover nested and recursive union routing, converter overrides, numeric-contract precedence, JSON DOM ambiguity, and repeated resolver entries. No production-library changes or coverage exclusions were needed.
Implements the typed invokers and the benchmark scenarios. Three items were measured and left out:
Discriminator lookup: parked on egil/208-discriminator-lookup. For eight same-length names it saves about 11 ns when the last one matches and costs about 3 ns when the first does (-4% to +1% of a migration).
Caching the IJsonMigrationTracked check per T: for reference types the converter runs as shared generic code, where the cached static readonly bool compiles to a CORINFO_HELP_GET_NONGCSTATIC_BASE call in front of the check it guards.
Bypassing Lazy<T> for the fallback migrator: its initialized path is one volatile read plus a field read.
Refreshing docs/perf/*.md still needs a clean-machine run of the full suite.
Performance
Pinned A/B on a Ryzen 9 5950X (WSL2, taskset -c 6,7). Every case, ref and round runs in a fresh process: a 4 s time-based warmup, then 15 x 200 ms samples, and the per-process statistic is the fastest sample. The ref order alternates each round. Delta is the median per-round candidate/baseline ratio with a bootstrap 95% interval. Baseline f9c917a (benchmarks only, production code as on main) vs e8cdc95.
case
.NET 10, 8 rounds
.NET 11, 6 rounds
B/op (.NET 10)
struct static, reflection
234 -> 217 ns, -6.6% [-9.1, -2.7]
223 -> 203 ns, -8.8% [-14.8, +0.9]
72 -> 24
struct external, reflection
231 -> 218 ns, -4.9% [-7.0, -1.7]
217 -> 209 ns, -4.1% [-6.9, -0.5]
72 -> 24
struct static, source gen
293 -> 281 ns, -3.3% [-4.7, -1.8]
152 -> 104
struct external, source gen
292 -> 277 ns, -5.0% [-6.4, -0.4]
152 -> 104
1 source, reference types
-0.6% [-4.1, +3.2]
152
8 sources, last match, reference types
-1.0% [-4.0, +0.3]
-2.9% [-9.8, +1.4]
152
control: plain STJ read
+0.5% [-1.0, +3.3]
-0.1% [-2.9, +2.4]
24
control: Manual benchmark
+0.7% [-0.3, +2.1]
-0.2% [-5.8, +1.4]
24
Struct migrations are 3-9% faster and allocate 48 B less per operation; reference-type migrations are unchanged. The earlier in-process BenchmarkDotNet runs could not resolve this: one launch per case and a fixed warmup of 1-6 iterations, on paths that need seconds for tiered compilation to settle.
Validation
Release build, warnings as errors: 0 warnings, 0 errors.
895 tests pass across .NET 10 and .NET 11.
CI: build, tests, pack and package validation pass.
Branch coverage (coverlet): MigratorReference.Typed.cs, MigratorInvokerFactory.cs, and JsonMigratableTypes are at 100% on both frameworks; .NET 11 UnionCaseRouting is also at 100%. Overall coverage is 97.47% on .NET 10 (654/671) and 98.37% on .NET 11 (904/919). Other core components retain uncovered defensive/fallback branches, so the full 100% core-component policy is still not met.
No bugs found — reviewed the discriminator-lookup bucketing, the fallback-caching in ExternalMigratorInvoker, and the reflection-based static-contract matching for correctness, thread-safety, and security concerns; nothing met the high-confidence bar.
One CLAUDE.md/AGENTS.md compliance issue (not tied to a specific diff line, so posting as a general comment):
Four commit bodies contain internal QA/process bookkeeping instead of consumer-facing prose, and none use [skip notes]:
fix(stjm): preserve empty migration discriminators (df1482e) — body ends "...the full solution now passes 824 tests."
fix(stjm): retain non-public inherited migration overloads (df1482e) — body ends "...update the recorded solution test count."
fix(stjm): validate inherited migration overload shape (df1482e) — body ends "All 830 package and sample tests pass across .NET 10 and .NET 11."
Both root AGENTS.md and project AGENTS.md require commit bodies to be "clear, user-facing prose... explains what changed and why... Avoid internal-only context (chat logs, agent session IDs, implementation minutiae)" because body text is included verbatim in the generated release notes. generate-release-notes.ps1's $skipTypes list excludes docs/test/chore/etc. but not fix/perf, so these four bodies will surface as-is in the changelog's Bug Fixes / Performance sections unless reworded (or tagged [skip notes], though that would also drop the legitimately useful first paragraph of each body).
The reason will be displayed to describe this comment to others. Learn more.
Deep review of 2be1d71: production changes, benchmarks and tooling, plus a pinned A/B run to settle the throughput question.
I found no behavioral regressions in the production code. Quoted numbers, base-type converters, escaped/segmented/empty discriminators, inherited overloads (including the non-public and ref/non-bool cases), DI re-resolution and tracking on subclasses all hold, and the tests pin them. The inline comments cover structure, and micro-optimizations that add code without removing measurable work.
Why a speedup is hard to prove
Only value-type sources and targets can get faster. For reference types the new path does the same amount of dispatch as the old one:
old ReadAsObject -> IMigratorInvoker.TryMigrate (interface) -> is TSource -> delegate / IMigrate call -> is T
new (IMigrationReader<T>) cast -> TryRead (interface) -> converter.Read -> TryMigrate (virtual) -> TTarget.TryMigrateFrom / IMigrate call
For structs the old path boxed the source and the result (2 x 24 B) and unboxed both; the new path does neither. That is worth ~10 ns on a 220-290 ns deserialize, below what the current BDN job can resolve (see the comment on PerfBenchmarkConfig).
Pinned A/B
Ryzen 9 5950X, WSL2, taskset -c 6,7, other work running on the host. Baseline 2bcb5e7 vs candidate afde4f1; the migration hot path is unchanged in 2be1d71 (2eb34b2 only touches cold resolver code). The same benchmark source is compiled against each. Every (case, ref, round) is a fresh process: 4 s time-based warmup, then 15 x 200 ms samples; the per-process statistic is the fastest sample. 8 rounds with ref order alternating ABBA. Delta is the median of the per-round candidate/baseline ratios, with a bootstrap 95% interval.
.NET 10.0.12, ns/op:
case
baseline
candidate
delta
95% CI
B/op
struct static, reflection
234.3
216.4
-7.3%
-11.2 .. -4.2
72 -> 24
struct external, reflection
235.8
221.8
-5.9%
-7.1 .. -5.4
72 -> 24
struct static, source gen
293.2
280.8
-3.7%
-5.3 .. -2.7
152 -> 104
struct external, source gen
293.5
278.6
-5.3%
-7.3 .. -3.7
152 -> 104
8 sources, last (bucketed)
290.2
289.1
-0.5%
-5.1 .. +2.0
152
8 sources, first (bucketed)
290.6
282.5
-3.7%
-10.1 .. -0.3
152
4 sources, last (linear)
294.8
291.4
-2.2%
-4.4 .. +2.0
152
1 source
295.2
284.6
-1.5%
-7.5 .. +1.4
152
control: plain STJ read
114.6
115.2
+0.6%
-4.3 .. +3.7
24
control: migratable read, no migration
170.4
169.9
+0.3%
-3.1 .. +4.5
24
control: Manual benchmark
118.3
113.6
-4.1%
-7.1 .. -0.5
24
.NET 11.0.0-rc.1, 6 rounds, same protocol:
case
baseline
candidate
delta
95% CI
B/op
struct static, reflection
229.0
210.0
-8.5%
-9.9 .. -7.1
72 -> 24
struct external, reflection
220.3
212.0
-5.5%
-8.6 .. -1.8
72 -> 24
8 sources, last (bucketed)
283.6
291.5
+3.9%
-3.0 .. +6.5
152
control: plain STJ read
115.8
115.0
-2.7%
-7.2 .. +4.5
24
control: Manual benchmark
114.6
117.9
+0.8%
-2.3 .. +4.8
24
How to read it:
On .NET 10 the Manual control moved 4% although its measured code never enters the library (on .NET 11 it stayed at +0.8%). Its [GlobalSetup] runs Static() and External(), so its hot code is jitted at different addresses in each build. The 8-source first case moving -3.7% while the isolated lookup says bucketing is slower there points the same way. Treat about ±4% as the floor for cross-build comparisons like this one.
Struct paths: 8-19 ns (4-9%) faster on both runtimes; on .NET 10, 8-13 ns beyond the Manual control's shift. That matches removing two small allocations and two unbox checks. This is the throughput claim the PR can make, together with the 48 B.
Reference-type paths: inside the floor. Nothing to prove there, by construction.
What made this resolvable where the BDN runs were not: a time-based warmup long enough for tiering to finish, many processes instead of one launch per case, adjacent A/B pairs with alternating order, a statistic that ignores slow samples from interference, and controls to calibrate the floor.
Where the time goes
Candidate, .NET 10, fastest sample, median of 3 processes:
plain STJ read of the struct source 122 ns
same read through JsonMigratableConverter (no migration) 182 ns +60
static migration, source NOT [JsonMigratable] 188 ns
static migration, source [JsonMigratable] (benchmark) 230 ns +41
A migratable source is read through its own JsonMigratableConverter, whose Inspect copies the reader and re-reads and re-compares a discriminator the target converter has already matched. That pass costs ~41 ns per migration, about 3x what this PR saves. Skipping it when the source was selected by discriminator is where a real throughput gain is. The +60 ns on current-format reads (Inspect plus a second ReadStack from JsonResumableConverter<T>.Read) is the next one. Both are follow-ups, not blockers.
Commits
981bbf5, fb6b5f4, 71722b1 and 9bbc43c fix regressions introduced earlier on this branch. Merged as they are, generate-release-notes.ps1 publishes them under Performance and Bug Fixes as fixes for bugs that never shipped, with internal details ("824 tests", laptop comparisons). Fold them into c599d76 with fixups before merging.
2eb34b2 and 2be1d71 (coverage) are independent of the dispatch change and could merge on their own.
The reason will be displayed to describe this comment to others. Learn more.
The derived positional record renames the base parameters, so the compiler synthesizes a second set of properties instead of reusing the inherited ones. Reflection on the built assembly:
Every instance now carries SourceMetadata + Metadata, ElementMetadata + Element, ElementAcceptsNonObjectShapes + AcceptsNonObjectElements, and the source type info three times (SourceTypeInfo, TypeInfo, typeInfo). Two public names for one value also invites the next change to read the wrong one.
Reusing the base names makes the compiler skip synthesis:
Nothing compares, hashes or prints these references, so record only buys generated Equals/GetHashCode/PrintMembers/<Clone>$ over ~28 fields. Plain classes would fit better, but that part predates this PR.
The reason will be displayed to describe this comment to others. Learn more.
(IMigrationReader<T>)migrator relies on an invariant the types don't state: every MigratorReference in this context is a MigratorReference<?, T>. The base record is concrete and unsealed, so nothing stops an untyped instance from reaching this cast at runtime.
In Tier1 code this line is a CORINFO_HELP_CHKCASTINTERFACE helper call ahead of the TryRead interface call, in the shared and the struct instantiations alike. An abstract generic middle layer (the JsonConverter -> JsonConverter<T> shape) states the invariant, and swaps the interface cast + interface dispatch for a class cast + vtable call:
if (serviceProvider?.GetService(migratorType) is { } service)
{
if (service is IMigrate<TSource, TTarget> typedMigrator)
{
return typedMigrator;
}
throw new InvalidOperationException(
$"Service provider returned instance of type '{service.GetType().FullName}' for migrator '{migratorType.FullName}', but the instance is not assignable to the migrator type.");
}
return fallbackMigrator.Value;
// Keep Lazy's once-only construction and cached failures on the cold path,
// but publish the successful instance directly for subsequent migrations.
// Provider-backed calls must still query DI before consulting this cache.
The reason will be displayed to describe this comment to others. Learn more.
Two caches for one value. On the initialized path Lazy<T>.Value is already a volatile read of _state plus a field read, so resolvedFallback ??= saves one load.
The base/derived split costs more than that: ResolveMigrator() used to be a private non-virtual call; now it is protected virtual, so every external migration pays a virtual call so that the no-DI case can skip one null check.
The previous single sealed class, made typed, does less work and is less code:
internalsealedclassExternalMigratorInvoker<TSource,TTarget>:MigratorInvoker<TSource,TTarget>{// ... ctor validation and Lazy as before, plus IServiceProvider? serviceProviderpublicoverrideboolTryMigrate(TSourcesource,outTTargetmigrated)=>ResolveMigrator().TryMigrateFrom(source,outmigrated);privateIMigrate<TSource,TTarget>ResolveMigrator(){if(serviceProvider?.GetService(migratorType)is{}service){returnserviceasIMigrate<TSource,TTarget>??thrownewInvalidOperationException(/* ... */);}returnfallbackMigrator.Value;}}
That also removes ServiceProviderMigratorInvoker and the branch in CreateExternalInvokerGeneric.
The reason will be displayed to describe this comment to others. Learn more.
This doesn't remove work. Tier1 code for the shared reference-type instantiation, .NET 10:
; JsonMigratableConverter`1[System.__Canon]:Read (Tier1)movrdi, qword ptr [rbx]call CORINFO_HELP_GET_NONGCSTATIC_BASE ; CanTrackMigration: a helper call per readcmp byte ptr [rax],0je SHORT G_M000_IG43movrsi,r15movrdi,0x708576048130call[CORINFO_HELP_ISINSTANCEOFINTERFACE] ; the check it guards
Shared __Canon code can't treat a generic type's static as a constant, so every reference-type read now makes a helper call to maybe skip an isinst: one helper call traded for another at best (sealed, untracked T), one added otherwise. The struct instantiation (JsonMigratableConverter1[PerfStructStaticTarget]) contains neither call: the JIT already folded value is IJsonMigrationTracked` there.
I'd drop the field and keep Tracking_is_set_when_only_the_migrated_subclass_implements_tracking, which pins real behavior either way.
Separate, and pre-existing on main: for a struct target that implements IJsonMigrationTracked, the pattern match boxes a copy and sets the flag on the box. Repro against both 2bcb5e7 and this branch:
struct target: Value=42 MigratedDuringDeserialization=False
class target: Value=42 MigratedDuringDeserialization=True
The fix is to write the boxed copy back when T is a value type (return the value, or take it by ref). Worth its own issue, since this PR is what makes struct targets a first-class path.
The reason will be displayed to describe this comment to others. Learn more.
I can't find a case where this pays for its three strategy classes.
Isolated lookup cost: a copy of Linear and Bucketed over 8 same-length names (version-1 .. version-8), reader positioned on the value, .NET 10, median ns per lookup:
first last miss
linear 2.6 20.9 18.9
bucketed 5.7 9.7 1.6
So bucketing saves ~11 ns when the match is last of eight and costs ~3 ns when it is first. FrozenDictionary<long, ...> with 8 keys is itself a linear scan over sorted keys, which is why last still costs more than first. In the full deserialize (~290 ns) that is -4% to +1%, only for registries with more than four sources. The pinned A/B in the review summary shows the 8-source cases inside the ±4% build-to-build floor.
It has also needed two follow-ups inside this PR: c599d76 bucketed by length only and regressed same-length names, 981bbf5 fixed that and broke empty discriminators, fb6b5f4 fixed those. The four-source cutoff is still unmeasured.
I'd split it out: land the typed dispatch (a measured allocation and struct-path win) on its own, and bring the lookup back if a real registry shows the gain. If it stays, Bucketed : Linear inherits only to reuse a static helper; composition over the array reads more directly.
The reason will be displayed to describe this comment to others. Learn more.
PerfBenchmarkConfig is the main reason the timing runs can't resolve this PR. It predates the PR, but these benchmarks inherit it: in-process no-emit toolchain, 1 launch, 1 warmup (3 or 6 via the script), 5 iterations (15 via the script).
These paths take seconds to settle, and not monotonically. Fresh process on the candidate, no warmup, pinned to two vCPUs, ns/op per 100 ms slice:
VersionsMigrationHotPathBenchmarks, 8 sources, last (.NET 10)
0.2-1.0 s 3262 3043 2992 2531 2424 1918 1770 1790 1670
1.1-2.4 s 632 300 289 289 298 290 288 291 289 289 286 295 288 291
2.5-3.2 s 563 570 565 555 549 559 562 434 <- back to ~550 ns, 2.5 s in
3.3-5.0 s 288 285 285 289 294 288 288 301 ...
Another process of the same case sat on the same ~550 ns plateau at 1.1-1.8 s instead, so it looks like a code state (a tiering step) rather than host noise. A fixed warmup count ends inside that window whenever the machine is a little slower, and the transition lands in the measured iterations. That is the 418 -> 137 ns step in scripts/tests/fixtures/transition-after-warmup.txt.
For .NET 10 the fix is the default job:
AddJob(Job.Default// out of process: every case gets its own process.WithLaunchCount(5));// samples per-process variance (tiering timing, code layout)// no WarmupCount: auto warmup keeps going while iterations still trend
.NET 11 is the real constraint. I checked on a .NET 11 RC1 host: BDN 0.15.6 and 0.15.8 both throw NotImplementedException: GetRuntimeVersion not implemented for NotRecognized from CsProjCoreToolchain.Validate, even with an explicit CoreRuntime.CreateForNewVersion("net11.0", ...) + CsProjCoreToolchain.From(...). BDN 0.16.0-preview.2 builds and runs the default job out of process on .NET 11. Either move to 0.16 when it fits, or keep the in-process job for net11 only, drop the fixed WarmupCount there, and treat the per-case processes as its launch count.
The reason will be displayed to describe this comment to others. Learn more.
With SourceCount = 1, First, Middle and Last all select version 1, so 12 of the 42 cases are three copies of four workloads. A [ParamsSource] that yields only valid combinations shrinks the matrix and the run.
Until then the copies are a free A/A check: any spread between them is noise, and a candidate delta of the same size means nothing.
The reason will be displayed to describe this comment to others. Learn more.
This block, -IsolateHotPathCases, -DiagnoseTiering, -RequireStableMeasurements and check-perf-stability.ps1 rebuild process-per-case isolation and a warmup screen on top of an in-process job. On .NET 10 BDN's default out-of-process job does both: one process per case, and auto warmup that continues while iterations still trend down. On .NET 11 with BDN 0.15.x, per-case processes are the only way to get more than one launch (see the PerfBenchmarkConfig comment), so that part has a reason to exist until BDN 0.16. The rest can go:
-DiagnoseTiering answered a one-off question; it overloads baseline/candidate to mean tiering on/off in a script named compare-refs.
The stability screen regex-parses console lines, which break with BDN's log format. If a raw-iteration check stays, read the fulljson exporter's Measurements (they carry IterationMode/IterationStage). Auto warmup removes most of what it screens for.
--validate-hot-path hand-copies the [Params] matrix and drifts when a value is added. [GlobalSetup] already throws on a wrong result, so a short BDN run over the filter validates every case without a second copy of the matrix.
The reason will be displayed to describe this comment to others. Learn more.
"Typed migration" names this PR's implementation technique, not a behavior, so the class name stops meaning anything once this merges. The tests belong with their features:
tracking, provider re-resolution and fallback construction: RegistrationAndTrackingTests
inherited static contracts: with the other static-discovery tests in RegistrationAndTrackingTests
escaped, segmented, empty and unknown discriminators: EdgeCaseBehaviorTests
MigrationContractCompatibilityTests (2be1d71) has the same issue: it is named for why it was written (a coverage audit). Its resolver-composition tests already borrow ResolverChainTests.ForwardingResolver, which says where they live; the number-handling and element-history cases fit NumericSourceMigrationTests and NonObjectPayloadMigrationTests.
Nit: UniformSourceOptions (line 158) sits between tests with no blank line before the next [Fact].
Add struct migration and many-version lookup scenarios whose setup validates the migrated result. Versions are measured at the first, middle and last position of 4 and 8 registered sources, and once for a single source. Production code is unchanged, so this commit is the baseline for the typed dispatch comparison.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add an opt-in fresh process per parameter combination to the controlled reference comparison runner. Preserve its power, affinity and report comparison behavior, and fail if a case does not produce exactly one successful result.
The initial issue #208 laptop reports showed large drift in an unchanged manual baseline. Isolation removes cross-case JIT state from subsequent measurements without changing the workloads.
The hot-path benchmarks measure differences of a few nanoseconds per operation, but migration paths take seconds to reach their final tiered code, so a fixed one to six warmup iterations let tier transitions land in the measured iterations. Give them their own job: out of process with five launches on .NET 10, in process on .NET 11 where BenchmarkDotNet 0.15 cannot start a child process, and automatic warmup on both. The scenario job also drops its fixed warmup count, and run-perf.ps1 and perf-compare-refs.ps1 pass a warmup count only when one is given.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Migrations now read the source and call the migrator through typed code instead of object-typed invokers. A migration whose source or target is a value type no longer boxes either value, saving two allocations per migration; in pinned .NET 10 and .NET 11 benchmarks such migrations ran 4-9% faster. Reference-type migrations are unchanged.
Existing behavior is preserved, including quoted numeric sources, source converters declared for a base type, inherited static contracts with public or non-public derived-target overloads, per-call service provider resolution, and cached fallback construction failures.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin how a target with six registered sources matches source discriminators: escaped and segmented JSON values, names sharing a length and prefix or suffix, unknown values, and an empty registered discriminator. These characterize the current linear match so any faster lookup must keep the same results.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A struct target that implements IJsonMigrationTracked now reports MigratedDuringDeserialization after a migration or a legacy read. The flag was previously set on a boxed copy, so the returned value always reported false.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove an unused resolver helper and compile union-only discovery only for the .NET 11 asset. Read reflection parameter metadata without redundant null-pattern checks.
The merge-readiness coverage audit identified these unreachable paths. Existing serializer behavior is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Protect number-handling precedence, collection element histories, resolver composition and union legacy routing through public serialization APIs. Merge-preparation review identified these useful inherited coverage gaps; the added cases validate preserved behavior without internal-state manipulation. [skip notes]
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
egil
changed the title
perf(stjm): use typed migration dispatch and precomputed discriminator lookups
perf(stjm): use typed migration dispatch
Sep 24, 2026
Start both benchmark launch modes in the selected revision's performance project directory and restore the caller's directory even when a benchmark fails. BenchmarkDotNet resolves out-of-process projects from the working directory, so using an absolute assembly path alone could measure the caller's checkout under a baseline label.
Verified the isolated and normal comparison paths with a pinned struct case: the baseline reports 72 B per operation and the candidate reports 24 B.
Cover discriminator forwarding through nested unions, external recursive migration safeguards, custom source converters, strict numeric contracts, and JSON DOM ambiguity through the public serializer. Verify that repeated resolver entries retain existing registrations.
These cases protect stored data from being silently routed to the wrong current model. The added tests cover 23 previously missed .NET 11 branches and bring union routing and migratable-type detection to 100% branch coverage without changing production code or excluding defensive checks.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The checked-in hot-path benchmark configuration does not explicitly match the warmup and iteration budgets used by the reported performance measurements.
The PR's performance methodology says each sample is 200 ms after a 4-second time-based warmup, but this job leaves both iteration time and warmup budget at BenchmarkDotNet defaults. That makes the checked-in benchmark configuration unable to reproduce the reported measurements; configure those budgets explicitly or update the performance claims to match the actual defaults.
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
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.
egil
left a comment
Changes
Migration now reads the source and calls the migrator through typed code instead of object-typed invokers. A value-type source or target is no longer boxed, which removes two allocations per struct migration. Reference-type migrations do the same work as before.
MigratorReference<TTarget>letsJsonMigratableConverter<T>read and migrate a source without knowing its type;MigratorReference<TSource, TTarget>reads through the source's typed converter and calls a typed invoker.TTarget.TryMigrateFromthrough theIMigrateFromconstraint. Inherited contracts with a derived-target overload, public or not, keep working through a cached typed delegate.IJsonMigrationTrackednow reportsMigratedDuringDeserialization. The flag used to be set on a boxed copy.Relation to #208
Implements the typed invokers and the benchmark scenarios. Three items were measured and left out:
egil/208-discriminator-lookup. For eight same-length names it saves about 11 ns when the last one matches and costs about 3 ns when the first does (-4% to +1% of a migration).IJsonMigrationTrackedcheck perT: for reference types the converter runs as shared generic code, where the cachedstatic readonly boolcompiles to aCORINFO_HELP_GET_NONGCSTATIC_BASEcall in front of the check it guards.Lazy<T>for the fallback migrator: its initialized path is one volatile read plus a field read.Refreshing
docs/perf/*.mdstill needs a clean-machine run of the full suite.Performance
Pinned A/B on a Ryzen 9 5950X (WSL2,
taskset -c 6,7). Every case, ref and round runs in a fresh process: a 4 s time-based warmup, then 15 x 200 ms samples, and the per-process statistic is the fastest sample. The ref order alternates each round. Delta is the median per-round candidate/baseline ratio with a bootstrap 95% interval. Baselinef9c917a(benchmarks only, production code as on main) vse8cdc95.ManualbenchmarkStruct migrations are 3-9% faster and allocate 48 B less per operation; reference-type migrations are unchanged. The earlier in-process BenchmarkDotNet runs could not resolve this: one launch per case and a fixed warmup of 1-6 iterations, on paths that need seconds for tiered compilation to settle.
Validation
MigratorReference.Typed.cs,MigratorInvokerFactory.cs, andJsonMigratableTypesare at 100% on both frameworks; .NET 11UnionCaseRoutingis also at 100%. Overall coverage is 97.47% on .NET 10 (654/671) and 98.37% on .NET 11 (904/919). Other core components retain uncovered defensive/fallback branches, so the full 100% core-component policy is still not met.🤖 Generated with Claude Code