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

Support use of fast-path serialization in combined JsonSerializerContexts. #80741

Merged

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jan 17, 2023

Fixes #71933 by adding a new JsonTypeInfo.OriginatingResolver public property that is used by source generated JsonSerializerContext instances to register themselves with JsonTypeInfo instances that they generate. This information can be used to determine if the fast-path serialization delegate is compatible with the current configuration, even if the originating JsonSerializerContext is encapsulated behind a custom resolver.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@eiriktsarpalis eiriktsarpalis self-assigned this Jan 17, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jan 17, 2023
@ghost
Copy link

ghost commented Jan 17, 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

Fixes #71933 by adding a new JsonTypeInfo.OriginatingResolver public property that is used by source generated JsonSerializerContext instances to register themselves with JsonTypeInfo instances that they generate. This information can be used to determine if the fast-path serialization delegate is compatible with the current configuration, even if the originating JsonSerializerContext is encapsulated behind a custom resolver.

NB the new property hasn't been approved in API review yet.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: -

@krwq
Copy link
Member

krwq commented Jan 19, 2023

Do we actually need new API? I can see at least two ways we can do it without new API:

  • not assign SerializeHandler in cases where fast path should not be used - we can move the fast path checking logic to a generated context.
  • another option is we wrap every context with combined (as effective resolver, from user perspective they wouldn't know about it). Then combined one could check if all contexts are compatible between each other and with the options. Then combined context can decide if fast path can be used (so rather than checking if resolver is context we check if it's combined resolver and go from there)
    the second option might be easier and should be compatible with contexts generated in previous versions

@eiriktsarpalis
Copy link
Member Author

not assign SerializeHandler in cases where fast path should not be used - we can move the fast path checking logic to a generated context.

That might just work actually. We could remove the runtime invalidation altogether and just decide if the fast path should be assigned at the context layer.

@eiriktsarpalis
Copy link
Member Author

not assign SerializeHandler in cases where fast path should not be used - we can move the fast path checking logic to a generated context.

That might just work actually. We could remove the runtime invalidation altogether and just decide if the fast path should be assigned at the context layer.

Actually, I was wrong about this. We would still need to expose new API but even then this cannot account for contexts generated by older versions of the sdk; we still need to do this invalidation in the JsonTypeInfo layer.

@eiriktsarpalis eiriktsarpalis marked this pull request as draft January 30, 2023 18:29
@eiriktsarpalis
Copy link
Member Author

Converting to draft, as more infrastructural changes on JsonTypeInfo are required before we are able to implement proper invalidation.

@eiriktsarpalis eiriktsarpalis force-pushed the fix-fast-path-composition branch from e4a54f0 to 598634b Compare February 3, 2023 23:44
@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review February 3, 2023 23:47
@eiriktsarpalis
Copy link
Member Author

I've updated the PR and it should be ready for review again. @krwq & @layomia PTAL.

Comment on lines +667 to +682
if (_elementTypeInfo?.IsCompatibleWithCurrentOptions == false ||
_keyTypeInfo?.IsCompatibleWithCurrentOptions == false)
{
Copy link
Member

Choose a reason for hiding this comment

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

should you be checking IsConfigured here as well?

Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Feb 8, 2023

Choose a reason for hiding this comment

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

It could be false if you're configuring recursive types. Checking IsConfigured on JsonPropertyInfo is a different story, it completes regardless of cycles.

return OriginatingResolver switch
{
JsonSerializerContext ctx => ctx.IsCompatibleWithGeneratedOptions(Options),
DefaultJsonTypeInfoResolver => true, // generates default contracts by definition
Copy link
Member

@krwq krwq Feb 8, 2023

Choose a reason for hiding this comment

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

should this also check for Modifiers.Count == 0 and GetType() == typeof(DefaultJsonTypeInfoResolver)?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case you don't need to, any customization to the particular JsonTypeInfo would get picked up by the IsModified property.

@eiriktsarpalis eiriktsarpalis force-pushed the fix-fast-path-composition branch from 0be172d to 09f8062 Compare February 8, 2023 14:26
@eiriktsarpalis eiriktsarpalis merged commit 0304f1f into dotnet:main Feb 8, 2023
@eiriktsarpalis eiriktsarpalis deleted the fix-fast-path-composition branch February 8, 2023 19:44
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
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 fast path serialization not working with combined contexts
3 participants