-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generic JsonPropertyInfo metadata type(s) #82720
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsUsers working on the contract model should have access to the currently internal public class JsonPropertyInfo<TPropertyType> : JsonPropertyInfo
{
public new Func<object, TPropertyType, bool> ShouldSerialize { get; set; }
public new Func<object, TPropertyType> Get { get; set; }
public new Action<object, TPropertyType> Set { get; set; }
} JsonPropertyInfo vs JsonPropertyInfo<TPropertyType> vs JsonPropertyInfo<TDeclaringType, TPropertyType>We have considered different approaches here and it all boils down to perf of the property setter.
proves to be overall fastest. Current implementation would require a bit of work for this to be changed and such support can be added later. Given above we've decided to for a time being support only non-generic Here are benchmark results: https://gist.github.com/krwq/d9d1bad3d59ff30f8db2a53a27adc755
|
What about going with hybrid approach? More precisely |
This. Having two type parameters generally results in quadratic increase of the static footprint. We recently removed such a type at the expense of performance in certain scenaria because it was contributing to over 1 MB of NativeAOT application sizes. |
@eiriktsarpalis so this particular idea goes out of the window. Too bad. Throwing spaghetti at the wall to see if some stick: would it be possible to create nuget package that would enable fastest Or maybe flip the idea completely: dont introduce hybrid The idea here is to have no change in size for those who care about AoT size (cloud, IoT, mobile etc.) while those who care about speed (desktop, traditional servers etc.) would enable explicitly fastest but fattest version. |
Not really. Unless the built-in converter infrastructure can actually see those generic types there wouldn't be much use for the strongly typed delegates. FWIW the internal JsonPropertyInfo implementation follows the first variant already -- it might be boxing the declaring type but at least it ensures that this only happens once per serialization or deserialization: Line 280 in 14bf690
|
Users working on the contract model should have access to the currently internal
JsonPropertyInfo<TPropertyType>
class. This would let them write allocation-free code for theGet
,Set
andShouldSerialize
delegates:Alternative design
We also discussed this alternative:
It's unlikely we'd pursue this since it would increase the static footprint in NativeAOT due to the large number of generic specializations it would incur.
Notes on Performance (taken from #63686)
We have considered different approaches here and it all boils down to perf of the property setter.
According to simple perf tests run on different combinations of declaring types and property types as well 4 different approaches of setters using setter in form of:
delegate void PropertySetter<DeclaringType, PropertyType>(ref DeclaringType obj, PropertyType val);
proves to be overall fastest. Current implementation would require a bit of work for this to be changed and such support can be added later. Given above we've decided to for a time being support only non-generic
PropertyInfo
with the slowest setter since such type already exists and corresponding setter would have to be added regardless of choice. In the futurePropertyInfo<TDeclaringType, TPropertyType>
should be added to support for the fastest possible case.Here are benchmark results: https://gist.github.com/krwq/d9d1bad3d59ff30f8db2a53a27adc755
Here is the benchmark code: https://gist.github.com/krwq/eb06529f0c99614579f84b69720ab46e
The text was updated successfully, but these errors were encountered: