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

Initial prototype of Activator.CreateFactory #45458

Conversation

GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Dec 2, 2020

** DRAFT ** DRAFT ** DRAFT **

Contributes to #36194. Doesn't fully resolve that issue since there's still a second set APIs which aren't present in this PR.

This is the follow-up to #32520, which takes advantage of that refactoring to introduce new Activator.CreateFactory(Type) and Activator.CreateFactory<T> APIs. Think of a factory as a cached version of CreateInstance, where we've already done all the initial work and can keep invoking our factory over and over every time we need a new instance.

As a reminder, the purpose of this new API is two-fold. First, it allows callers to remove their own usage of ref-emit. Second, it tries to avoid spinning up the JIT as much as possible. Activator.CreateFactory(Type) should be JIT-free in nearly all cases, including for value types. Activator.CreateFactory<T> may spin up the JIT for value types T.

I've also hooked this factory up through System.Text.Json to show how it can replace their existing usage of DynamicMethod in some scenarios.

This is a draft PR and isn't fully ready for review. There's still some code cleanup that needs to take place, but it should give a good indication of how things should look when they're fully finished.

To-do:

  • Make mono runtime compile and tests pass
  • Figure out whether the Delegate changes were appropriate or if they should be backed out
  • Fix layering in Activator.cs and Activator.CoreCLR.cs
  • Fix suppressions and dynamic dependency attributes introduced here
  • Add API docs

There are some other uses of Activator.CreateInstance (see here for one example), but I didn't change those call sites as part of this PR since I don't think they're on hot paths.

/cc @layomia and @jozkee as area owners of System.Text.Json so that they can see how this would look in practice. If you're wondering about the double-delegate pattern, see #45408 and #36194 (comment) for context.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@@ -27,6 +31,9 @@ internal sealed class ReflectionEmitMemberAccessor : MemberAccessor
return null;
}

#if NET6_0 // should really be NET6_0_OR_GREATER
return new JsonClassInfo.ConstructorDelegate(Activator.CreateFactory(type));
Copy link
Member

Choose a reason for hiding this comment

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

There are 20 instance of ConstructorDelegate in 7 files. I think we should just replace it with Func<object>.

Assert.NotNull(factory);
StructWithPublicDefaultConstructor instance = factory();
Assert.True(instance.ConstructorInvoked);
Assert.True(IsAddressOnLocalStack(instance.AddressPassedToConstructor)); // using Func<T>, value type ctor is called using ref to stack local, no boxing
Copy link
Member

@jkotas jkotas Dec 2, 2020

Choose a reason for hiding this comment

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

This is implementation detail. I can imagine some implementations may choose to allocate the box to keep things simple.


// Then create a delegate to factory.CreateInstance, closed over the "this" parameter.

return (Func<object?>)factory.CreateDelegate((RuntimeType)typeof(Func<object?>));
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this just call new Func<object?>(factory.CreateInstance) ? This looks slow and complicated for no good reason.

{
if (typeof(T).IsValueType)
{
IActivationFactory factory = (IActivationFactory)RuntimeTypeHandle.CreateInstanceForAnotherGenericParameter((RuntimeType)typeof(ActivationFactory<>), (RuntimeType)typeof(T));
Copy link
Member

@jkotas jkotas Dec 2, 2020

Choose a reason for hiding this comment

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

CreateInstanceForAnotherGenericParameter is AOT unfriendly API that depends on JIT. Can this be just something something like this?

class StructFactory<T>
{
    T CreateInstance()
    {
         T value;
         _pfnConstructor(&value);
         return value;
    }
}
return new Func<T>(new StructFactory<T>().CreateInstance);

?

@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@ghost ghost closed this Jan 25, 2021
@ghost
Copy link

ghost commented Jan 25, 2021

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@steveharter
Copy link
Member

@GrabYourPitchforks based on prior discussion I assume the perf numbers are the same as ref.emit (once warmed up). Is that correct?

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Jan 28, 2021

@steveharter Yes, it should meet or beat ref emit in terms of wall clock time + memory utilization.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2021
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.

5 participants