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

Improve performance of ActivatorUtilities.CreateInstance #693

Closed
pakrym opened this issue Mar 12, 2018 · 6 comments · Fixed by #1796
Closed

Improve performance of ActivatorUtilities.CreateInstance #693

pakrym opened this issue Mar 12, 2018 · 6 comments · Fixed by #1796
Labels
help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@pakrym
Copy link

pakrym commented Mar 12, 2018

Currently, it's 30x slower than invoking factory delegate.
While it could never reach the performance of invoking the factory delegate there is a lot of possible improvements (especially around allocations) in ActivatorUtilities.CreateInstance

@pakrym
Copy link
Author

pakrym commented Jun 6, 2019

@anurse I'm pretty sure #1796 didn't fix all of the allocations.

@Yves57
Copy link

Yves57 commented Jun 7, 2019

@pakrym Here is a snapshot of the allocated memory when allocating 10000 objects with ActivatorUtilities.CreateInstance() (with the official "2.2.0" DependencyInjection package).
I have crossed out the allocations optimized with the last PR.
It remains:

  • ConstructorInfo[] -> I don't see any possible optimization. But I may be wrong.
  • ParameterInfo[] -> I don't see any possible optimization. But I may be wrong.
  • The object[] allocated in the ConstructorMatcher constructor. We may imagine reuse the array during the matching loop. But this optimization would be valid only if there is more than two constructors, and if the constructors have same number of parameters (ConstructorInfo.Invoke() requests a parameter array of the exact length). So it seems very restrictive.

But there is maybe a more generic but trickier optimization here. The method CheckArguments is called RuntimeConstructorInfo.Invoke() (see the orange rectangle). A copy of the parameters is done because they may be changed by CheckValue(). But CheckValue() seems to returns the object as-is in the normal case. So we could imagine a lazy allocation of the copyOfParameters array only if the parameter is changed by CheckValue().
The advantage is that any object construction (and maybe other reflection calls) could benefit of this optimization. But the optimization seems too big to be correct. I suppose that something is wrong in my analysis :-).

image

@pakrym
Copy link
Author

pakrym commented Jun 7, 2019

@Yves57 thank you for detailed analysis, I think you've done all the appropriate optimizations. We can keep the issue closed.

@Yves57
Copy link

Yves57 commented Jun 7, 2019

@pakrym Do you think it makes sense to open an issue in CoreClr about the "lazy allocation" (last point of my previous comment)?

@pakrym
Copy link
Author

pakrym commented Jun 7, 2019

@Yves57, yeah, do it.

@analogrelay
Copy link

TIL that it's the person who merges the PR that gets credit for closing linked issues ;P.

Sounds good, thanks for filing that issue @Yves57 !

@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants