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

Split TryRead implementation into TryPopulate and TryCreateObject #79659

Closed
wants to merge 2 commits into from

Conversation

krwq
Copy link
Member

@krwq krwq commented Dec 14, 2022

Contributes to: #78556
Contibutes to: #77018

Cleanup converter internal implementation - splits TryRead into:

  • TryCreateObject - creates intermediate object (it can be same as final)
  • TryPopulate - populates intermediate object
  • TryConvert - converts intermediate object into final object (or just casts for most of the cases)

TODO:

  • finish TryPopulate on collections
  • finish TryPopulate on dictionaries
  • possibly TryPopulate can be generalized
  • Consider implementing Parametrized ctors in terms of: intermediate object = arguments state; final object - call ctor and go over props
  • check if TryCreateObject can be strongly typed
  • consider removing unused args
  • TODOs in code (i.e. generalize exceptions)
  • consider fixing System.Text.Json contract customization ignoring custom CreateObject delegates in certain collection types #73382 as part of this - it will also simplify implementation
    • Also see JsonConverter.SupportsCreateObjectDelegate
    • consider sealing TryCreateObject
    • ensure IReadOnlyDictionary<TKey, TValue> can be deserialized but not populated
    • finish writing tests replacing JsonTypeInfo_CreateObject_UnsupportedCollection_ThrowsInvalidOperationException
  • consider fixing Support using any dictionary as JsonExtensionData #31645 as part of this, it requires us to track extension data instance in TryPopulate and calling TryConvert and setter later than we currently do - it might require a bit plumbing so might be better as follow up work
  • run benchmarks

@ghost
Copy link

ghost commented Dec 14, 2022

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

Contributes to: #78556

Cleanup converter internal implementation - splits TryRead into TryPopulate and TryCreateOrGetObject and unify some code.

Author: krwq
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@krwq
Copy link
Member Author

krwq commented Dec 15, 2022

[Triage] Offline review feedback:
I've cut content and manage TODO list in the description of the PR now

@krwq krwq marked this pull request as draft December 15, 2022 17:30
@ghost ghost closed this Jan 16, 2023
@ghost
Copy link

ghost commented Jan 16, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@krwq krwq reopened this Jan 16, 2023
@krwq krwq force-pushed the json-converters-populate branch 2 times, most recently from a7e69af to 3e4786c Compare January 16, 2023 12:07
{
internal override bool CanHaveMetadata => false;

protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref List<TElement> collection, in TElement value, JsonTypeInfo collectionTypeInfo)
Copy link
Member

Choose a reason for hiding this comment

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

In any case, we don't stand to win much by making it strongly typed -- I don't think any of the intermediate collections we're using are structs so we aren't boxing anything redundant right now.

Copy link
Member Author

@krwq krwq Jan 30, 2023

Choose a reason for hiding this comment

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

actually ReadOnlyArray is a struct and uses intermediate type - that's why I added that in the first place

Copy link
Member

Choose a reason for hiding this comment

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

ReadOnlyArray? Was this added in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, there are existing tests though which implement read only collections as structs and those are failing unless this is ref previously we changed state.Current.ReturnValue here and reassigning if this was value type

}

protected override void ConvertCollection(ref ReadStack state, JsonSerializerOptions options)
private protected override bool TryConvert(ref Utf8JsonReader reader, JsonTypeInfo jsonTypeInfo, scoped ref ReadStack state, List<TElement> obj, out TElement[] value)
Copy link
Member

Choose a reason for hiding this comment

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

What does the bool result in TryConvert signify?

Copy link
Member Author

Choose a reason for hiding this comment

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

honestly I added it because I think we can implement parametrized constructors in terms of these 3 and in that case it will need the bool result. In general it means same as in other places: "I did not finish reading, continuation will need to happen"

Copy link
Member

Choose a reason for hiding this comment

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

Isn't convert something meant to run after serialization has completed to materialize the final instance from the intermediate one? If so, there's not async reading necessary that might necessitate producing partial results.

Copy link
Member Author

Choose a reason for hiding this comment

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

that is certainly possible it's not needed and I added it initially because I wasn't initially sure if we do that or not. I will reiterate on that

where TCollection : ConcurrentQueue<TElement>
{
protected override void Add(in TElement value, ref ReadStack state)
private protected override void Add(ref TCollection collection, in TElement value, JsonTypeInfo collectionTypeInfo)
Copy link
Member

Choose a reason for hiding this comment

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

Why does it need to be private protected? The type itself is internal so it shouldn't matter much.

Copy link
Member Author

Choose a reason for hiding this comment

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

no preference

using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
/// <summary>
/// Default base class implementation of <cref>JsonDictionaryConverter{TCollection}</cref> .
/// </summary>
internal abstract class DictionaryDefaultConverter<TDictionary, TKey, TValue>
: JsonDictionaryConverter<TDictionary, TKey, TValue>
internal abstract class DictionaryDefaultConverter<TDictionary, TKey, TValue, IntermediateType>
Copy link
Member

Choose a reason for hiding this comment

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

See my earlier feedback. I think this could regress application sizes without much tangible impact.

/// <summary>
/// Base class for object, enumerable and dictionary converters
/// </summary>
internal abstract class JsonAdvancedConverter<T, IntermediateType> : JsonAdvancedConverter<T>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would avoid terms like "advanced" in type names since they don't necessarily convey what this layer of abstraction is meant to achieve.

Copy link
Member Author

Choose a reason for hiding this comment

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

didn't have a better name, I'm open for feedback


if (IsReadOnly(obj))
{
// TODO: not possible for objects, might be a good idea to still generalize exception
Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 30, 2023

Choose a reason for hiding this comment

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

Object deserialization is more common than collection deserialization, wouldn't want to spend CPU cycles checking for something that is always true.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's really a single check per object, I was thinking that this type could eventually be public and making this generalized (if we do delegates that will also be true)

}

protected virtual void ConvertCollection(ref ReadStack state, JsonSerializerOptions options) { }
private protected abstract void Add(ref IntermediateType collection, in TElement value, JsonTypeInfo collectionTypeInfo);
Copy link
Member

Choose a reason for hiding this comment

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

I think once we get to the point of bringing collection support to contract customization, I would imagine we would be exposing a Action<object, object>? Add { get; set; } to JsonTypeInfo. Perhaps we could abstract away the concept of intermediate accumulators by having all the collection lambdas accept an additional object? state parameter.

Copy link
Member

Choose a reason for hiding this comment

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

By pushing all these concerns to JsonTypeInfo, it might be that we could replace all enumerable converter types with just one that calls into delegates. I expect this might offer even more size reduction benefits.

@ghost ghost closed this Mar 2, 2023
@ghost
Copy link

ghost commented Mar 2, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
This pull request was closed.
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.

2 participants