-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Perf] XmlSerializer.Deserialize() regression between release/2.0.0 and release/2.1 #25275
Comments
If this was due to Marvin.ComputeHash calls, maybe this regression was addressed by dotnet/coreclr#16654 and dotnet/coreclr#16659, which are meant to address https://github.com/dotnet/corefx/issues/27485? |
@stephentoub it seems that you are right! I just used BenchmarkDotNet following the instructions from: dotnet/corefx#27164 And compared 2.0.5 vs 2.1.preview vs latest packages on MyGet (CoreCLR 4.6.26301.09, CoreFX 4.6.26228.08): BenchmarkDotNet=v0.10.12.449-nightly, OS=Windows 10 Redstone 3 [1709, Fall Creators Update] (10.0.16299.248)
Intel Core i7-3687U CPU 2.10GHz (Ivy Bridge), 1 CPU, 4 logical cores and 2 physical cores
Frequency=2533323 Hz, Resolution=394.7385 ns, Timer=TSC
.NET Core SDK=2.1.300-preview2-008162
[Host] : .NET Core 2.1.0-preview2-26131-06 (CoreCLR 4.6.26130.05, CoreFX 4.6.26130.01), 64bit RyuJIT
Job-VQCTZN : .NET Core ? (CoreCLR 4.6.26301.09, CoreFX 4.6.26228.08), 64bit RyuJIT
2.0 : .NET Core 2.0.5 (CoreCLR 4.6.26020.03, CoreFX 4.6.26018.01), 64bit RyuJIT
2.1 : .NET Core 2.1.0-preview2-26131-06 (CoreCLR 4.6.26130.05, CoreFX 4.6.26130.01), 64bit RyuJIT
Runtime=Core LaunchCount=1 TargetCount=3
WarmupCount=3
I am going to check it once again tomorrow to be 100% sure. If the regression is gone I am going to close the issue. |
I have verified that the initial regression of 30% for Windows (#27636) was fixed by dotnet/coreclr#16654 and dotnet/coreclr#16659 Unfortunately, this is not true for Linux (at least Ubuntu 16.04). |
I would hope when taking dotnet/coreclr#16659 into account that master would now be a bit faster than 2.0 -- the change in dotnet/coreclr#16654 should have undone the regression all by itself. |
Saw you left a note over in dotnet/coreclr#16659 that you see ~3% or so from this change, so is master now 3% faster than 2.0? |
@AndyAyersMS Sorry for not making myself clear. Master is actually a little bit faster (few %) than 2.0 now. So for Windows not only the regression is gone, but it's also few % faster (depending on the scenario). For Linux (at least Ubuntu 16.04) it's still 100% slower. |
This is a follow up of https://github.com/dotnet/coreclr/issues/16627
There is a performance regression of
XmlSerializer.Deserialize(Stream)
for 2.1 compared to 2.0. For Windows it's from 10 to 30%, for Linux it's from 50% to 100%.I did a small investigation, it looks that few calls to
system.private.xml!System.Marvin.ComputeHash(System.ReadOnlySpan<Byte>, UInt64)
are responsible for that.2.1 most time-consuming methods by name:
2.0 most time-consuming methods by name:
Methods which call
Marvin.ComputeHash
Later I am going to provide a separate PR with a new benchmark for our benchmarking suite. For now please use following plain simple code:
The csproj:
To run the benchmark:
dotnet run -c Release -f netcoreapp2.0
anddotnet run -c Release -f netcoreapp2.0
Some Flame Graphs from PerfView which give good overview:
For 2.0:
For 2.1:
The problematic part:
It's my first perf investigation as a new member of the .NET Performance Team, please let me know if something is missing or wrong.
The text was updated successfully, but these errors were encountered: