Add benchmarks for (de)serialization using ReferenceHandling.Preserve#1130
Add benchmarks for (de)serialization using ReferenceHandling.Preserve#1130adamsitnik merged 8 commits intodotnet:masterfrom
Conversation
Deserialize using PreserveBenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-alpha.1.20071.3
[Host] : .NET Core 5.0.0 (CoreCLR 5.0.20.7004, CoreFX 5.0.20.7004), X64 RyuJIT
Job-KCALPP : .NET Core 5.0.0 (CoreCLR 5.0.20.7004, CoreFX 5.0.20.7004), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250.0000 ms MaxIterationCount=20
MinIterationCount=15 WarmupCount=1
Deserialize NOT using preserve
|
Serialize using preserveBenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-alpha.1.20071.3
[Host] : .NET Core 5.0.0 (CoreCLR 5.0.20.7004, CoreFX 5.0.20.7004), X64 RyuJIT
Job-CRDAEN : .NET Core 5.0.0 (CoreCLR 5.0.20.7004, CoreFX 5.0.20.7004), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250.0000 ms MaxIterationCount=20
MinIterationCount=15 WarmupCount=1
Serialize NOT using preserve
Here you can see that some types show less time when preserve is on, such as |
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you!
When should we expect the new API to get merged into dotnet/runtime? I am not sure I should merge the PR now or wait.
adamsitnik
left a comment
There was a problem hiding this comment.
@jozkee the CI fails with following error:
[2020/01/21 22:27:02][INFO] corefx\System.Text.Json\Serializer\ReadPreservedReferences.cs(50,39): error CS0117: 'Categories' does not contain a definition for 'CoreFX' [C:\h\w\A9B50997\p\src\benchmarks\micro\MicroBenchmarks.csproj]
[2020/01/21 22:27:02][INFO] corefx\System.Text.Json\Serializer\WritePreservedReferences.cs(36,39): error CS0117: 'Categories' does not contain a definition for 'CoreFX' [C:\h\w\A9B50997\p\src\benchmarks\micro\MicroBenchmarks.csproj]
[2020/01/21 22:27:02][INFO] 0 Warning(s)
[2020/01/21 22:27:02][INFO] 2 Error(s)
Because I've changed the folder structure in the meantime in #1132 to use the new naming after repos got merged.
Basically everything that was called CoreFX is now called Libraries (this includes folder name and category name). Could you please fetch the latest changes and sync your branch?
| } | ||
|
|
||
|
|
||
| [BenchmarkCategory(Categories.CoreFX, Categories.JSON)] |
There was a problem hiding this comment.
The name has recently changed to Libraries
| [BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | |
| [BenchmarkCategory(Categories.Libraries, Categories.JSON)] |
| _settings = new JsonSerializerSettings { PreserveReferencesHandling = PreserveReferencesHandling.All }; | ||
| } | ||
|
|
||
| [BenchmarkCategory(Categories.CoreFX, Categories.JSON)] |
There was a problem hiding this comment.
| [BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | |
| [BenchmarkCategory(Categories.Libraries, Categories.JSON)] |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #if !NETFRAMEWORK && !NETCOREAPP2_1 && !NETCOREAPP2_2 && !NETCOREAPP3_0 && !NETCOREAPP3_1 |
There was a problem hiding this comment.
| #if !NETFRAMEWORK && !NETCOREAPP2_1 && !NETCOREAPP2_2 && !NETCOREAPP3_0 && !NETCOREAPP3_1 | |
| #if NETCOREAPP5_0 |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #if !NETFRAMEWORK && !NETCOREAPP2_1 && !NETCOREAPP2_2 && !NETCOREAPP3_0 && !NETCOREAPP3_1 |
There was a problem hiding this comment.
I think that as for now we can simplify it to #if NETCOREAPP5_0 and later when we have more 5.0-only files extend the project file with a new rule for 5.0
| #if !NETFRAMEWORK && !NETCOREAPP2_1 && !NETCOREAPP2_2 && !NETCOREAPP3_0 && !NETCOREAPP3_1 | |
| #if NETCOREAPP5_0 |
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: empty line
and exclude benchmarks for versions < 5.0
…efeerenceHandlingBenchmarks
| <Compile Remove="runtime\PacketTracer\**\*.cs" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' Or ('$(TargetFrameworkIdentifier)' == '.NETCoreApp' And '$(_TargetFrameworkVersionWithoutV)' < '5.0')"> |
There was a problem hiding this comment.
@adamsitnik I added the exclude condition to avoid using the directive which would need extra changes in future netcoreapp releases.
| <Compile Remove="libraries\System.Text.Json\Serializer\WritePreservedReferences.cs" /> | ||
| </ItemGroup> | ||
|
|
||
| <!-- TODO: Remove package dependency for future --> |
There was a problem hiding this comment.
People that pull my changes will be broken and will need to install the latest SDK build; to avoid that, the 5.0 benchmarks will use the NuGet package version of System.Text.Json.
There was a problem hiding this comment.
In general for tests targeting the current master branch we have the expectation that you need to always have an updated SDK matching it. It's fine to assume they'll have the right 5.0 bits.
There was a problem hiding this comment.
I see, then I will proceed to remove this. Thanks.
There was a problem hiding this comment.
In general for tests targeting the current master branch we have the expectation that you need to always have an updated SDK matching it.
Interesting. I didn't know that. Sounds good :)
Does the benchmarks_ci.py script install the latest SDK for the developer?
https://github.com/dotnet/performance/blob/ca80d8e2886b583d0a69635740b188248d3d6fdd/docs/benchmarkdotnet.md#using-python-script
cf5306a to
4feb6a6
Compare
|
|
||
| [BenchmarkCategory(Categories.ThirdParty, Categories.JSON)] | ||
| [Benchmark(Baseline = true)] | ||
| public T NewtonsoftDeserializePreserved() => JsonConvert.DeserializeObject<T>(_serialized, _settings); |
There was a problem hiding this comment.
Comparing to Newtonsoft is useful for the first time analysis and comparison. However, I don't think we should check this in. We don't want fluctuations in that library between perf runs show up as regressions which are not actionable for us (and whoever is reviewing the regression would have to filter those out, since filing issues for it doesn't make sense).
There was a problem hiding this comment.
If a benchmark belongs to ThirdParty category, we don't run it in our lab. So it can stay as is and be used for future manual comparisons.
| public string SerializePreserved() => JsonSerializer.Serialize(_value, _options); | ||
|
|
||
| [BenchmarkCategory(Categories.ThirdParty, Categories.JSON)] | ||
| [Benchmark(Baseline = true)] |
There was a problem hiding this comment.
This Baseline isn't doing what we would want it to do since this benchmark is generic and instead of comparing
SerializePreserved vs NewtonsoftSerializePreserved, we are comparing one model vs another, which isn't helpful.
Even though, I think we should remove the Newtonsoft test, it brings up an interesting question on how would we get the baseline comparison we want (even on local runs), if we wanted to? @adamsitnik - any suggestions?
There was a problem hiding this comment.
Is very likely that what is causing such behavior is that I am running the benchmarks with --join, it is not refreshing the baseline when joining, should it?
Here's my command:
dotnet run -c Release -f netcoreapp5.0 --filter *WritePreservedReferences* --join
ahsonkhan
left a comment
There was a problem hiding this comment.
Otherwise, looks good.
cc @ahsonkhan, @steveharter, @layomia