-
Notifications
You must be signed in to change notification settings - Fork 256
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 (de)serializing dictionaries in System.Text.Json #530
Conversation
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.
Otherwise, LGTM. Can you please share the perf results from a local run?
I would be interested to see the ToString/ToUtf8Bytes difference just as a data point before we drop one.
|
||
[BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | ||
[Benchmark] | ||
public Dictionary<string, string> DeserializeDict() |
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.
Maybe there is a way to have a generic benchmark method with the type of the dictionary being a type parameter. All these tests are quite similar.
@adamsitnik, any suggestions on how we could set that up (or do you think the tests are fine as is)?
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.
@layomia have you considered adding the Dictionary and ImmutableDictionary to existing JSON serializer benchmarks?
You would need to add the type here:
performance/src/benchmarks/micro/corefx/System.Text.Json/Serializer/WriteJson.cs
Lines 13 to 17 in 2e05afc
[GenericTypeArguments(typeof(LoginViewModel))] | |
[GenericTypeArguments(typeof(Location))] | |
[GenericTypeArguments(typeof(IndexViewModel))] | |
[GenericTypeArguments(typeof(MyEventsListerViewModel))] | |
public class WriteJson<T> |
and implement the data generation in:
https://github.com/dotnet/performance/blob/master/src/benchmarks/micro/Serializers/DataGenerator.cs
If you do that you can quickly add the deserialization benchmarks to https://github.com/dotnet/performance/blob/master/src/benchmarks/micro/corefx/System.Text.Json/Serializer/ReadJson.cs
{ | ||
public class ReadDictionary | ||
{ | ||
private const string _jsonString = @"{""Hello"":""World"",""Hello2"":""World2""}"; |
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.
It would be useful to see perf of JSON with a lot more key/value pairs as well (say 100).
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 would also strongly recommend a bigger dictionary.
If we try to deserialize a very small one then most probably most of the time will be spend not in the deserializaiton of the dictionary but elsewhere: in searching for the type information, properies metadata etc.
BTW you can use
public static T[] ArrayOfUniqueValues<T>(int count) |
Dictionary<string, string> dictionary = ValuesGenerator.ArrayOfUniqueValues<string>(100).ToDictionary(value => value);
[Benchmark] | ||
public string SerializeIDict_ToString() | ||
{ | ||
return JsonSerializer.ToString(_iDict); |
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.
In the interest of avoiding test bloat (and only testing a single dimension), it's probably not useful to add the ToString()
tests for all the specialized test cases (like WriteJson.Dictionary), when we have the ToUtf8Bytes
tests already. I think the overhead of transcoding the end result to UTF-16 is well understood (and captured by the existing tests).
{ | ||
private Dictionary<string, string> _dict = new Dictionary<string, string>() { { "Hello", "World" }, { "Hello2", "World2" } }; | ||
private static IDictionary<string, string> _iDict = new Dictionary<string, string>() { { "Hello", "World" }, { "Hello2", "World2" } }; | ||
private IReadOnlyDictionary<string, string> _iReadOnlyDict = new Dictionary<string, string>() { { "Hello", "World" }, { "Hello2", "World2" } }; |
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.
Similarly, writing a larger dictionary would be good to see. You could set it up in the GlobalSetup
as well, maybe with a Params for KeyCount, with 2 and 100 as reasonable values.
See these sections over at dotnet/corefx#37710:
|
Gotcha: dotnet/corefx#37710 (comment) S.T.Json:
Json.Net:
How come we are slower for serialization? It also looks like we are slower for serializing and deserializing immutable collections.
We should consider doing this or find some other ways to optimize this. We are quite a bit slower (almost 50%). @steveharter, any ideas? |
I can spend some time looking at this as part of testing/validation next week and beyond. @steveharter might be able to provide some insight now.
Agreed. Json.NET uses IL emit extensively for immutable collections: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Utilities/DynamicReflectionDelegateFactory.cs. |
|
||
[BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | ||
[Benchmark] | ||
public IReadOnlyDictionary<string, string> DeserializeIReadOnlyDict() |
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.
is IReadOnlyDictionary
testing a different code path than IDictionary
or Dictionary
? If it's the same code path on the serializer side, I would stay only with Dictionary
|
||
[BenchmarkCategory(Categories.CoreFX, Categories.JSON)] | ||
[Benchmark] | ||
public IImmutableDictionary<string, string> DeserializeIImmutableDict() |
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.
same as above: what is the difference in executed code between ImmutableDictionary and IImmutableDictionary?
@layomia ping |
Thanks. I'll address the feedback on this shortly and apply it to related PRs. |
@layomia similar to other stale PRs (#478, #589) I have cloned your fork, updated the code and pushed some fixes. I need to make sure that we don't regress it for 3.0. PTAL at my changes and the doc that I've recently written https://github.com/dotnet/performance/blob/master/docs/microbenchmark-design-guidelines.md |
sample results: dotnet run -c Release -f netcoreapp3.0 --filter *WriteJson<*Dictionary*>* *ReadJson<*Dictionary*>* --join BenchmarkDotNet=v0.11.3.1003-nightly, OS=Windows 10.0.18362
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.0.100-preview7-012697
[Host] : .NET Core 3.0.0-preview7-27826-20 (CoreCLR 4.700.19.32603, CoreFX 4.700.19.32613), 64bit RyuJIT
Job-WTATHE : .NET Core 3.0.0-preview7-27826-20 (CoreCLR 4.700.19.32603, CoreFX 4.700.19.32613), 64bit RyuJIT
IterationTime=250.0000 ms MaxIterationCount=20 MinIterationCount=15
WarmupCount=1
|
@adamsitnik LGTM, thanks! |
cc @ahsonkhan @adamsitnik @billwert