Minor deserialization perf improvements for collections by steveharter · Pull Request #40889 · dotnet/corefx

steveharter · 2019-09-06T15:06:10Z

Defer calls to ReadStack.JsonPath until an exception needs that information (avoids String and StringBuilder allocs).
Remove dead code and unused variables

Benchmarks for deserializing single collection System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<string, string>>

Before
|                   Method |     Mean |     Error |    StdDev |   Median |      Min |      Max | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------------:|------------:|------------:|--------------------:|
|    DeserializeFromString | 65.60 us | 0.6393 us | 0.5668 us | 65.50 us | 64.64 us | 66.60 us |      6.6489 |      0.5319 |           - |            40.87 KB |
| DeserializeFromUtf8Bytes | 63.73 us | 0.9506 us | 0.8891 us | 63.58 us | 62.40 us | 65.20 us |      5.5444 |      0.5040 |           - |            35.07 KB |
|    DeserializeFromStream | 64.76 us | 1.2135 us | 1.0758 us | 64.87 us | 63.33 us | 66.52 us |      5.6921 |      0.5175 |           - |            35.14 KB

After
|                   Method |     Mean |     Error |    StdDev |   Median |      Min |      Max | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------------:|------------:|------------:|--------------------:|
|    DeserializeFromString | 64.71 us | 0.8625 us | 0.8067 us | 64.36 us | 63.72 us | 66.20 us |      6.4317 |      0.7718 |           - |            40.75 KB |
| DeserializeFromUtf8Bytes | 62.82 us | 0.6094 us | 0.5701 us | 62.72 us | 62.14 us | 63.98 us |      5.4781 |      0.7470 |           - |            34.94 KB |
|    DeserializeFromStream | 63.31 us | 0.6213 us | 0.5812 us | 63.20 us | 62.42 us | 64.38 us |      5.5710 |      0.7597 |           - |            35.01 KB |

stephentoub · 2019-09-06T19:11:21Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs

            {
                yield return (TDeclaredProperty)item;
            }
        }


Unrelated to your PR, but you might consider using Enumerable.Cast instead of this method:

corefx/src/System.Linq/src/System/Linq/Cast.cs

Lines 33 to 55 in bca5498

public static IEnumerable<TResult> Cast<TResult>(this IEnumerable source)

{

if (source is IEnumerable<TResult> typedSource)

{

return typedSource;

}

if (source == null)

{

ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);

}

return CastIterator<TResult>(source);

}

private static IEnumerable<TResult> CastIterator<TResult>(IEnumerable source)

{

foreach (object? obj in source)

{

yield return (TResult)obj!;

}

}

}

@layomia can you go through this code and update the casts per suggestion above?

ahsonkhan · 2019-09-07T02:50:46Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs

                !creator.CreateImmutableDictionary(sourceDictionary, out collection))
            {
-                ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionType, jsonPath);
+                ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionType, state.JsonPath);


nit: I was playing around with the deserializer code, and I think JsonPath should be changed to a method rather than being a property (GetJsonPath()). Making it a property gives the wrong impression that its cheap, when it is actually doing the work to build the path.

It might be worth taking pieces from this commit and incorporate it into yours:
ahsonkhan@c76bbd6

Thanks. Yes the main reason for this PR is because JsonPath is not cheap. I'm make the changes.

Also making a method vs property affects debuggability, so I'll update the [DebuggerDisplay] to call the method.

ahsonkhan · 2019-09-07T02:51:28Z

src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs

        // sourceList argument then uses the delegateKey argument to identify the appropriate cached
        // CreateRange<TRuntimePropertyType> method to create and return the desired immutable collection type.
-        public override IDictionary CreateImmutableDictionaryInstance(Type collectionType, string delegateKey, IDictionary sourceDictionary, string jsonPath, JsonSerializerOptions options)
+        public override IDictionary CreateImmutableDictionaryInstance(ref ReadStack state, Type collectionType, string delegateKey, IDictionary sourceDictionary, JsonSerializerOptions options)


Why re-order the parameters? Why not replace the string jsonPath parameter with ref ReadStack state in the same order? This applies for all the changes here.

The ReadStack and jsonPath parameters are not equivalent.

Having ReadStack first is being consistent with the calling methods (e.g. CreateFromDictionary(ref ReadStack state, IDictionary sourceDictionary, JsonSerializerOptions options)

layomia · 2019-09-09T15:41:15Z

I'll create a PR incorporating #40889 (comment) & applicable pieces from #40889 (comment) when this PR is merged.

…x#40889) Commit migrated from dotnet/corefx@27ddbe9

Misc collection perf improvements

5dcff2b

steveharter added tenet-performance Performance related issue area-System.Text.Json labels Sep 6, 2019

steveharter added this to the 5.0 milestone Sep 6, 2019

steveharter requested review from ahsonkhan and layomia September 6, 2019 15:06

steveharter self-assigned this Sep 6, 2019

stephentoub reviewed Sep 6, 2019

View reviewed changes

ahsonkhan reviewed Sep 7, 2019

View reviewed changes

Make path properties methods instead

30fac85

steveharter merged commit 27ddbe9 into dotnet:master Sep 9, 2019

steveharter deleted the MiscPerf branch September 9, 2019 21:55

steveharter added a commit that referenced this pull request Oct 10, 2019

Minor deserialization perf improvements for collections (#40889)

21ed8ff

steveharter added a commit to steveharter/dotnet_corefx that referenced this pull request Oct 14, 2019

Minor deserialization perf improvements for collections (dotnet#40889)

e71bb5c

steveharter added a commit to steveharter/dotnet_corefx that referenced this pull request Oct 14, 2019

Minor deserialization perf improvements for collections (dotnet#40889)

aaf7fc0

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Minor deserialization perf improvements for collections (dotnet/coref…

e364fa1

…x#40889) Commit migrated from dotnet/corefx@27ddbe9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor deserialization perf improvements for collections#40889

Minor deserialization perf improvements for collections#40889
steveharter merged 2 commits intodotnet:masterfrom
steveharter:MiscPerf

steveharter commented Sep 6, 2019

Uh oh!

stephentoub Sep 6, 2019

Uh oh!

steveharter Sep 6, 2019

Uh oh!

ahsonkhan Sep 7, 2019

Uh oh!

steveharter Sep 9, 2019 •

edited

Loading

Uh oh!

ahsonkhan Sep 7, 2019

Uh oh!

steveharter Sep 9, 2019

Uh oh!

layomia commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	public static IEnumerable<TResult> Cast<TResult>(this IEnumerable source)
	{
	if (source is IEnumerable<TResult> typedSource)
	{
	return typedSource;
	}

	if (source == null)
	{
	ThrowHelper.ThrowArgumentNullException(ExceptionArgument.source);
	}

	return CastIterator<TResult>(source);
	}

	private static IEnumerable<TResult> CastIterator<TResult>(IEnumerable source)
	{
	foreach (object? obj in source)
	{
	yield return (TResult)obj!;
	}
	}
	}

Conversation

steveharter commented Sep 6, 2019

Uh oh!

stephentoub Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

steveharter Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

steveharter Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Sep 7, 2019

Choose a reason for hiding this comment

Uh oh!

steveharter Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

layomia commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

steveharter Sep 9, 2019 •

edited

Loading