Use Reflection.Emit accessors when dynamic code is supported - #131452
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 525851f0-03df-490b-8c4c-647cc4feb55a
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
There was a problem hiding this comment.
Pull request overview
This PR changes System.Text.Json’s internal MemberAccessor selection logic to prefer the Reflection.Emit-based accessor whenever dynamic code is supported (rather than requiring dynamic code to be compiled), restoring the pre-#130503 behavior to improve steady-state reflection-based serialization performance on interpreter-based runtimes.
Changes:
- Switch accessor selection from
RuntimeFeature.IsDynamicCodeCompiledback toRuntimeFeature.IsDynamicCodeSupported. - Update/remove the prior rationale comment (replacing it with a shorter comment).
Every other perf improvement without numbers turns out to be a regression |
This one actually had perf numbers saying this is a throughput regression: #38693 (comment) and #38693 (comment). So it's more like "AI can help us to churn out fixes without reading the past conversations that were saying doing this without a tradeoff is hard". |
|
Well, my takeaway is that automation (AI and performance testing in this case) at the end of the day steered us towards the right direction with minimal human involvement. And we fixed a few lurking bugs along the way too. |
This restores the
Reflection.Emit-based member accessor whenever dynamic code is supported, reverting only the accessor-selection change from #130503. The reflection invocation fixes and exception-propagation tests from that PR remain intact.Performance evidence
The accessor-selection change in #130503 was the first bad commit for dotnet/perf-autofiling-issues#76620. On Ubuntu 22.04 x64 with WASM and JavaScriptCore:
8df98c4ddb840c)The reflection-based serializer became 2.08x slower while the source-generation control remained unchanged. Additional JavaScriptCore and V8 benchmarks found interpreted
DynamicMethodaccessors 3–41x faster per repeated invocation than reflection. Creating them was 62–66x slower, butSystem.Text.Jsoncaches generated accessors and amortizes that cost.Tradeoff
This intentionally reverses the trimming decision made for #38693. Interpreter deployments will retain the
Reflection.Emitstack, restoring the approximately 50 KB size cost measured there and increasing accessor creation cost. The measurements above show that interpreted emitted IL does provide a substantial steady-state throughput and allocation benefit for reflection-based serialization, contrary to the premise of #130503.Validation
dotnet builddotnet build /t:test .\tests\System.Text.Json.Tests\System.Text.Json.Tests.csproj— 53,409 .NET tests and 53,163 .NET Framework tests passed with zero failuresNote
This pull request was generated by GitHub Copilot.