Skip to content

Add JSON benchmarks for missing-property and case-insensitive#1322

Merged
billwert merged 2 commits intodotnet:masterfrom
steveharter:BenchmarksCaseInsensitive
May 12, 2020
Merged

Add JSON benchmarks for missing-property and case-insensitive#1322
billwert merged 2 commits intodotnet:masterfrom
steveharter:BenchmarksCaseInsensitive

Conversation

@steveharter
Copy link
Contributor

Adds new benchmarks to track changes made in dotnet/runtime#35848

Verifies ~2x perf improvement on the missing-property and case-insensitive scenarios.

Before changes from the runtime PR linked above:

|                     Method |     Mean |     Error |    StdDev |   Median |      Min |      Max |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|-------:|------:|------:|----------:|
|        DeserializeBaseline | 1.209 us | 0.0139 us | 0.0130 us | 1.207 us | 1.186 us | 1.231 us | 0.0676 |     - |     - |     448 B |
|         DeserializeMissing | 1.593 us | 0.0161 us | 0.0150 us | 1.595 us | 1.557 us | 1.622 us | 0.0386 |     - |     - |     264 B |
|    DeserializeCaseMatching | 1.304 us | 0.0120 us | 0.0112 us | 1.301 us | 1.289 us | 1.329 us | 0.1169 |     - |     - |     752 B |
| DeserializeCaseNotMatching | 2.369 us | 0.0240 us | 0.0224 us | 2.371 us | 2.333 us | 2.411 us | 0.1426 |     - |     - |     928 B |

After:

|                     Method |       Mean |    Error |   StdDev |     Median |        Min |        Max |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |-----------:|---------:|---------:|-----------:|-----------:|-----------:|-------:|------:|------:|----------:|
|        DeserializeBaseline | 1,228.1 ns | 12.32 ns | 11.53 ns | 1,226.9 ns | 1,210.2 ns | 1,246.1 ns | 0.0685 |     - |     - |     448 B |
|         DeserializeMissing |   841.5 ns |  6.32 ns |  5.60 ns |   840.0 ns |   831.2 ns |   851.4 ns | 0.0134 |     - |     - |      88 B |
|    DeserializeCaseMatching | 1,224.4 ns | 12.83 ns | 12.00 ns | 1,224.7 ns | 1,209.8 ns | 1,246.2 ns | 0.0676 |     - |     - |     448 B |
| DeserializeCaseNotMatching | 1,234.0 ns |  9.58 ns |  8.49 ns | 1,233.8 ns | 1,223.1 ns | 1,249.1 ns | 0.0688 |     - |     - |     448 B |


[BenchmarkCategory(Categories.Libraries, Categories.JSON)]
[Benchmark]
public T DeserializeMissing() => JsonSerializer.Deserialize<T>(_serializedCamelCased, _optionsBaseline);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this? The current name took going to your original issue for me to understand.

Suggested change
public T DeserializeMissing() => JsonSerializer.Deserialize<T>(_serializedCamelCased, _optionsBaseline);
public T DeserializeWithMissingProperties() => JsonSerializer.Deserialize<T>(_serializedCamelCased, _optionsBaseline);

May also be worth a comment to make it clear that the properties will be "missing" because we're not using the case insensitive option.

Copy link
Contributor Author

@steveharter steveharter May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm one issue with names is that the results comparer truncates at 80 chars, so I've been trying to keep names small otherwise it may not be possible to distinguish between tests in the output.

However I didn't do a good job at that. It looks like System.Text.Json.Serialization.Tests.ReadMissingAndCaseInsensitive<Location>. is already at 77 characters leaving only 3 characters which currently is not even enough to distinguish between the 4 tests I added.

We could fix that issue with the results comparer, or I could simplify the class name and remove "DeserializeWith" but that still doesn't leave enough room to add "Properties".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix that in results comparer. @adamsitnik I assume this was a "nice output" choice and not something technical?

Our actual reporting backend does does not truncate these names (@DrewScoggins to correct me). I'm in favor of descriptive names so we should do what we need to in order to make that possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some renames and added comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't truncate.

Copy link
Contributor

@billwert billwert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change I'd like and then LGTM.

@steveharter steveharter requested a review from billwert May 12, 2020 15:41
@billwert
Copy link
Contributor

Thanks @steveharter! LGTM. We'll sort out the comparison tool separately.

@billwert billwert merged commit 39fc592 into dotnet:master May 12, 2020
ooooolivia pushed a commit to ooooolivia/performance that referenced this pull request May 13, 2020
…#1322)

* Add JSON benchmarks for missing and case-insensitive

* Rename tests
ooooolivia pushed a commit to ooooolivia/performance that referenced this pull request May 13, 2020
…#1322)

* Add JSON benchmarks for missing and case-insensitive

* Rename tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants