Skip to content
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

Open
eiriktsarpalis opened this issue Feb 27, 2023 · 5 comments
Open

Add generic JsonPropertyInfo metadata type(s) #82720

eiriktsarpalis opened this issue Feb 27, 2023 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json tenet-performance Performance related issue
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 27, 2023

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 the Get, Set and ShouldSerialize delegates:

namespace System.Text.Json.Serialization.Metadata;

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; }
}

Alternative design

We also discussed this alternative:

namespace System.Text.Json.Serialization.Metadata;

public class JsonPropertyInfo<TDeclaringType, TPropertyType> : JsonPropertyInfo
{
    public new Func<TDeclaringType, TPropertyType, bool> ShouldSerialize { get; set; }

    public new Func<TDeclaringType, TPropertyType> Get { get; set; }
    public new PropertySetter<TDeclaringType, TPropertyType> Set { get; set; }
}

public delegate void PropertySetter<TDeclaringType, TPropertyType>(ref TDeclaringType obj, TPropertyType value);

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 future PropertyInfo<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

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 27, 2023
@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Feb 27, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Feb 27, 2023
@ghost
Copy link

ghost commented Feb 27, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

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 the Get, Set and ShouldSerialize delegates:

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.
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 future PropertyInfo<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

Author: eiriktsarpalis
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: Future

@BreyerW
Copy link

BreyerW commented Feb 28, 2023

What about going with hybrid approach? More precisely ShouldSerialize and Get would use object while Set would use ref TDeclaringType. Would that reduce static footprint increase to more palatable levels? Or is main source of static footprint increase on class itself due to extra generic? It would be shame to lose about 10% - 25% perf on almost every scenario compared to object version. I have excluded Get since it doesnt return by ref so i think there wont be that big difference unless you used custom delegate with ref readonly or just ref return. ShouldSerialize is somewhat niche i think so its no brain to leave as object if that would reduce static footprint increase

@eiriktsarpalis
Copy link
Member Author

Or is main source of static footprint increase on class itself due to extra generic?

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.

@BreyerW
Copy link

BreyerW commented Mar 1, 2023

@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 JsonPropertyInfo<TDeclaringType, TPropertyType> while built-in System.Text.Json gets object + 1 generic version?

Or maybe flip the idea completely: dont introduce hybrid object + 1 generic at all but the fastest version guarded by switch or stored in separate nuget package or some other delivery device that im not aware of.

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.

@eiriktsarpalis
Copy link
Member Author

Throwing spaghetti at the wall to see if some stick: would it be possible to create nuget package that would enable fastest JsonPropertyInfo<TDeclaringType, TPropertyType> while built-in System.Text.Json gets object + 1 generic 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

2 participants