-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Cache polymorphic properties #41753
Cache polymorphic properties #41753
Changes from 1 commit
5d363fa
92b9b1e
2f0cf17
826bc61
df45181
e72ba56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,11 @@ | |||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||
| // See the LICENSE file in the project root for more information. | ||||||
|
|
||||||
| using System.Collections.Generic; | ||||||
| using System.Collections.Concurrent; | ||||||
| using System.Diagnostics; | ||||||
| using System.Reflection; | ||||||
| using System.Text.Json.Serialization; | ||||||
| using System.Threading; | ||||||
|
|
||||||
| namespace System.Text.Json | ||||||
| { | ||||||
|
|
@@ -209,18 +210,26 @@ internal JsonPropertyInfo CreateRootObject(JsonSerializerOptions options) | |||||
| options: options); | ||||||
| } | ||||||
|
|
||||||
| internal JsonPropertyInfo CreatePolymorphicProperty(JsonPropertyInfo property, Type runtimePropertyType, JsonSerializerOptions options) | ||||||
| internal JsonPropertyInfo GetOrAddPolymorphicProperty(JsonPropertyInfo property, Type runtimePropertyType, JsonSerializerOptions options) | ||||||
| { | ||||||
| JsonPropertyInfo runtimeProperty = CreateProperty( | ||||||
| property.DeclaredPropertyType, runtimePropertyType, | ||||||
| property.ImplementedPropertyType, | ||||||
| property.PropertyInfo, | ||||||
| parentClassType: Type, | ||||||
| converter: null, | ||||||
| options: options); | ||||||
| property.CopyRuntimeSettingsTo(runtimeProperty); | ||||||
| static JsonPropertyInfo CreateRuntimeProperty(Type runtimePropertyType, (JsonPropertyInfo property, JsonSerializerOptions options, Type classType) arg) | ||||||
| { | ||||||
| JsonPropertyInfo runtimeProperty = CreateProperty( | ||||||
| arg.property.DeclaredPropertyType, runtimePropertyType, | ||||||
| arg.property.ImplementedPropertyType, | ||||||
| arg.property.PropertyInfo, | ||||||
| parentClassType: arg.classType, | ||||||
| converter: null, | ||||||
| options: arg.options); | ||||||
|
|
||||||
| arg.property.CopyRuntimeSettingsTo(runtimeProperty); | ||||||
|
|
||||||
| return runtimeProperty; | ||||||
| } | ||||||
|
|
||||||
| var cache = LazyInitializer.EnsureInitialized(ref property.RuntimePropertyCache, () => new ConcurrentDictionary<Type, JsonPropertyInfo>()); | ||||||
|
||||||
|
|
||||||
| return runtimeProperty; | ||||||
| return cache.GetOrAdd(runtimePropertyType, (type, arg) => CreateRuntimeProperty(type, arg), (property, options, Type)); | ||||||
|
||||||
| public Type Type { get; private set; } |
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 "ClassType" better? (somewhat redundant) Or just doc? The actual type can be a POCO type or any property type (string, List<string>, etc).
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 already have a ClassType property, so that wouldn't work.
| public ClassType ClassType { get; private set; } |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||
| // See the LICENSE file in the project root for more information. | ||||
|
|
||||
| using System.Collections; | ||||
| using System.Collections.Concurrent; | ||||
| using System.Diagnostics; | ||||
| using System.Reflection; | ||||
| using System.Text.Json.Serialization; | ||||
|
|
@@ -584,5 +585,7 @@ private void VerifyWrite(int originalDepth, Utf8JsonWriter writer) | |||
| ThrowHelper.ThrowJsonException_SerializationConverterWrite(ConverterBase); | ||||
| } | ||||
| } | ||||
|
|
||||
| public ConcurrentDictionary<Type, JsonPropertyInfo> RuntimePropertyCache; | ||||
|
||||
| private readonly ConcurrentDictionary<Type, JsonPropertyInfo> _objectJsonProperties = new ConcurrentDictionary<Type, JsonPropertyInfo>(); |
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.
On _objectJsonProperties I believe @layomia is working to get rid of that cache.
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.
Should this be stored on the JsonSerializerOptions instead with the rest of the caches?
By storing the cache on the actual JsonPropertyInfo like it is currently means we are tied to a particular class + particular property that may have specific attribute (e.g. [JsonIgnore] and [JsonConverter] which we need to preserve.
If we store the cache on a JsonSerializerOptions instance then we would would need to key on class type + property name + property type.
If we store the cache on a JsonClassInfo instance then we would need to key on property name + property type.
The cache in the current location of a JsonPropertyInfo instance is the most specific, but that also means we are instantiating a new dictionary for every polymorphic property, which probably isn't ideal, especially in the future when we likely support polymorphic properties in cases other than System.Object.
So I think storing on JsonClassInfo is the best location like we have for the current property cache already there: public volatile Dictionary<string, JsonPropertyInfo> PropertyCache. However, we should be able to just use the existing cache by changing the "string" key to be property name + property type (just for polymorphic properties) then we can just re-use the existing cache.
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 should be able to just use the existing cache
I don't think that would be a good idea, since it would require allocating a string for every access to serve as the key. So you're likely paying lots of string allocations to avoid a single dictionary allocation.
Just moving the cache to JsonClassInfo (while using a (name, type) tuple as the key) sounds reasonable to me.
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.
@svick yes allocating a string for the key is not ideal, but small overhead compared to what we have today. A dictionary is heavy-weight so having one instance for a Type is much better than per property.
However creating a new cache on JsonClassInfo with the propertyName+propertyType tuple as you suggest would be OK, but JsonPropertyInfo+propertyType might be a bit more performant and works because the same JsonPropertyInfo coming in is (should) be the same instance each time.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,5 +75,12 @@ public int this[int index] | |
|
|
||
| public string NonIndexerProp { get; set; } | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void WriteCollectionAsObject() | ||
| { | ||
| string json = JsonSerializer.Serialize(new { Prop = (object)new[] { 0 } }); | ||
| Assert.Equal(@"{""Prop"":[0]}", json); | ||
| } | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add tests with different properties with the same |
||
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: Don't use var here.