-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regressions in System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties> #73388
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsRun Information
Regressions in System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties>
Reprogit clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f net6.0 --filter 'System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties>*' PayloadsHistogramSystem.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties>.NewCustomConverter
Description of detection logic
DocsProfiling workflow for dotnet/runtime repository
|
Likely caused by #72228 cc @eiriktsarpalis |
Guessing you mean #72937? That PR is outside the reported commit range. |
@eiriktsarpalis yeah, I meant the one which is outside, it seems we didn't file an issue for it, see "global history" https://pvscmdupload.blob.core.windows.net/reports/allTestHistory/refs/heads/main_arm64_ubuntu%2020.04/System.Text.Json.Serialization.Tests.ColdStartSerialization(SimpleStructWithProperties).NewCustomConverter.html |
Got it. That particular PR changes how converters are resolved for properties (they are now resolved lazily via the metadata cache) which may or may not result in redundant calculations being made. Given that the particular benchmark measures serialization with metadata being regenerated from scratch, unless regressions have been recorded in other benchmarks, I don't believe this is blocking release. We should still investigate in case there are any obvious fixes we could apply, but otherwise we could just move it to 8.0.0 -- the particular benchmark still records a net improvement compared to .NET 6 due to the changes in #64646. |
Should be investigated in unison with #71392, it concerns the same benchmark that regressed as part of the same feature. |
Run Information
Regressions in System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties>
Test Report
Repro
Payloads
Baseline
Compare
Histogram
System.Text.Json.Serialization.Tests.ColdStartSerialization<SimpleStructWithProperties>.NewCustomConverter
Description of detection logic
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: