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

Add cheap/fast path for MethodBase.Invoke for use in expression interpreter #6686

Closed
bartdesmet opened this issue Sep 19, 2016 · 5 comments
Closed
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection tenet-performance Performance related issue
Milestone

Comments

@bartdesmet
Copy link
Contributor

The expression interpreter in System.Linq.Expressions uses Invoke(object, object[]) on MethodBase when interpreting MethodCallExpression, NewExpression, etc. One place is here, in the context of a commit to avoid array copies on our side of the call.

The current approach suffers from a few limitations due to the lack of control over the reflection behavior. Culprits are in MethodBase.CheckArguments:

  1. Type.Missing can't be passed as-is, these get rewritten to the default value for the parameter (see https://github.com/dotnet/corefx/issues/4112)
  2. a copy of the incoming array is made, even though we won't mutate it from the interpreter (see https://github.com/dotnet/corefx/issues/4125)

Case 1 is a functional regression from the expression tree compiler. Case 2 is a heavy source of allocations (think of expression trees having loops) and we don't have to worry about any in-place mutations of the array (i.e. we can transfer ownership altogether).

We'd like a fast path for the expression interpreter to take. Questions:

  • Any disagreements on our conclusions on the need for such an API?
  • Should this be a public API?
    • If yes, what should be the API shape?
    • If not, is [InternalsVisibleTo] to System.Linq.Expressions acceptable?

Some possible APIs (public or not) are listed below.

Option 1 - InvokeFast variant

An API that suppresses both behaviors and claims to be "fast"; this is really all we'd need for the expression interpreter:

public object InvokeFast(object obj, object[] parameters);

Option 2 - BindingFlags additions

An API addition to BindingFlags and no changes to Invoke at all (but we'd need access to the verbose overload to use that one in the interpreter):

public enum BindingFlags
{
    ...
    NoTypeMissingResolution = 0x02000000,
    NoArgumentsArrayCopy = 0x04000000,
}

I can noodle around with this if we reach some agreement on the need and ideally the shape.

@gkhanna79
Copy link
Member

CC @atsushikan @danmosemsft

@ghost
Copy link

ghost commented Mar 6, 2017

Rather than an ad-hoc fix just for Type.Missing, why not a BindingFlags.NoConversion bit to disable all of CheckArguments() weird convenience casting altogether?

In another words, all passed in parameters must be the exact type already. Hence, removes Reflection's internal need to mutate (and hence copy) the argument array. No more overhead for all those checks that CheckArguments() must do - the new bit converts Invoke() into a simpler, faster and more predictable api.

Note that any budgeting for this has to account for corert which has a completely different implementation of Reflection.

@NinoFloris
Copy link
Contributor

I'd like this very much, especially for high performance scenario's that have no other way than to depend on reflection (like interpreters)

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@steveharter steveharter added the tenet-performance Performance related issue label Feb 25, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@GrabYourPitchforks GrabYourPitchforks self-assigned this Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks modified the milestones: Future, 5.0 Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks removed their assignment Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks removed this from the 5.0 milestone Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks self-assigned this Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks added this to the 5.0 milestone Apr 16, 2020
@steveharter
Copy link
Member

For case 1 (pass Type.Missing directly -- no default value) a new API is necessary to opt-in to that functionality.

For case 2 (avoid a copy of the incoming array) assume #12832 will address that.

cc @GrabYourPitchforks

@steveharter steveharter added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner enhancement Product code improvement that does NOT require public API changes/additions labels Apr 16, 2020
@GrabYourPitchforks GrabYourPitchforks modified the milestones: 5.0.0, Future Jun 24, 2020
@steveharter
Copy link
Member

For case 1 (Type.Missing) the original sample in https://github.com/dotnet/corefx/issues/4112 is now working; not sure what was changed in expressions to allow that.

Expression<Action> f = () => Console.WriteLine(Type.Missing);

// Prints System.Reflection.Missing
f.Compile()();

// Prints System.Reflection.Missing (NOW WORKS THE SAME AS ABOVE)
f.Compile(true)();

For case 2 (perf) we are actively working on perf. An IL Emit path was added in 7.0 and new APIs that avoid the object[] allocation are expected in 8.0.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection tenet-performance Performance related issue
Projects
No open projects
Development

No branches or pull requests

7 participants