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

Disable fast path serialization for types with properties using custom converters. #58291

Conversation

eiriktsarpalis
Copy link
Member

Disables fast path serialization for types with properties using custom converters. While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public. We should consider doing this in .NET 7.

Fixes #58119.

@ghost
Copy link

ghost commented Aug 27, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Disables fast path serialization for types with properties using custom converters. While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public. We should consider doing this in .NET 7.

Fixes #58119.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 6.0.0

@eiriktsarpalis
Copy link
Member Author

Should be backported to release/6.0

@steveharter
Copy link
Member

While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public

Why is this? I assume we just need to call myconverter.Write(.

@eiriktsarpalis
Copy link
Member Author

While technically it should be possible to support fast path serialization for this scenario, it would require making the JsonPropertyInfo.ConverterBase property public

Why is this? I assume we just need to call myconverter.Write(.

property-level converters need to be instantiated and cached with property-level metadata (otherwise we would have to allocate a new converter on each serialization). We currently do cache a JsonPropertyInfo[], however the converter property in JsonPropertyInfo has not been made public yet (and therefore cannot be consumed by the fast path serializer). If we wanted to get this working without adding new APIs we would probably need to create a separate JsonConverter[] but that would come at the cost of added allocations and redundant code.

@steveharter
Copy link
Member

The long-term and more ideal implementation is to use the property-level converter in the fast-path, meaning add the new serializer-gen interop API (it will need to be public, but treated as internal). If we are going to add any other new interop APIs then I think we should use the converter in the fast path, otherwise I think the PR here is fine.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, we should disable fast path and use serializer, and fix this in 7.0.

Also this PR needs to get in before the "property factory" support in #58267 is added, since fast path also needs to be disabled for properties that have factory converters.

@steveharter steveharter merged commit 7b32a1a into dotnet:main Sep 2, 2021
@steveharter
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1195028196

@eiriktsarpalis eiriktsarpalis deleted the sourcegen-propertyconverter-disablefastpath branch September 3, 2021 17:40
@ghost ghost locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json code generator: custom converter ignored?
3 participants