-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add benchmarks for Utf8JsonReader.Get* #1381
Conversation
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18362.900 (1903/May2019Update/19H1)
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=5.0.100-preview.8.20326.4
[Host] : .NET Core 5.0.0 (CoreCLR 5.0.20.32602, CoreFX 5.0.20.32602), X64 RyuJIT
Job-FKMNJY : .NET Core 5.0.0 (CoreCLR 5.0.20.32602, CoreFX 5.0.20.32602), X64 RyuJIT
PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable IterationTime=250.0000 ms
MaxIterationCount=20 MinIterationCount=15 WarmupCount=1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
|
||
|
||
private static readonly byte[] _jsonSingleBytes = Encoding.UTF8.GetBytes(float.MaxValue.ToString("F")); | ||
private static readonly byte[] _jsonDoubleBytes = Encoding.UTF8.GetBytes(double.MaxValue.ToString("F")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static readonly byte[] _jsonDoubleBytes = Encoding.UTF8.GetBytes(double.MaxValue.ToString("F")); | |
private static readonly byte[] _jsonDoubleBytes = Encoding.UTF8.GetBytes(double.MaxValue.ToString()); |
This "F" format representation for this value is quite long, and it shows in the results for double
compared to the other number types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point was to get the perf. result in the worst case scenario, why we should not aim to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had to pick one, I think it's more valuable to benchmark the most common scenarios. A 312 digit number would be a rare occurrence in a JSON payload.
private static readonly byte[] _jsonGuidBytes = Encoding.UTF8.GetBytes($"\"{Guid.Empty}\""); | ||
private static readonly byte[] _jsonDateTimeBytes = Encoding.UTF8.GetBytes($"\"{DateTime.MaxValue:O}\""); | ||
private static readonly byte[] _jsonDateTimeOffsetBytes = Encoding.UTF8.GetBytes($"\"{DateTimeOffset.MaxValue:O}\""); | ||
private static readonly byte[] _jsonStringBytes = Encoding.UTF8.GetBytes($"\"{new string('a', 1024*2)}\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this string is too long. I think a string like the "The quick brown fox jumps over the lazy dog" pangram would do here, and be closer in length to strings founds in typical payloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it might be useful to have both here. A longer string is going to give us protection against any bad scaling that might be introduced that we could miss with a shorter string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add
[Params(true, false)]
public bool UseLargeValue;
So we can test both scenarios, on true
we use the MaxValue
and on false
we can use default
.
var reader = new Utf8JsonReader(_jsonSByteBytes); | ||
reader.Read(); | ||
return reader.GetByte(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jozkee do you know what part of the total time is spent for the creation of Utf8JsonReader
, calling Read
and calling GetByte()
? What is the typical usage of reader.GetX()
methods?
I am asking because the creation of the reader and call to reader.Read
might be more expensive than the call to GetX
which we are trying to measure here. If this is true, we should reconsider rewriting the benchmark to something like:
public sbyte ReadAndGetSBytes()
{
var reader = new Utf8JsonReader(_jsonSByteBytes); // _jsonSByteBytes should contain more than 1 SByte (100 should be enough)
sbyte result = default;
while (reader.Read())
{
result += reader.GetSByte();
}
return result;
}
this should allow for removal of UseLargeValue
and hence reducing the number of new benchmarks and the time it takes to run them by half. Please see https://github.com/dotnet/performance/blob/master/docs/microbenchmark-design-guidelines.md#Test-Cases for more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, for GetInt32
, ~80% time is spent in the first two lines:
[Benchmark]
public void CreateAndReadInt32()
{
var reader = new Utf8JsonReader(_jsonInt32Bytes);
reader.Read();
}
Method | UseLargeValue | Mean | Error | StdDev | Median | Min | Max | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|---|---|
CreateAndReadInt32 | False | 39.95 ns | 0.548 ns | 0.512 ns | 39.81 ns | 39.42 ns | 41.11 ns | - | - | - | - |
GetInt32 | False | 46.41 ns | 0.302 ns | 0.283 ns | 46.47 ns | 45.90 ns | 46.81 ns | - | - | - | - |
CreateAndReadInt32 | True | 49.73 ns | 0.211 ns | 0.187 ns | 49.71 ns | 49.48 ns | 50.02 ns | - | - | - | - |
GetInt32 | True | 62.31 ns | 0.647 ns | 0.573 ns | 62.42 ns | 60.74 ns | 63.15 ns | - | - | - | - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I added ReadToEnd*
benchmarks in order to measure how much time is spent in traversing the 100 elements of the json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach that we could take to avoid having the ReadtoEnd*
methods could be just call reader.GetInt32()
over the same token a hundred times, that could remove the overhead of reader.Read()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jozkee thank you for adding more benchmarks to the performance repo and keeping the performance in mind when working on the BCL! With the recent changes, the benchmarks look much better. Please see my new comments. As soon as they are addressed we are ready to merge. Thanks!
src/benchmarks/micro/libraries/System.Text.Json/Utf8JsonReader/Perf.Get.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.Json/Utf8JsonReader/Perf.Get.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.Json/Utf8JsonReader/Perf.Get.cs
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.Json/Utf8JsonReader/Perf.Get.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.Json/Utf8JsonReader/Perf.Get.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @jozkee !
Given that dotnet/runtime#38056 touches several
Utf8JsonReader.Get*
methods, I wanted to make sure that performance was not being affected and wrote these benchmarks.The results of these benchmarks can be used as a reference for future changes.
cc @layomia @steveharter