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

Avoid Type.MakeGenericType when (de)serializing objects #35028

Closed
steveharter opened this issue Apr 15, 2020 · 6 comments
Closed

Avoid Type.MakeGenericType when (de)serializing objects #35028

steveharter opened this issue Apr 15, 2020 · 6 comments
Assignees
Labels
Milestone

Comments

@steveharter
Copy link
Member

As part of researching approaches to #1568 it was found that avoiding Type.MakeGenericType for our object converter (non-collections and non-valuetypes) will help "reach" scenarios (AOT platforms that may not support it) as well as improve CPU performance and reduce memory consumption.

This means that the T in JsonConverter<T> will be of type System.Object; some changes are necessary in the object factory and JsonConverter base classes for this to work.

@steveharter steveharter added this to the 5.0 milestone Apr 15, 2020
@steveharter steveharter self-assigned this Apr 15, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 15, 2020
@MichalStrehovsky
Copy link
Member

non-valuetypes

MakeGenericType over valuetypes is the thing that contributes to most of JITtting though - MakeGenericType over a reference type usually ends up using shared code and JIT doesn't have to be spun up.

I wrote a quick summary of generics in .NET in a twitter thread recently: https://threadreaderapp.com/thread/1244602445461950470.html

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Apr 17, 2020
@steveharter
Copy link
Member Author

MakeGenericType over valuetypes is the thing that contributes to most of JITtting though -

The two main benefits:

  • Helps for "reach" scenarios where Type.MakeGenericType is not supported. This is one of several issues that will be created to address "reach".
  • Helps first-time startup perf since no reflection Emit needs to occur for these types.

@jmaine
Copy link
Contributor

jmaine commented May 28, 2020

@MichalStrehovsky, Could we remove/reduce the need to JITing in the case of value types on generics by having the Jit transform the valuetype to a pointer to a valuetype?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky, Could we remove/reduce the need to JITing in the case of value types on generics by having the Jit transform the valuetype to a pointer to a valuetype?

Yes, that's kind of how Universal Shared Code looks like. It can be used for any T, but it's a lot less efficient than code specialized for a given T. RyuJIT/CoreCLR currently doesn't have support for that. (See sample of how Universal Shared Code looks like in the above mentioned Twitter thread).

@jmaine
Copy link
Contributor

jmaine commented May 30, 2020

@MichalStrehovsky, I believe that is what I was referring to when I wrote this issue: #37143.

@steveharter
Copy link
Member Author

Closing this for the following reasons:

  • Type.MakeGenericType is supported on platforms including Xamarin; the serialization usage was made drastically simpler in 5.0 and no reported issues have been reported.
  • We have a code-gen initiative Type.MakeGenericType will not be used for AOT scenarios. This will help scenarios that want faster startup time and less memory usage.
  • Using System.Object as the T will make writing custom converters a bit less intuitive since a cast will be necessary.

A prototype of the original idea is located at https://github.com/steveharter/runtime/tree/UseObjectAsT.

@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
Projects
None yet
Development

No branches or pull requests

5 participants