-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Minor deserialization perf improvements for collections #40889
Conversation
yield return (TRuntimeProperty)item; | ||
} | ||
} | ||
|
||
private IEnumerable<TDeclaredProperty> CreateGenericTDeclaredPropertyIEnumerable(IList sourceList) | ||
{ | ||
foreach (object item in sourceList) | ||
{ | ||
yield return (TDeclaredProperty)item; | ||
} | ||
} |
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.
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!; | |
} | |
} | |
} |
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 can you go through this code and update the casts per suggestion above?
{ | ||
IDictionary collection = null; | ||
|
||
if (!options.TryGetCreateRangeDelegate(delegateKey, out ImmutableCollectionCreator creator) || | ||
!creator.CreateImmutableDictionary(sourceDictionary, out collection)) | ||
{ | ||
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionType, jsonPath); | ||
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(collectionType, state.JsonPath); |
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.
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
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. 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.
@@ -304,41 +304,25 @@ public override IEnumerable CreateImmutableCollectionInstance(Type collectionTyp | |||
// Creates an IEnumerable<TRuntimePropertyType> and populates it with the items in the | |||
// 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) |
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.
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.
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.
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)
I'll create a PR incorporating #40889 (comment) & applicable pieces from #40889 (comment) when this PR is merged. |
Benchmarks for deserializing single collection
System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<string, string>>