-
Notifications
You must be signed in to change notification settings - Fork 5k
Conversation
Can you also measure and add (and share the results) for other polymorphic test cases outside of public class Foo
{
public IEnumerable<int> Prop { get; set; } // or some POCO instead of int if needed
}
public class FooObject
{
public object Prop { get; set; }
}
public void BuildObjects()
{
var model = new List<int>();
for (int i = 0; i < 100; i++) // arbitrary length chosen
{
model.Add(i);
}
_value = new Foo
{
Prop = model
};
_valueObject = new FooObject
{
Prop = model
};
} |
Is there any case for deserialization where we might have a similar problem that needs to be fixed (such as dealing with polymorphic properties other than @steveharter - can you think of other areas in the serializer code that could have similar issues where one-time operations end up running every time, potentially in edge cases? |
return runtimeProperty; | ||
} | ||
|
||
var cache = LazyInitializer.EnsureInitialized(ref property.RuntimePropertyCache, () => 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.
nit: Don't use var here.
|
||
return runtimeProperty; | ||
return cache.GetOrAdd(runtimePropertyType, (type, arg) => CreateRuntimeProperty(type, arg), (property, options, Type)); |
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.
@steveharter, unrelated to this PR, but here is some code cleanup we should consider (separately - out of scope for this PR which we should keep focused on the perf fix only):
Using Type
as a property name makes code like this quite hard to parse/reason about. I was confused where Type
is coming from and how we were passing in System.Type
(i.e. a type) as a method parameter.
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; } |
@@ -584,5 +585,7 @@ private void VerifyWrite(int originalDepth, Utf8JsonWriter writer) | |||
ThrowHelper.ThrowJsonException_SerializationConverterWrite(ConverterBase); | |||
} | |||
} | |||
|
|||
public ConcurrentDictionary<Type, JsonPropertyInfo> RuntimePropertyCache; |
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?
Also, can the following existing cache be used for the runtime properties as well?
corefx/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
Line 23 in 437d9bc
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.
As far as I can tell, such cases don't currently behave polymorphically, so I don't think it makes sense to measure them. For example, consider the following code: using System;
using System.Text.Json;
class A
{
public string P1 => "Base";
}
class B : A
{
public string P2 => "Derived";
}
class Program
{
static void Main()
{
Console.WriteLine(JsonSerializer.Serialize(new { Prop = (A)new B() }));
Console.WriteLine(JsonSerializer.Serialize(new { Prop = (object)new B() }));
}
} This prints:
Which shows that a property statically typed as Or maybe I misunderstood what you meant?
What is the part that you're missing from the test I added? Non-trivial number of items in the collection? Named types? A POCO type in the collection? All of the above? I don't think any of those are relevant here. I'll look into responding to the rest of the comments later, hopefully tomorrow |
Ah yes. Thanks for reminding me that we don't use the runtime type for serializing polymorphic types (outside of
I was looking for a test where a collection property is treated as
Sounds good. |
We only support polymorphic serialization today when the property type is System.Object, so currently this PR only helps with that. When we add additional support then this scenario will be more common.
So class class B:A doesn't have a perf issue today and this PR won't help\change that. |
{ | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests with different properties with the same Type
but with different attributes (e.g. [JsonIgnore]
) to ensure the cache's key works as expected.
return runtimeProperty; | ||
} | ||
|
||
var cache = LazyInitializer.EnsureInitialized(ref property.RuntimePropertyCache, () => 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.
Is this preferred over Lazy<T>
?
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 think it's preferred here, since Lazy<T>
would require an allocation of an instance of Lazy<T>
for every instance of JsonPropertyInfo
(even if it didn't have any polymorphic properties).
But that's a moot point anyway if we're going to reuse JsonClassInfo.PropertyCache
(more on that later).
Currently we JIT the delegates but there is a fallback that uses reflection (see "MemberAccessorStrategy") for cases that can't JIT. I wonder if reflection is more lightweight upfront and would work without having to cache although I suspect that caching (and hashtable lookup) would still be faster, but probably not the full 68x as quoted earlier. |
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 for the PR.
See the comment on moving cache from JsonPropertyInfo to JsonClassInfo (e.g. in string format of something like "{PropertyName}_{PropertyType}" and the comment about adding additional tests to ensure cache is property keying correctly.
@svick our internal cutoff for 3.1 Preview 2 is October 23rd meaning this PR would need to likely get into master this week for us to have time to port to 3.1. |
1f54727
to
2f0cf17
Compare
@steveharter I think I have addressed all the comments. I will have limited computer access until Monday, so I probably won't be able to address any new comments until then. Though feel free to add commits to this PR if that becomes necessary. |
src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: Ahson Khan <ahkha@microsoft.com>
https://dev.azure.com/dnceng/public/_build/results?buildId=392129
Edit: This API is only available on Netstandard2.1 and isn't available on netstandard 2.0. |
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.
|
||
return runtimeProperty; | ||
return cache.GetOrAdd((property, runtimePropertyType), (key, arg) => CreateRuntimeProperty(key, arg), (options, Type)); |
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.
Specify the generic explicitly?
return cache.GetOrAdd((property, runtimePropertyType), (key, arg) => CreateRuntimeProperty(key, arg), (options, Type)); | |
return cache.GetOrAdd<(JsonSerializerOptions, Type)>((property, runtimePropertyType), (key, arg) => CreateRuntimeProperty(key, arg), (options, Type)); |
Edit: Nevermind
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.
This API isn't available on netstandard 2.0, which is why the build is failing.
Can you #if/def the code using BUILDING_INBOX_LIBRARY and in the else block, only use netstandard2.0 compatible APIs?
I think I have fixed the error. The CI failures don't seem related to this PR. |
Filed an issue for the unrelated test failures: |
@svick, can you please resolve the merge conflict so we can get this checked in? Thanks. |
@ahsonkhan The conflict should be resolved. |
* Cache polymorphic properties * Move RuntimePropertyCache to JsonClassInfo * Added test of RuntimePropertyCache using properties with different attributes * Fixed typo Co-Authored-By: Ahson Khan <ahkha@microsoft.com> * Use allocating overload of GetOrAdd on .Net Standard 2.0
* Cache polymorphic properties * Move RuntimePropertyCache to JsonClassInfo * Added test of RuntimePropertyCache using properties with different attributes * Fixed typo Co-Authored-By: Ahson Khan <ahkha@microsoft.com> * Use allocating overload of GetOrAdd on .Net Standard 2.0
"This should be an okay thing to do, since most polymorphic properties should only use a small number of types" What happens when its many types especially some people will use interfaces of interfaces . Maybe an LRU list be used so dont need to deal with this or should this dont use many types be documented ? |
* Cache polymorphic properties * Move RuntimePropertyCache to JsonClassInfo * Added test of RuntimePropertyCache using properties with different attributes * Fixed typo Co-Authored-By: Ahson Khan <ahkha@microsoft.com> * Use allocating overload of GetOrAdd on .Net Standard 2.0 Commit migrated from dotnet/corefx@4629961
Fixes https://github.com/dotnet/corefx/issues/41638.
When a polymorphic property (i.e. one whose type is
object
) is serialized, itsJsonPropertyInfo
is created every time, based on the runtime type of the property. This is a problem, because it means accessing the property will JIT compile theGet
delegate every time, which takes a lot of time.This PR changes that, by caching the
JsonPropertyInfo
for each encountered runtime type. This should be an okay thing to do, since most polymorphic properties should only use a small number of types.One case where this could cause an issue is if doing this prevents the type from unloading. But, as far as I can tell, that is already a problem with System.Text.Json, and one that can be solved by switching the instance of
JsonSerializerOptions
that's used.After adding a benchmark for this case the results of comparing System.Text.Json.Serialization microbenchmarks show clear improvement (up to 68x) for the new benchmark and no real change for other benchmarks:
summary:
better: 17, geomean: 5.809
worse: 4, geomean: 1.041
total diff: 21