Skip to content
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.ReadJson<Hashtable> #66341

Closed
performanceautofiler bot opened this issue Mar 8, 2022 · 6 comments · Fixed by #73369
Closed

Regressions in System.Text.Json.Serialization.Tests.ReadJson<Hashtable> #66341

performanceautofiler bot opened this issue Mar 8, 2022 · 6 comments · Fixed by #73369
Assignees
Labels
arch-x64 area-System.Text.Json os-linux Linux OS (any supported distro) runtime-coreclr specific to the CoreCLR runtime
Milestone

Comments

@performanceautofiler
Copy link

Run Information

Architecture x64
OS ubuntu 18.04
Baseline 783e260c78f87dda7cd88b2d8d6305c742e0f78d
Compare 1665aca8f09eba553c8e411a8c01105d71dcabbf
Diff Diff

Regressions in System.Text.Json.Serialization.Tests.ReadJson<Hashtable>

Benchmark Baseline Test Test/Base Test Quality Edge Detector Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
DeserializeFromUtf8Bytes - Duration of single invocation 34.65 μs 42.03 μs 1.21 0.02 True

graph
Test Report

Repro

git clone https://github.com/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f net6.0 --filter 'System.Text.Json.Serialization.Tests.ReadJson&lt;Hashtable&gt;*'

Payloads

Baseline
Compare

Histogram

System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromUtf8Bytes


Description of detection logic

IsRegressionChecked: Marked as regression because the three check build points were 0.05 greater than the baseline.
IsImprovementBase: Marked as not an improvement because the compare was not 5% less than the baseline, or the value was too small.
IsRegressionBase: Marked as regression because the compare was 5% greater than the baseline, and the value was not too small.
IsRegressionChecked: Marked as regression because the three check build points were 0.05 greater than the baseline.
IsRegressionWindowed: Marked as regression because 42.02635741300213 > 36.362182159171695.
IsChangePoint: Marked as a change because one of 3/1/2022 11:44:08 AM, 3/8/2022 2:08:40 PM falls between 2/27/2022 3:52:50 PM and 3/8/2022 2:08:40 PM.
IsRegressionStdDev: Marked as regression because -58.688617384345434 (T) = (0 -41789.89683683401) / Math.Sqrt((174960.4880672375 / (24)) + (276089.457963497 / (41))) is less than -1.9983405425199077 = MathNet.Numerics.Distributions.StudentT.InvCDF(0, 1, (24) + (41) - 2, .025) and -0.19948594027648228 = (34839.838828958185 - 41789.89683683401) / 34839.838828958185 is less than -0.05.
IsImprovementBase: Marked as not an improvement because the compare was not 5% less than the baseline, or the value was too small.
IsChangeEdgeDetector: Marked as regression because Edge Detector said so.

Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

@performanceautofiler performanceautofiler bot added CoreClr untriaged New issue has not been triaged by the area owner labels Mar 8, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@kunalspathak kunalspathak transferred this issue from dotnet/perf-autofiling-issues Mar 8, 2022
@kunalspathak kunalspathak changed the title [Perf] Changes at 3/1/2022 1:15:47 PM Regressions in System.Text.Json.Serialization.Tests.ReadJson<Hashtable> Mar 8, 2022
@kunalspathak
Copy link
Member

Introduced in #65748. @eiriktsarpalis

@ghost
Copy link

ghost commented Mar 9, 2022

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

Issue Details

Run Information

Architecture x64
OS ubuntu 18.04
Baseline 783e260c78f87dda7cd88b2d8d6305c742e0f78d
Compare 1665aca8f09eba553c8e411a8c01105d71dcabbf
Diff Diff

Regressions in System.Text.Json.Serialization.Tests.ReadJson<Hashtable>

Benchmark Baseline Test Test/Base Test Quality Edge Detector Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
DeserializeFromUtf8Bytes - Duration of single invocation 34.65 μs 42.03 μs 1.21 0.02 True

graph
Test Report

Repro

git clone https://github.com/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f net6.0 --filter 'System.Text.Json.Serialization.Tests.ReadJson&lt;Hashtable&gt;*'

Payloads

Baseline
Compare

Histogram

System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromUtf8Bytes


Description of detection logic

IsRegressionChecked: Marked as regression because the three check build points were 0.05 greater than the baseline.
IsImprovementBase: Marked as not an improvement because the compare was not 5% less than the baseline, or the value was too small.
IsRegressionBase: Marked as regression because the compare was 5% greater than the baseline, and the value was not too small.
IsRegressionChecked: Marked as regression because the three check build points were 0.05 greater than the baseline.
IsRegressionWindowed: Marked as regression because 42.02635741300213 > 36.362182159171695.
IsChangePoint: Marked as a change because one of 3/1/2022 11:44:08 AM, 3/8/2022 2:08:40 PM falls between 2/27/2022 3:52:50 PM and 3/8/2022 2:08:40 PM.
IsRegressionStdDev: Marked as regression because -58.688617384345434 (T) = (0 -41789.89683683401) / Math.Sqrt((174960.4880672375 / (24)) + (276089.457963497 / (41))) is less than -1.9983405425199077 = MathNet.Numerics.Distributions.StudentT.InvCDF(0, 1, (24) + (41) - 2, .025) and -0.19948594027648228 = (34839.838828958185 - 41789.89683683401) / 34839.838828958185 is less than -0.05.
IsImprovementBase: Marked as not an improvement because the compare was not 5% less than the baseline, or the value was too small.
IsChangeEdgeDetector: Marked as regression because Edge Detector said so.

Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

Author: performanceautofiler[bot]
Assignees: -
Labels:

area-System.Text.Json, untriaged, refs/heads/main, x64, ubuntu 18.04, RunKind=micro, Regression, CoreClr

Milestone: -

@eiriktsarpalis eiriktsarpalis self-assigned this Mar 9, 2022
@eiriktsarpalis
Copy link
Member

Thanks. This is likely an expected side-effect of the refactorings related to #63747. I'll see if I can negate the loss in future refactorings but it's likely a regression we'd want to take. In any case this should specifically concern deserialization of collections containing polymorphic elements which is not a particularly common scenario.

@steveharter
Copy link
Member

@eiriktsarpalis can you point to the code changes that caused the regression please and what is \ is not affected. For example does this affect all system.object - based collections (i.e. hashtable, arraylist, types implementing non-generic IList.

@eiriktsarpalis
Copy link
Member

The performance regression is directly related to the fact that ObjectConverter is no longer ConverterStrategy.Value. This specifically impacts deserialization performance for objects of depth 1, since they now incur an additional ReadStack.Push() operation. The second ReadStack.Push() operation is always particularly expensive, since it's when a stackframe buffer is first allocated.

This does not impact polymorphic serialization for boxed objects of depth 1, since we can introspect the runtime type of the serialized value and dispatch to the relevant value converter ahead of performing any Push operations.

@jkotas jkotas added arch-x64 and removed x64 labels Mar 14, 2022
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Mar 14, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Mar 14, 2022
@krwq krwq modified the milestones: 7.0.0, 8.0.0 Jul 6, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, 7.0.0 Aug 4, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2022
@jeffhandley jeffhandley added runtime-coreclr specific to the CoreCLR runtime os-linux Linux OS (any supported distro) and removed CoreClr labels Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-System.Text.Json os-linux Linux OS (any supported distro) runtime-coreclr specific to the CoreCLR runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants