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

[API Proposal]: Generic Activator API to create a type from a possibly non-public parameterless constructor #85541

Closed
jkoritzinsky opened this issue Apr 28, 2023 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime

Comments

@jkoritzinsky
Copy link
Member

Background and motivation

To construct a object of type T from a parameterless constructor, we have a few API options today. If we know that the constructor is public, we can use Activator.CreateInstance<T>() and Activator.CreateInstance(Type). If the constructor may be non-public, we only have a non-generic Activator.CreateInstance(Type, bool) method.

For our SafeHandleMarshaller type that we use in the interop source generators, we need to support non-public constructors for backward-compatibility reasons. As a result, we need to use Activator.CreateInstance(Type, bool) to optionally support a non-public default constructor.

CoreCLR accelerates both Activator.CreateInstance<T>() and Activator.CreateInstance(Type, bool) to be almost as fast as writing new T() with a specific T, so adding a new generic Activator.CreateInstance method to support constructing an object from a non-public constructor isn't necessary.

NativeAOT has a very powerful optimization path for Activator.CreateInstance<T>() to avoid pulling in the reflection stack. It does not have this optimization for Activator.CreateInstance(Type, bool), and it cannot have this optimization in a non-generic context. As a result, the new SafeHandleMarshaller type roots the reflection stack, increasing the size of any apps that use it by quite a bit. Since LibraryImports with SafeHandles are used in the code for basically every application type (Web, Console, UI), we end up always rooting the reflection stack. If we add this API, we can implement an intrinsic implementation on NativeAOT that does not root the reflection stack and avoids the size regression.

cc: @dotnet/ilc-contrib

API Proposal

namespace System;

public static class Activator
{
+    public void CreateInstance<T>(bool nonPublic);
}

API Usage

T instance = Activator.CreateInstance<T>(nonPublic: true);

Alternative Designs

We can just have this method as internal in System.Private.CoreLib to avoid expanding the public API surface of Activator, as the SafeHandleMarshaller type will live in CoreLib.

We could also introduce a custom intrinsic in NativeAOT's CoreLib that the SafeHandleMarshaller could use on NativeAOT.

Risks

No response

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime labels Apr 28, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

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

Issue Details

Background and motivation

To construct a object of type T from a parameterless constructor, we have a few API options today. If we know that the constructor is public, we can use Activator.CreateInstance<T>() and Activator.CreateInstance(Type). If the constructor may be non-public, we only have a non-generic Activator.CreateInstance(Type, bool) method.

For our SafeHandleMarshaller type that we use in the interop source generators, we need to support non-public constructors for backward-compatibility reasons. As a result, we need to use Activator.CreateInstance(Type, bool) to optionally support a non-public default constructor.

CoreCLR accelerates both Activator.CreateInstance<T>() and Activator.CreateInstance(Type, bool) to be almost as fast as writing new T() with a specific T, so adding a new generic Activator.CreateInstance method to support constructing an object from a non-public constructor isn't necessary.

NativeAOT has a very powerful optimization path for Activator.CreateInstance<T>() to avoid pulling in the reflection stack. It does not have this optimization for Activator.CreateInstance(Type, bool), and it cannot have this optimization in a non-generic context. As a result, the new SafeHandleMarshaller type roots the reflection stack, increasing the size of any apps that use it by quite a bit. Since LibraryImports with SafeHandles are used in the code for basically every application type (Web, Console, UI), we end up always rooting the reflection stack. If we add this API, we can implement an intrinsic implementation on NativeAOT that does not root the reflection stack and avoids the size regression.

cc: @dotnet/ilc-contrib

API Proposal

namespace System;

public static class Activator
{
+    public void CreateInstance<T>(bool nonPublic);
}

API Usage

T instance = Activator.CreateInstance<T>(nonPublic: true);

Alternative Designs

We can just have this method as internal in System.Private.CoreLib to avoid expanding the public API surface of Activator, as the SafeHandleMarshaller type will live in CoreLib.

We could also introduce a custom intrinsic in NativeAOT's CoreLib that the SafeHandleMarshaller could use on NativeAOT.

Risks

No response

Author: jkoritzinsky
Assignees: -
Labels:

api-suggestion, area-System.Runtime

Milestone: -

@jkoritzinsky jkoritzinsky added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 28, 2023
@jkoritzinsky
Copy link
Member Author

Marked blocking as this is blocking merging in #85419

@jkotas
Copy link
Member

jkotas commented Apr 28, 2023

For our SafeHandleMarshaller type that we use in the interop source generators, we need to support non-public constructors for backward-compatibility reasons.

Why can't we tell people to fix their safe handles first if they want to use source generators?

@jkoritzinsky
Copy link
Member Author

We can choose to take that breaking change if we want to, but we considered that a large breaking change and didn't want to take it.

@jkoritzinsky
Copy link
Member Author

If taking the breaking change is preferable to providing a NativeAOT-intrinsic API here, we can do the breaking change instead.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2023

CoreCLR accelerates both Activator.CreateInstance() and Activator.CreateInstance(Type, bool) to be almost as fast as writing new T() with a specific T, so adding a new generic Activator.CreateInstance method to support constructing an object from a non-public constructor isn't necessary.

We can implement the exact same optimization for native AOT as well. We do not need a new API just to compensate for a missing optimization on native AOT.

I think depending on private reflection in modern marshallers is sub-optimal. I think we should make SafeHandleMarshaller to have new constraint and require SafeHandles that want to be used by it to have a public constructor.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Apr 28, 2023

CoreCLR accelerates both Activator.CreateInstance() and Activator.CreateInstance(Type, bool) to be almost as fast as writing new T() with a specific T, so adding a new generic Activator.CreateInstance method to support constructing an object from a non-public constructor isn't necessary.

We can implement the exact same optimization for native AOT as well. We do not need a new API just to compensate for a missing optimization on native AOT.

If we can implement it with the existing API surface, I'd happily support that.

I think depending on private reflection in modern marshallers is sub-optimal. I think we should make SafeHandleMarshaller to have new constraint and require SafeHandles that want to be used by it to have a public constructor.

We can't make it have the new() constraint as that would block too many scenarios for managed->unmanaged marshalling where we don't need the support (we have many places where we use abstract SafeHandle or SafeBuffer types in our own P/Invokes as the handle type is a private nested class). I'm fine with using Activator.CreateInstance<T>() directly though and not supporting non-public SafeHandle constructors if need be (we can detect this in the generator and handle it smoothly).

I've just built the runtime with the smaller breaking change (requiring a public constructor for ref, out, and return scenarios) and we don't have any breaks in the product. I'll validate that this change removes the reflection stack. If it does (which it should), we can go the breaking change route.

@jkoritzinsky
Copy link
Member Author

I have confirmed that only allowing public constructors removes the reflection stack. I'll update the SafeHandle PR to include the breaking change.

@jkoritzinsky jkoritzinsky closed this as not planned Won't fix, can't repro, duplicate, stale Apr 28, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 28, 2023
@teo-tsirpanis teo-tsirpanis removed the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests

3 participants