Skip to content

Conversation

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Feb 22, 2022

Makes a few infrastructural changes in anticipation of #63747. More specifically

  • Makes ObjectConverter (the polymorphic converter for object) use ConverterStrategy.Object instead of ConverterStrategy.Value. This was an important corner case complicating our existing implementations of polymorphism and reference handling.
  • Simplify the reference handling implementation: moves all reference resolution logic to a single location inside JsonConverter<T>.TryWrite and avoids rerunning the calculations in polymorphic converters or continuations.
  • Tidy up the JsonConverter<T>.TryWrite/TryRead methods removing redundant checks and moving logic specific to ObjectConverter to the converter itself.

@ghost
Copy link

ghost commented Feb 22, 2022

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

Issue Details

Makes a few infrastructural changes in anticipation of #63747. More specifically

  • Makes ObjectConverter (the polymorphic converter for object) use ConverterStrategy.Object instead of ConverterStrategy.Value. This was an important corner case complicating our existing implementations of polymorphism and reference handling.
  • Simplify the reference handling implementation: moves all reference resolution logic to a single location inside JsonConverter<T>.TryWrite and avoids rerunning the calculations in polymorphism converters or continuations.
  • Tidy up the JsonConverter<T>.TryWrite/TryRead methods removing redundant checks and moving logic specific to ObjectConverter to the converter itself.
Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Feb 22, 2022
@steveharter
Copy link
Contributor

Please report the benchmark changes. Several hot path methods were changed.

@eiriktsarpalis
Copy link
Member Author

@steveharter I ran the full suite and benchmark numbers were identical, no substantial speed-ups or regressions here.

@eiriktsarpalis eiriktsarpalis merged commit 097d9ea into dotnet:main Mar 1, 2022
@eiriktsarpalis eiriktsarpalis deleted the refactor-objectconverter branch March 1, 2022 13:08
@kunalspathak
Copy link
Contributor

kunalspathak commented Mar 8, 2022

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.

4 participants