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

Proposal: Add new GetValue method to System.Reflection #19484

Closed
Esidar opened this issue Nov 26, 2016 · 35 comments
Closed

Proposal: Add new GetValue method to System.Reflection #19484

Esidar opened this issue Nov 26, 2016 · 35 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Reflection enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@Esidar
Copy link

Esidar commented Nov 26, 2016

This new method would accept an "object" to which a Value could be copied instead of being unnecessarily boxed. Example of use would look like this:

static object boxedFloat = null;
....
boxedFloat = fieldInfo.GetValue( myObject, boxedFloat );

Boxing of the value would occur only once. This would eliminate unnecessarily memory allocations in situations when GetValue is called many times. When method is used on a class types, copying would not occur and stored object would be returned instead.

One of the uses of GetValue method is User Interface based on MVVM (Model View Viewmodel) where values used to control an UI are pulled directly from the data. In realtime applications (like games) this boxing makes hundreds if not thousands of unnecessary memory allocations every second.

Another use of GetValue is serialization, and having ability to serialize hundreds of values without the need of boxing/unboxing, would greatly improve memory usage and performace.

There was also an idea https://github.com/dotnet/coreclr/issues/8277 that this new GetValue method would take a form of generic method, but I think this would limit the use as one of the advantages of working with System.Object is this abstraction that makes some parts of the code simpler.

So, ideally this new method should remove the unnecessary boxing while keeping convenience of System.Object. Additionally please bear in mind this should work with both FieldInfo and PropertyInfo, as well as give option to get values from nested Value as in following example:

struct Vector3
{
   float x,y,z;
}
struct Transform
{
    Vector3 position { get; set }
}

transform = positionProperty.GetValue( transformObject, transform );
boxedX = xField.GetValue( transform, boxedX );
DisplayValue( boxedX );

(note intermixing fields with property)

@svick
Copy link
Contributor

svick commented Nov 26, 2016

Interesting, I didn't realize you can reassign values inside boxes.

If this is going to work with properties, I think it should be made completely general and work with any method that returns a value.

And a couple questions:

  1. What should the behavior be if the passed-in boxed value is of the wrong type? The same as null, i.e. allocate a new box?
  2. Wouldn't it be simpler if the input and output were specified at the same time using a ref parameter, i.e.: fieldInfo.GetValue(myObject, ref boxedFloat);? No need to specify boxedFloat twice.

@jkotas
Copy link
Member

jkotas commented Nov 27, 2016

FieldInfo.GetValue/SetValue are part of reflection invoke APIs.

All current reflection invoke APIs have a general problem that they expected everything boxed. It is not just a performance problem mentioned here, but also a functional deficiency problem.

Among other things, it means that it is not possible to write high fidelity IL interpreter using reflection. Let's say that you would like to interpret static void m1(ref int x) { m2(ref x); }. There is no way to do it using reflection today because of there is no reflection invoke method that can pass x along as byref.

If we were to add a new reflection invoke APIs, we should aim to solve both of these problems.

CoreRT has internal Internal.Runtime.CallInterceptor.MakeDynamicCall that is basically a box-free version of the MemberInfo.Invoke. It would need to be cleaned up and argument validation added for it to become good public API.

@MichalStrehovsky
Copy link
Member

There is no way to do it using reflection today because of there is no reflection invoke method that can pass x along as byref

MethodBase.Invoke will actually modify the elements of the parameters array if one of the parameters is a byref (a ref or out parameter). A better example would be a method that returns a byref (this will be exposed as a new language feature in C# soon). There's no way to reflection-invoke those.

Another thing to consider would be finding a replacement for FieldInfo.GetValueDirect and FieldInfo.SetValueDirect. These APIs let you access fields on a value type without boxing the struct that owns the field. Since TypedReference never was a first class citizen (and won't be part of .NET Core) a replacement API would be a nice addition.

@bartdesmet
Copy link
Contributor

MethodBase.Invoke will actually modify the elements of the parameters array if one of the parameters is a byref (a ref or out parameter).

While that's true, it's rarely the case that the storage locations of the objects or values being passed by reference are truly in the object[] being passed. Typically one copies the value into the object[] and performs a write-back after the Invoke method returns. This causes issues such as https://github.com/dotnet/corefx/issues/4411 and https://github.com/dotnet/corefx/issues/13458.

The expression interpreter runs into a number of other issues as well. For example, we can't pass Type.Missing in a verbatim manner, see https://github.com/dotnet/corefx/issues/4112. Also, the object[] allocation and the copy made by the reflection API is expensive, see https://github.com/dotnet/corefx/issues/4125. An issue to discuss addressing just these two limitations is opened at https://github.com/dotnet/coreclr/issues/7253.

Having boxes passed to reflection APIs has also led to behavioral changes in the expression interpreter compared to the compiler, such as https://github.com/dotnet/corefx/issues/13007 (though one can argue the interpreter is doing the right thing in this case, merely by accident).

@Esidar
Copy link
Author

Esidar commented Nov 28, 2016

Looking through available features (to avoid major changes in FX/CLR) I've stumbled upon arglist. C# have this undocumented keyword that is supported by CLR which allow for passing varying number of arguments and the use looks like this:

void Foo( __arglist )
{
   var it = new ArgIterator( __arglist );
}
void Test()
{
    float myFloat = 1;
    Foo( __arglist( 10, ref myFloat ) );
}

As you can see, you can even pass float as a ref.

From what I can see by disassembling a code (x86-64), values are passed clean, without any boxing.

New ReflectionAPI could utilize this to have both: box free GetValue as in fieldInfo.GetValue( obj, ref myFloat ) and a pretty neat, box free way of calling methods fooMethodInfo.Invoke( 10, ref myFloat )

Of course there would be a need to make a syntax nicer than a keyword __arglist, also, I'm not sure how this API would work with other languages. There is also a limit on how __arglist can be used. Currently ArgIterator, to read a value/argument, returns TypedReference and the only way to retrieve value is via TypedReference.ToObject() which does boxing. So that limits the use in managed environment. It is not a problem in native environment because GetValue and Invoke (both are implemented in native code), can read values and pointers directly from ArgIterator without boxing.

@Esidar
Copy link
Author

Esidar commented Nov 28, 2016

Having boxes passed to reflection APIs

Problem shown in #19094 cannot be fixed by removing boxing. Boxing ensures that value types are valid during delegate invocation. In given example, variable o is valid only when delegate is created. Actual call to function f can occur much later when variable o is no longer available, thus copy must be made.

@MichalStrehovsky
Copy link
Member

TypedReference, ArgIterator and RuntimeArgumentHandle are a no-go. None of those types are included in .NET Core or .NET Standard - they only exist in the legacy runtimes and AFAIK there are no plans to bring them back. I don't think we really need them. MakeDynamicCall mentioned above can get by without them, for example.

@jkotas
Copy link
Member

jkotas commented Nov 28, 2016

MakeDynamicCall mentioned above can get by without them

The LocalVariableSet type used by MakeDynamicCall is very close to TypedReference. It is basically array of TypedReferences. It may be interesting to bring TypedReference back and making it useful, instead of creating a new same thing under a different name.

@Esidar
Copy link
Author

Esidar commented Nov 29, 2016

Is there a specific reason why TypedReference is being avoided? I guess there can be stability issue (reference to a local that is no longer available), but is there anything else?

Another solution would be use of new Tuples feature. methodInfo.Invoke could accept tuple struct instead of object[] array. CoreCR/VM could then use the same tuple if invoked method has any ref/out arguments

var args = ( name: "Mike", value: 10.0f );
methodInfo.Invoke( obj, args );
Console.Write( args.value );

I don't know the implementation details of the tuples, but afaik tuple is a struct that still needs to be boxed but we're boxing one struct instead of N-arguments and N-out-results. This wouldn't solve most of the problems, but GetValue could also use tuple to return a value (again, tuple is boxed only once and can be reused), could solve problems with properly resolving Missing arguments or unboxing values in Closures and so on.

@jkotas
Copy link
Member

jkotas commented Nov 29, 2016

Is there a specific reason why TypedReference is being avoided?

TypedReference has not been very useful: C# does not have a first class support for it, it requires non-trivial custom logic in the codegenerator and runtime, not all .NET runtimes support it. It is missing some methods that would make it useful and easy to work with, e.g. it should have a method to cast the value to strong typed value, like ref T As<T>() - this method is equivalent to the undocumented __refvalue C# keyword.

The non-boxing reflection method to get the value can then be: TypedReference GetValueReference(). To get the field value without boxing, one would write: fieldInfo.GetValueReference(myObject).As<float>().

new Tuples feature

I do not think Tuples are a good fit for this.

@jkotas
Copy link
Member

jkotas commented Nov 29, 2016

I have opened dotnet/corefx#14088 to bring back TypedReference that should be the first on this.

cc @davidwrighton

@Thaina
Copy link

Thaina commented Jan 13, 2017

I think it should support return ref on FieldInfo directly, without TypedReference

I think TypedReference add more complexity to implementation. It make reference that would need to be fixed can be passed around. While return ref are compatible with dotnet behaviour

GameObject obj;

FieldInfo fi = typeof(obj).GetField("Transform");
ref Matrix4x4 transform = fi.GetRefValue<Matrix4x4>(ref obj);
// ref transform live in this stack
transform.M43 = 1; // obj.Transform.M43 = 1

Also, GetMethod should be able to use by cast it into generic delegate that can be used with ref object

Matrix4x4 matrix;

MethodInfo mi = typeof(matrix).GetMethod("RotateY");
RefAction<Matrix4x4,float> rotate = mi.GetRefAction<Matrix4x4,float>();
rotate(ref matrix,90); // Internally call matrix.RotateY(90);

////

namespace System
{
    public delegate void RefAction<T>(ref T obj);
    public delegate void RefAction<T,P1>(ref T obj,P1 param1);
    public delegate void RefAction<T,P1...>(ref T obj,P1 param1...);

    public delegate R RefFunc<T,R>(ref T obj);
    public delegate R RefFunc<T,P1>(ref T obj,P1 param1);
    public delegate R RefFunc<T,P1...>(ref T obj,P1 param1...);

    public delegate ref R RefFuncRef<T,R>(ref T obj);
    public delegate ref R RefFuncRef<T,P1>(ref T obj,P1 param1);
    public delegate ref R RefFuncRef<T,P1...>(ref T obj,P1 param1...);
}

class System.Reflection.MethodInfo
{
    public RefAction<T> GetRefAction<T,P>(P param) where T : class
    {
        // return void action(ref T t,param) that wrap void t.action(param);
    }
}

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

The implementation that returns TypedReference is more efficient and more powerful.

It is more efficient because of it does not suffer from generic code bloat.

It is more powerful because of you can easily add a strong typed generic convenience wrapper extension method around it that just returns ref (this wrapper can be part of the API proposal too); but the other direction is not possible.

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

Also, GetMethod should be able to use by cast it into generic delegate that can be used with ref object

There are existing methods that do that like Delegate.CreateDelegate(Type type, Object firstArgument, MethodInfo method). If you wish to have more convenient wrappers around them, it would be best to split it into a separate API proposal issue.

@Thaina
Copy link

Thaina commented Jan 13, 2017

@jkotas You are right that it more powerful, which, at the same time, overkill. It not safe to pass TypedReference around. And TypedReference is hard to have it implement everywhere while return ref is more promising to be standard

as you say

it requires non-trivial custom logic in the codegenerator and runtime, not all .NET runtimes support it.

So I want to trade off here. We can live with generic in the most case

@Thaina
Copy link

Thaina commented Jan 13, 2017

Also with Delegate.CreateDelegate the returned delegate does not work with mutable struct (without boxing?). So I propose something to make it work

The point here is to avoid bringing back TypedReference. Which I think generic is satisfied what we need and it make code always be strong type

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

TypedReference is coming back for other reasons. It is used in non-trivial percentage of .NET libraries. See dotnet/standard#20.

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

It not safe to pass TypedReference around.

There is nothing inherently unsafe about it. The safety rules for TypedReference are exactly same as the rules for ref and other byref-like types.

@Thaina
Copy link

Thaina commented Jan 13, 2017

@jkotas any ref T is syntactically error in async and lambda expression/function callback. Was the TypedReference get the same behaviour in compile time?

I mean

void DoSomething(ref int i)
{
    i++; // normally fine
    Action plusplus = () => i++; // error, cannot access ref
}

is it the same for this?

void DoSomething(TypedReference tr)
{
    Action action = () => {
        // would it error if we use tr here?
    };
}

@Thaina
Copy link

Thaina commented Jan 13, 2017

It is used in non-trivial percentage of .NET libraries

I quite suspect that most of those could be replace with ref return. To support old code is ok. but I don't think we need to use in the future anymore

I said "not bringing it back" is misused, I was actually mean not encourage to using it more in the future or make any new library rely on it. It fine to have it support like volatile. But should not use it to implement the new reflection

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

Was the TypedReference get the same behaviour in compile time?

TypedReference was in .NET since forever. byref-like type safety rules has been always enforced for it by the C# compiler. You example works as expected today - there is no extra work required to make it work.

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

But should not use it to implement the new reflection

It is not possible to build things like good high fidelity IL interpreter on top of reflection without having the TypedReference-based reflection. I think it is important that any reflection APIs give you full power. We do not need yet another partial solution.

@Thaina
Copy link

Thaina commented Jan 13, 2017

@jkotas Well, that's why I propose generic + return ref based API. And I think it could done all of things TypedReference based api could do, with strong typed

Could you please guide me what TypedReference could do but generic + return ref could not?

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

If you are building things like IL interpreter, the actual types are not known. You need a type that can store any value that can appear as method argument or on IL stack. TypedReference is such type.

@Thaina
Copy link

Thaina commented Jan 13, 2017

OK, I quite get some image for the reason. So now I would like to agree with you

Thank you very much for explanation

2 last things I concern is
1 - Is TypedReference box things? Could we get ref of struct without boxing/unboxing with it?
2 - Will it cls compatible in the future?

@jkotas
Copy link
Member

jkotas commented Jan 13, 2017

  1. TypedReference does not box things.
  2. The CLS compliance guidelines were set 15+ years ago and have not change since then. I doubt anybody will bother with updating them. It does not really help anything.

@Thaina
Copy link

Thaina commented Jan 13, 2017

Thank you very much

@Thaina
Copy link

Thaina commented Jan 13, 2017

So actually what I should want is ref T field = fieldInfo.GetValueDirect<T>(typedReference) ?

@jnm2
Copy link
Contributor

jnm2 commented Jan 14, 2017

The ability to assign to a field reference obtained via reflection with equal performance to setting a field directly would be amazing. I could stop emitting IL for this.

@IS4Code
Copy link

IS4Code commented Apr 14, 2017

Provided TypedReference gets all the features C# already has with the undocumented keywords, it would be the most feasible way to pass around a simple reference to a field, without having to care about its type. Typed references can already be passed around as safely as normal typed references. Copying the value to an already-boxed instance sounds interesting, but I'd like to point out that ECMA-335 specifies the return type of the unbox instruction (akin to this behaviour) to be a controlled-mutability managed pointer. Although the CLR, as far as I know, makes no attempts in verifying the mutability of the pointer, while the similar behaviour of the Invoke method is one thing, the specification implies that at least it shouldn't be encouraged.

@jkotas
Copy link
Member

jkotas commented Oct 3, 2017

Related proposal: dotnet/corefx#24390

@joshfree joshfree assigned ghost Mar 8, 2018
@ghost ghost assigned steveharter and unassigned ghost Sep 12, 2018
@ghost
Copy link

ghost commented Sep 12, 2018

This is a long range cross cutting feature (runtime/C#/framework changes.)

@steveharter steveharter removed their assignment Jun 4, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@steveharter steveharter added the tenet-performance Performance related issue label Feb 25, 2020
@GrabYourPitchforks
Copy link
Member

Faster FieldInfo accessors are being tracked by #23716.

@GrabYourPitchforks GrabYourPitchforks removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2020
@Ziflin
Copy link

Ziflin commented May 20, 2020

@GrabYourPitchforks Where exactly did this proposal end up? I'm still looking for a non-boxing solution similar to this one, but didn't see where #23716 was either.

@steveharter
Copy link
Member

Assuming #45152 supersedes this.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
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 enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests