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 allocation in object construction by reflection #12832

Closed
Yves57 opened this issue Jun 7, 2019 · 6 comments · Fixed by #50814
Closed

Avoid allocation in object construction by reflection #12832

Yves57 opened this issue Jun 7, 2019 · 6 comments · Fixed by #50814
Assignees
Labels
Milestone

Comments

@Yves57
Copy link
Contributor

Yves57 commented Jun 7, 2019

During an analysis about memory allocations in an ASP.NET Core service (github/dotnet/extensions#693), I have found that there is maybe an optimization in (at least) object construction by reflection.
When constructing an object by reflection, the method MethodBase.CheckArguments() seems to duplicate the parameters (copyOfParameters) because they may be changed by CheckValue(). But this method seems to just return the value as-is in the normal cases. So isn't it possible to do a "lazy allocation" only if necessary?

/cc @pakrym

@jkotas
Copy link
Member

jkotas commented Jun 8, 2019

because they may be changed by CheckValue()

It is not just because of they may be changed by CheckValue(). It is also protect against cases where the value is change by the caller (e.g. on a different thread) that would violate type safety invariant in the code that follows.

Having said this, there are ways how to optimize this. It just is not as simple as allocating the array lazily.

@Yves57
Copy link
Contributor Author

Yves57 commented Jun 8, 2019

Got it!
But the external array modification is probably a very uncommon case. The ASP.NET Core ActivatorUtilities.CreateInstance() is typically a case where this protection seems not useful. Maybe create a new flag BindingFlags.ThreadSafeParameters set by the caller?

@jkotas
Copy link
Member

jkotas commented Jun 8, 2019

I do not think a new flag is necessary. It should be possible to optimize this for everybody without compromising type safety and security characteristics provided by the current implementation.

@Yves57
Copy link
Contributor Author

Yves57 commented Jun 8, 2019

I hope this optimization will be developped! I want to see what kind of trick can avoid this allocation! 😃

@benaadams
Copy link
Member

Could change RuntimeMethodHandle::InvokeMethod to take a Span rather than a regular array, then copy to a stack array instead; though not sure there are any examples of passing a Span the the vm

@steveharter
Copy link
Member

Moving to future

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@steveharter steveharter modified the milestones: Future, 5.0 Feb 11, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Mar 6, 2020
@GrabYourPitchforks GrabYourPitchforks self-assigned this Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks modified the milestones: 5.0.0, Future Jun 29, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 6, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants