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

Fix generic recursion condition when calling DeserializeAsyncEnumerable in NativeAOT. #85176

Merged
merged 2 commits into from
Apr 22, 2023

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Apr 21, 2023

This PR makes the following changes:

  1. Moves the JsonTypeInfo<Queue<T>> factory from JsonTypeInfo<T> to JsonSerializer to avoid triggering polymorphic recursion.
  2. Removes the non-generic DeserializeAsyncEnumerable APIs added in Preview 1 via Add weakly typed Serialize and Deserialize overloads accepting JsonTypeInfo. #79397. Supporting that feature necessitates every JsonTypeInfo<T> encapsulating the factory for JsonTypeInfo<Queue<T>> triggering the recursion condition.

I've validated that the AOT warning no longer occurs using manual testing.

Fix #84922.

cc @Sergio0694

@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, 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.

@ghost
Copy link

ghost commented Apr 21, 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

This PR makes the following changes:

  1. Moves the JsonTypeInfo<Queue<T>> factory from JsonTypeInfo<T> to JsonSerializer to avoid triggering polymorphic recursion.
  2. Removes the non-generic DeserializeAsyncEnumerable APIs added in Preview 1 via Add weakly typed Serialize and Deserialize overloads accepting JsonTypeInfo. #79397. Supporting that feature necessitates every JsonTypeInfo<T> encapsulating the factory for JsonTypeInfo<Queue<T>> triggering the recursion condition.
Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

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

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Apr 21, 2023
@Sergio0694
Copy link
Contributor

Thank you!! Will validate with the Store too and report back 🙂

@eiriktsarpalis eiriktsarpalis added the size-reduction Issues impacting final app size primary for size sensitive workloads label Apr 21, 2023
@ghost
Copy link

ghost commented Apr 21, 2023

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR makes the following changes:

  1. Moves the JsonTypeInfo<Queue<T>> factory from JsonTypeInfo<T> to JsonSerializer to avoid triggering polymorphic recursion.
  2. Removes the non-generic DeserializeAsyncEnumerable APIs added in Preview 1 via Add weakly typed Serialize and Deserialize overloads accepting JsonTypeInfo. #79397. Supporting that feature necessitates every JsonTypeInfo<T> encapsulating the factory for JsonTypeInfo<Queue<T>> triggering the recursion condition.

I've validated that the AOT warning no longer occurs using manual testing.

Fix #84922.

cc @Sergio0694

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

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

Milestone: 8.0.0

@jkotas
Copy link
Member

jkotas commented Apr 21, 2023

LGTM once @Sergio0694 confirms that it fixes the problem for store. Thank you!

@MichalPetryka
Copy link
Contributor

LGTM once @Sergio0694 confirms that it fixes the problem for store. Thank you!

#84922 (comment)

Copy link
Contributor

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Just commented in the other thread, LGTM!! :shipit:

@eiriktsarpalis thank you!! 🙌

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the pending feedback, LGTM

@eiriktsarpalis eiriktsarpalis merged commit 4da4aaf into dotnet:main Apr 22, 2023
@eiriktsarpalis eiriktsarpalis deleted the fix/polymorphic-recursion branch April 22, 2023 18:38
@eiriktsarpalis eiriktsarpalis removed new-api-needs-documentation size-reduction Issues impacting final app size primary for size sensitive workloads labels Apr 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 22, 2023
@eiriktsarpalis eiriktsarpalis added the size-reduction Issues impacting final app size primary for size sensitive workloads label May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8.4 MB size regression updating System.Text.Json to 8.0.0
6 participants