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

Reduce generic usage of JsonConverter<T> for object and collection converters #36784

Closed
steveharter opened this issue May 20, 2020 · 1 comment
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented May 20, 2020

Currently, the JsonConverter<T> base class is closed with T being the Type declared by a property or the root type passed to the serializer. This helps avoid boxing for value converters, such as for an int property and is an important feature for performance. However, the same pattern is used for object- and collection-converters which typically use reference types, not value types.

By changing this to use System.Object for T instead of the actual Type reduces assembly size and reduces memory consumption by avoiding JITing (todo: quantify) and calls to Type.MakeGenericType(). In addition, limiting usage of generics helps with AOT scenarios and may be required in certain cases (pending testing of Xamarin on iOS and Android devices). See the 5.0 serializer goals for more detail

For example,

  • For an int Type, T is System.Int32 (value converter).
  • For a MyPOCO Type, T is MyPOCO (object converter).
  • For a List<int> Type, T is List<int> (collection converter).

Proposal:

  • For object and collection converters, T will be System.Object instead of the actual Type. Note there are alternatives that need to be considered such as not even using generics for these types of converters, although that makes it more difficult for the serializer design to compose multiple converters (e.g. List<Dictionary<string, MyPoco>> which uses 4 converters).
  • The base class of JsonConverter<T> which is JsonConverter could add Read- and Write- method that directly operate on System.Object instead of T. This enables loosely-typed scenario which are not easy or performant today (re-entry into the serializer or usage of Type.MakeGenericType is necessary).

Currently in 5.0, a converter is returned by the public JsonSerializerOptions.GetConverter(Type type) method. In 3.0\3.1 this method returned null for objects and collections since the implementation for those did not share the same infrastructure as value converters. In 5.0 the GetConverter method returns a non-null converter such as JsonConverter<List<int>>. It is important for 5.0 that we don't expose implementation details that we may later want to change. We may want to change the semantics in 5.0 to instead to return null for object and collection converters, or to return JsonConverter<object>.

@steveharter steveharter added api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json labels May 20, 2020
@steveharter steveharter added this to the 5.0 milestone May 20, 2020
@steveharter steveharter self-assigned this May 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 20, 2020
@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label May 20, 2020
@steveharter
Copy link
Member Author

steveharter commented Aug 12, 2020

As covered in #35028, the perf implications are mitigated by having an options code-gen approach for 6.0 that avoids MakeGenericType and because object and collections converters are normally based on reference types which do not have the same perf impacts as value types.

On this previous statement "It is important for 5.0 that we don't expose implementation details that we may later want to change" we no longer expect that post-5.0 will we want to change semantics of the existing converter model. The existing converter model was intended for value types, not objects and collections (because it is hard to do so correctly for non-trivial cases) but 5.0 now allows obtaining and forwarding to the built-in object and collection converters using the same programming model. In 3.1 it was not possible to get a built-in converter to an object or collection.

In 6.0 we expected to have a new programming model for object and collection converters including exposing metadata.

A prototype based on the original idea is at https://github.com/steveharter/runtime/tree/ObjConverter.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

2 participants