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

New reflection invoke APIs [draft] #286

Closed
wants to merge 5 commits into from

Conversation

steveharter
Copy link
Member

Adding to get some feedback, especially around TypedReference support by Roslyn.


For 8.0, there are two primary goals:
1) Support byref-like types both for invoking and passing as arguments. An unsafe approach may need to be used for V8. This unblocks scenarios.
2) Support "fast invoke" so that dynamic methods no longer have to be used for performance. Today both `System.Text.Json` and [dependency injection](https://learn.microsoft.com/dotnet/core/extensions/dependency-injection) use IL emit for performance. As a litmus test, these areas will be changed to use the new APIs proposed here.
Copy link
Member

@jkotas jkotas Dec 30, 2022

Choose a reason for hiding this comment

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

Replacing dynamic methods in Microsoft.Extensions.DependencyInjection is more important than replacing dynamic methods in System.Text.Json.

Our .NET 8 plan is to keep Microsoft.Extensions.DependencyInjection dependency on reflection for trimming and native AOT compatibility. It means that we really need a fast reflection invoke that does not depend on dynamic code generation and that supports everything that Microsoft.Extensions.DependencyInjection needs.

The plan for System.Text.Json is to use source generators for trimming and native AOT compatibility. It means that we do not need to get rid of the dynamic code generation in (non-source generated) System.Text.Json to achieve our .NET 8 goals.

cc @eerhardt @davidfowl

Copy link
Member

Choose a reason for hiding this comment

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

Right. Today, it's hard to remove reflection from dependency injection because the resolution of instances relies on the knowledge that only exists at runtime (the list of services registered in the container). We could do partial codegen for each module but that could end up being slower (it's similar to partial crossgen).

This is the implementation that gets used at runtime when resolving instances in AOT mode using DI:

https://github.com/dotnet/runtime/blob/5b8ebeabb32f7f4118d0cc8b8db28705b62469ee/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs

Copy link
Member Author

Choose a reason for hiding this comment

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

It means that we really need a fast reflection invoke that does not depend on dynamic code generation and that supports everything that Microsoft.Extensions.DependencyInjection needs.

To clarify: for "fast invoke", the new APIs still require "dynamic code generation" through IL Emit but of course use the "slow invoke" interpreted fallback approach when not available. So DI would use the new reflection APIs instead instead of expressions. An unstated goal is to support dotnet/runtime#66153 which doesn't want to use expressions in Blazor client due to having to bring along the large System.Linq.Expressions.dll.

Copy link
Member

Choose a reason for hiding this comment

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

for "fast invoke", the new APIs still require "dynamic code generation" through IL Emit but of course use the "slow invoke" interpreted fallback approach when not available.

For native AOT config, the invocation should be done via a stub generated at build time. This stub is similar to the stub generated via IL Emit and it has similar performance. It is how reflection invoke works in native AOT today. The slow interpreted fallback does not exist in native AOT.

Copy link
Member

Choose a reason for hiding this comment

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

it seems like we could delete a large amount of code that uses expression trees in some cases if we could have the CoreCLR version of these APIs use reflection emit under the covers. Then we could just use the built in methods and have them be "best in class" on each of the available platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll assume for now "InvokeDirect()` is as fast as possible with perhaps only argument number validated and then add back other validation to see what the perf impact is.

The API is designed so that the set\get parameters is common so at the end when it is time to call "invoke" either the fast, non-validating InvokeDirect(), or the slower Invoke() can be easily substituted by an #ifdef DEBUG for example.

Copy link
Member

Choose a reason for hiding this comment

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

I'll assume for now "InvokeDirect()` is as fast as possible with perhaps only argument number validated

The set\get parameters have to store the additional type information so that it can maybe validated later. Is that correct? InvokeDirect maybe as fast as possible, but the rest is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The set\get parameters have to store the additional type information so that it can maybe validated later. Is that correct?

Not when using the object set overloads. The invoker maintains a tuple (byreference, object, type) for each arg and depending on the set method used and whether the target method has byref parameters. For the simple object-only case used by DI, for example, we don't need the byreference or the type for Core at least.

For perf, Core will have both the byref- and object-based emit. NativeAOT may elect to force everything to byrefs to have just one stub.

Copy link
Member Author

@steveharter steveharter Apr 19, 2023

Choose a reason for hiding this comment

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

Here's an overall status on DI using these new APIs. Ideally, we can remove the usage of LINQ, but that depends on the perf:

  • Used DI factory logic ActivatorUtilities.CreateFactory(Type, Type[]) which is object-based with no byref args
  • The perf measures calling the factory method. The factory was earlier obtained from CreateFactory and that is not included in the benchmark.
  • That factory method was changed to use the new MethodInvoker.
  • The factory method has 3 args, 2 of which are "matched" and 1 is "unmatched" so there's some logic involved in calling the IServiceProvider for the unmatched case to obtain the missing arg.

The breakdown of time spent:

  • ~12% for calling the invoke delegate which allocates and calls ctor of the target type passing the 3 args.
  • ~5% for MethodInvoker.Invoke() overhead (very simple logic currently).
  • ~80% in DI code for setting the args and preparing to call MethodInvoker.Invoke(). This can vary depending on whether the fixed vs. variable MethodInvoker APIs, but not much. This also includes the DI logic for default values and mapping.

That 80% dwarfs the actual invoke of 12% and the IL is basically the fastest it can be except for a couple tiny optimizations like using Ldard_1 instead of Ldarg, 1. So we need to focus on the 80% if we want this faster.

With today's reflection (which uses emit since 7.0), the test ran in ~66ns compared to ~55ns with MethodInvoker. This ~17% gain also includes some other misc optimizations. In summary, this is not getting us much except note that there is no more alloc for the object[] to hold the args.

Compare this to LINQ with compiled expressions which is ~2.5x faster (~22ns). This in on my local Windows machine.

Based on this I have a new prototype:

  • Added additional InvokeDirect() APIs for fixed-length cases (say up to 8):
    • MethodInvoker.InvokeDirect(object, object, ...) or MethodInvoker.GetDirectInvokeDelegate(object, object, ...)
    • The new method does not require any special stack allocs like the MethodInvoker instance or setting of values. It will only take object args and be focused around existing MethodBase.Invoke cases which can only take object.
    • For NativeAOT, this can simply delegate to the MethodInvoker instance-based approach so no new stubs need to be created.
    • No validation; the delegate is the raw pointer to the dynamic method.
  • Perform a manual "AOT" generation of the DI factory lambda code depending how the matching of args occurs. The permutations of these new methods are based on whether default values are required and what arg mapping logic is required. For the simpler cases, it also avoids capturing unnecessary state by the factory's lambda and avoids various logic to check for default values etc.

This new prototype looks promising but will never be as fast as LINQ expressions since that can hard-code the default values and mapping information in the expression. For the happy and common paths, I think we can get it to within 1.2-1.4x instead of 2.5x. We need to decide if this approach and perf is acceptable and whether we would still need LINQ within DI to get that extra 20+% or so.

accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved

static unsafe void CallMe(__arglist)
{
// However, when enumerating __arglist here, the runtime throws when accessing a byref-like
Copy link
Member

Choose a reason for hiding this comment

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

Is the runtime tracking lifetime properly in this case?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW the compiler does do lifetime tracking here.

accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved

static unsafe void CallMe(__arglist)
{
// However, when enumerating __arglist here, the runtime throws when accessing a byref-like
Copy link
Member

Choose a reason for hiding this comment

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

FWIW the compiler does do lifetime tracking here.

@RikkiGibson RikkiGibson self-requested a review January 3, 2023 23:24
public ref struct TypedReference
{
// Equivalent of __makeref (except for a byref-like type since they can't be a generic parameter)
+ public static TypedReference Make<T>(ref T? value);
Copy link
Member

Choose a reason for hiding this comment

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

This is strictly less powerful than __makeref though because it can't support ref struct. Until we get support for ref struct generic parameters my recommendation would be to use __makeref everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

my recommendation would be to use __makeref everywhere.

Would we then advocate and evangelize it usage? I thought it was considered bad form to use any __ keywords nowadays.

Copy link
Member

Choose a reason for hiding this comment

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

One of the explicit goals of this work is to enable ref struct to participate here. Rather than say use TypedReference.Make<T> for most types but __makeref for ref struct I prefer the simpler, context-free, advice of use __makeref.

I agree that __ is explicitly unsupported now but part of this work is basically us defining these operators, adding docs and telling people when we support them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on separate discussion, my feeling here is: let's use __makeref etc. for the time being, but eventually make sure we introduce the ability to express TypedReference.Make<T> in the language in subsequent releases, and steer people towards that once it's available.

### Existing usages
`TypedReference` is used in `FieldInfo` via [`SetValueDirect(TypeReference obj, object? value)`](https://learn.microsoft.com/dotnet/api/system.reflection.fieldinfo.setvaluedirect) and [`object? GetValueDirect(TypeReference obj)`](https://learn.microsoft.com/dotnet/api/system.reflection.fieldinfo.getvaluedirect). The approach taken by `FieldInfo` will be expanded upon in the design here. The existing approach supports the ability to get\set a field on a target value type without boxing. Boxing the target through [`FieldInfo.SetValue()`](https://learn.microsoft.com/dotnet/api/system.reflection.fieldinfo.setvalue) is useless since the changes made to the boxed value type would not be reflected back to the original value type instance.

`TypedReference` is also used by the undocumented `__arglist` along with `System.ArgIterator` although `__arglist` is Windows-only. The approach taken by `__arglist` will not be leveraged or expanded upon in this design. It would, however, allow a pseudo-strongly-typed approach like
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a top level item to track expanding __arglist to non-windows platforms? Most of other non-trivial work items have a top level item and my guess is this is non-trivial work.

Copy link
Member

Choose a reason for hiding this comment

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

The idea of native varargs is tracked at dotnet/runtime#48796.

Copy link
Member

Choose a reason for hiding this comment

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

We should link to that here then.

2) Support "fast invoke" so that dynamic methods no longer have to be used for performance. Today both `STJ (System.Text.Json)` and [DI (dependency injection)](https://learn.microsoft.com/dotnet/core/extensions/dependency-injection) use IL Emit for performance although DI uses emit through expressions while STJ uses IL Emit directly.

## STJ and DI
As a litmus test, STJ and DI will be changed to use the new APIs proposed here. This is more important to DI since, unlike STJ which has a source generator that can avoid reflection, DI must continue to use reflection. See also https://github.com/dotnet/runtime/issues/66153 which should be addressed by having a fast constructor invoke that can be used by DI.
Copy link
Member

Choose a reason for hiding this comment

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

As a litmus test

I assume this is also gating? i.e. if for some reason we're unable to successfully change both of these in .NET 8 to use the new APIs, we won't ship the new APIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the thinking - if it's not fast enough for internal use, it may not be fast enough for others. However, I have a prototype for STJ using the proposed APIs and has a 5% regression in deserialization (note that without any Emit, deserialization is ~2x slower, so getting it to just 5% is pretty significant). That was the reason I also added the PropertyInfo.CreateGetter\SetterDelegate to match how STJ uses delegates today. There may also be some gains in going down an unsafe approach, but I haven't measured the perf of that yet.

For DI, I hope to use the new APIs directly.

accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
```diff
namespace System.Reflection
{
public abstract class PropertyInfo
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 359 to 360
+ public virtual Func<object, TValue> CreateGetterDelegate<TValue>();
+ public virtual Action<object, TValue> CreateSetterDelegate<TValue>();
Copy link
Member

@davidfowl davidfowl Jan 14, 2023

Choose a reason for hiding this comment

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

https://github.com/dotnet/aspnetcore/blob/4535ea1263e9a24ca8d37b7266797fe1563b8b12/src/Shared/PropertyHelper/PropertyHelper.cs#L73-L92

Typically, code is unable to reference the TValue. Should there also be an overload that is object -> object that does the reflection emit fallback? Or are we going to do something similar to what MethodInfo.Invoke does in .NET 7?

Copy link
Member Author

Choose a reason for hiding this comment

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

The proposed TypedReference APIs will be able to take a boxed object and use the internal reference, so object is supported that way.

However, if there are a variable number of parameters that need to be passed, you'll still need to pass them down somehow (e.g. object[] or an unsafe approach (e.g. TypedReference*[] \ void*) until an Invoke can be done.

@davidfowl
Copy link
Member

davidfowl commented Jan 14, 2023

The examples in this proposal feel unrealistic for the cases where reflection need to be used. Typically, there's an assembly boundary between the code doing the reflection and the code being reflected on.

App.dll (depends on Library.dll)

app.RegisterController<HomeController>();

public class AlbumController : ControllerBase
{
    [HttpGet("/albums")]
    public Task<IEnumerable<Album>> GetAlbums(Db db) => db.Albums.ToListAsync();
    
    [HttpGet("/albums/{id}")]
    public IActionResult GetAlbum(int id, Db db) => db.Albums.Find(id) switch
    {
        Album a =>  Ok(a),
        _ => NotFound();
    };
    
    [HttpPost("/albums")]
    public async Task<Person> GetPersonSlow(Album album, Db db)
    {
        db.Albums.Add(album);
        return Created(album);
    }
}

Library.dll

static void RegisterController<TController>(this WebApplication app)
{
    foreach (MethodInfo method in typeof(TController).GetMethods())
    {
        var routeAttribute = method.GetCustomAttribute<HttpMethodAttribute>();

        if (routeAttribute is null) continue;

        app.MapMethods(routeAttribute.Template!, routeAttribute.HttpMethods, async context =>
        {
            TController instance = Activator.CreateInstance<TController>();
            object[] args = PrepareArguments(method, context);
            object? result = method.Invoke(instance, args);
            if (result is IResult iResult)
            {
                await iResult.ExecuteAsync(context);
            }
            if (result is Task<IResult> taskOfResult)
            {
                await (await taskOfResult).ExecuteAsync(context);
            }
            // Etc
        });
    }
}

The interesting part here is the PrepareArguments method. This method needs to create the arguments for passing into the method. Let's say we're preparing the arguments for GetAlbum, we need to produce an int and a Db (reference type owned by the user that we cannot reference directly). Here's some sample code for how the binding for this method could look:

static object?[] PrepareArguments(MethodInfo methodInfo, HttpContext context)
{
    var parameters = methodInfo.GetParameters();
    var args = new object?[parameters.Length];
    for (int i = 0; i < args.Length; i++)
    {
        args[i] = Resolve(context, parameters[i]);
    }
    return args;
}

static object? Resolve(HttpContext context, ParameterInfo parameterInfo)
{
    // Assume query string
    if (parameterInfo.ParameterType == typeof(int))
    {
        return int.TryParse(context.Request.Query[parameterInfo.Name!], out var val) ?
            val : null;
    }

    // Assume service
    return context.RequestServices.GetRequiredService(parameterInfo.ParameterType);
}

In the above, I don't know how to avoid boxing the integer even with these new APIs. This is the missing piece from the examples.

Using the above example, here are 3 versions of what "dynamic code generation" could look like for the following thunk:

IActionResult GetAlbum(int id, Db db);
  1. For clarity, we start with a potentially source generated piece of code:
Task SourceGeneratedThunk(HttpContext context)
{
    int id = int.Parse(context.Request.Query["id"]);
    Db db = context.RequestServices.GetRequiredService<Db>();
    GetAlbum(id, db);
}

This code can only be generated in user code, it cannot be generated as part of framework logic since it directly references user code.

  1. Expression trees (implementation left out for brevity and sanity)

  2. ILEmit (I don't have all night 😅)

  3. Generics (what this model is pushing)

Task PrecompiledThunk<T0, T1>(HttpContext context, string name0) where T0 : IParseable<T0>
{
    T0 arg0 = T0.Parse(context.Request.Query[name0]);
    T1 arg1 = context.RequestServices.GetRequiredService<T1>();
    GetAlbum(arg0, arg1);
}
  1. Code referencing the types directly, but I don't know what that looks like.

I can't figure out how I would use TypedReference in these situations other than if I know the type at compile time, what am I missing?

I can't figure out how any of the code referenced (JSON and DI for e.g.) will take advantage of these APIs without calls to something like MakeGenericType or a source generator. All of today's reflection-based APIs (Reflection Emit, APIs, Expression) give you a way to reference, create, invoke types that weren't compiled against, and that's a strict requirement.

@MichalStrehovsky
Copy link
Member

If I understand this correctly, the problem David writes about in #286 (comment) is with getting a storage location where we could prepare the parameters in if we're in dynamic code that doesn't have any T's representing method parameters.

Does this assume we'll be able to dynamically create storage locations? Just throwing a thing here because I know very little about the GC - could we have something that allows to stackalloc a byte buffer that we mark as conservatively reported to the GC that could then be used to partition into TypedReferences for each parameter? I.e. the size of the buffer would be the sum of all arguments (plus padding).

@AaronRobinsonMSFT
Copy link
Member

could we have something that allows to stackalloc a byte buffer that we mark as conservatively reported to the GC that could then be used to partition into TypedReferences for each parameter?

Yes. This "feature" does exist internally. It was added for other Reflection scenarios. How to express this and what sort API would be provided is the more complicated question.

@davidfowl
Copy link
Member

If I understand this correctly, the problem David writes about in #286 (comment) is with getting a storage location where we could prepare the parameters in if we're in dynamic code that doesn't have any T's representing method parameters.

This is exactly the point I was trying to show with the example. I don't know how we would practically use this without a source generator or a call to MakeGenericType.

@jkotas
Copy link
Member

jkotas commented Jan 18, 2023

could we have something that allows to stackalloc a byte buffer that we mark as conservatively reported to the GC that could then be used to partition into TypedReferences for each parameter?

Yes. This "feature" does exist internally.

Similar feature that allows you to mark memory region as reported to GC exists internally. The reporting is still precise. CoreCLR is not compatible with conservative reporting. It is ok to use conservative reporting as internal simplifying assumption in implementation of some runtimes. I do not think we would want to make public APIs dependent on conservative reporting. API like this to allocate the space should be designed for precise GC reporting.

@steveharter
Copy link
Member Author

If I understand this correctly, the problem David writes about in #286 (comment) is with getting a storage location where we could prepare the parameters in if we're in dynamic code that doesn't have any T's representing method parameters.

This is exactly the point I was trying to show with the example. I don't know how we would practically use this without a source generator or a call to MakeGenericType.

and

In the above, I don't know how to avoid boxing the integer even with these new APIs. This is the missing piece from the examples.

I'll add a section for this. Basically, this design currently doesn't add any new ways to define a storage location, so you're left with what we have today: a parameter, field, variable or an array element.

However, in order to invoke efficiently without boxing, we do want variable-size collection support and there are several options for that, but it is also likely to assume a byref (or pointer with GC tracking) meaning it would ultimately be one of the existing storage locations as is the case with TypedReference. The LocalVariableSet approach in CoreRt should also be considered a reference implementation where the caller must allocate and also be GC-protected.

I can't figure out how any of the code referenced (JSON and DI for e.g.) will take advantage of these APIs without calls to something like MakeGenericType or a source generator.

FYI STJ, when not using the source generator, does use MakeGenericType to hold the logic that needs to deal with the field\property values to avoid boxing in order to call the strongly-typed converters and do other validation\housekeeping.

The .NET 7 release had a 3-4x perf improvement for the existing `Invoke()` APIs by using IL Emit when available and falling back to standard reflection when IL Emit is not available. Although this improvement is significant, it still doesn't replace the need to use the IL-Emit based alternatives (dynamic methods and expression trees) for highly-performance-sensitive scenarios including the `System.Text.Json` serializer. New APIs are required that don't have the overhead of the existing `Invoke()` APIs.

For .NET 8, there are two primary goals:
1) Support byref-like types both for invoking and passing as arguments; this unblocks various scenarios. An unsafe approach may used for .NET 8 if support for `TypedReference` isn't addressed by Roslyn (covered later).
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we know by know that we are not going to get safe TypedReferences from Roslyn in .NET 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - I'll clarify that and move to the "future" section.

Copy link
Member

Choose a reason for hiding this comment

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

Think there is a simple design where we have safe APIs for constructing TypedReference instances and getting the values back from them. The only type unsafe would come into the mix is when you attempted to extract the value of a ref struct by ref. That would put some burden on the implementation here to be careful with the values but consumers shouldn't need to.

The conversation on whether or not this will work for the reflection APIs keeps lagging though so it's hard to tell if we should be investing in it.

public void SetReturn<T>(ref T value)

// Invoke direct (limited validation and defaulting)
public unsafe void InvokeDirect(MethodBase method)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The unsafe API with non-validation should have Unsafe in the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

In most other areas we don't add "Unsafe" since it is a bit redundant since it's in the signature and requires unsafe context to call. I assume then "Unsafe" makes it clearer that this case is more dangerous than the others.

This is currently optional and being discussed. If `TypedReference` ends up supporting references to byref-like types like `Span<T>` then it will be much more useful otherwise just the existing `ref <T>` API can be used instead. The advantage of `TypedReference` is that it does not require generics so it can be made to work with `Span<T>` easier than adding a feature that would allowing generic parameters to be a byref-like type.

To avoid the use of C#-only "undocumented" keywords, wrappers for `__makeref`, `__reftype`, `__refvalue` which also enable other languages.
```diff
Copy link
Member

Choose a reason for hiding this comment

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

random: why diff here and not csharp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because TypedReference is an existing class with existing members.

accepted/2022/ReflectionInvoke.md Show resolved Hide resolved

// Target
public object? GetTarget()
public ref T GetTarget<T>()
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider a GetTarget method that can return the TypedReference? The current design has a lack of symmetry with the SetTarget methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should be added; currently that is a CS1599 (can't return a TypedReference) and I didn't call it out.

However, I'm pushing these TypedReference cases to the "future" section for now.

accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
```cs
namespace System.Reflection
{
public ref partial struct ArgumentValuesFixed
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using this struct vs. a Span<object> which is backed by an [InlineArray(8)] struct that you slice to the appropriate length?

Copy link
Member Author

@steveharter steveharter Apr 25, 2023

Choose a reason for hiding this comment

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

That struct actually has 3 different types of storage (object, ByReference, and Type) and they need to be grouped something like this since memory arithmetic is used for each:

public unsafe ref struct ArgumentValuesFixed
  {
      internal object? _obj0;
      internal object? _obj1-to-length;

      internal Type? _type0;
      internal Type? _type1-to-length;

      internal ref byte _ref0;
      internal ref byte _ref1-to-length;
}

Also, I moved this type to the "future" section and will add it back, if necessary, based on perf.

The .NET 7 release had a 3-4x perf improvement for the existing `Invoke()` APIs by using IL Emit when available and falling back to standard reflection when IL Emit is not available. Although this improvement is significant, it still doesn't replace the need to use the IL-Emit based alternatives (dynamic methods and expression trees) for highly-performance-sensitive scenarios including the `System.Text.Json` serializer. New APIs are required that don't have the overhead of the existing `Invoke()` APIs.

For .NET 8, there are two primary goals:
1) Support byref-like types both for invoking and passing as arguments; this unblocks various scenarios. An unsafe approach may used for .NET 8 if support for `TypedReference` isn't addressed by Roslyn (covered later).
Copy link
Member

Choose a reason for hiding this comment

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

Think there is a simple design where we have safe APIs for constructing TypedReference instances and getting the values back from them. The only type unsafe would come into the mix is when you attempted to extract the value of a ref struct by ref. That would put some burden on the implementation here to be careful with the values but consumers shouldn't need to.

The conversation on whether or not this will work for the reflection APIs keeps lagging though so it's hard to tell if we should be investing in it.

public static void ChangeIt(TypedReference tr)
{
// This compiles today:
ref Span<int> s = ref __refvalue(tr, Span<int>);
Copy link
Member

Choose a reason for hiding this comment

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

I think the easiest path forward for TypedReference safely holding a Span<int> is for this to turn into an unsafe operation. This is where the danger really sets in, unless we significantly complicate the existing lifetime rules, hence this is where unsafe should go. Essentially this is when the rules break down if we allow capturing Span<int> safely.

Note: specifically I mean when ref __refvalue(expr, T) and T is a ref struct.


// Possible for performance in System.Text.Json:
+ public virtual Func<object, TValue> CreateGetterDelegate<TValue>();
+ public virtual Action<object, TValue> CreateSetterDelegate<TValue>();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps CreateGetValueDelegate and CreateSetValueDelegate, to match GetValue, GetValueDirect, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Make sense.

public MethodInvoker(ref ArgumentValuesFixed values)

// Dispose needs to be called with variable-length case
public void Dispose()
Copy link
Member

Choose a reason for hiding this comment

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

Why is Dispose() needed for the variable-length case but not the fixed-length case?

Copy link
Member Author

Choose a reason for hiding this comment

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

The variable-length case hooks up in a low level to the runtime to track these references to support the GC and needs to be unregistered once finished.

accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
accepted/2022/ReflectionInvoke.md Outdated Show resolved Hide resolved
public MethodInvoker(ref ArgumentValuesFixed values)

// Dispose needs to be called with variable-length case
public void Dispose()
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if having Dispose which is only required in some cases won't trigger false positives in disposable analyzers.

Copy link
Member Author

@steveharter steveharter Apr 25, 2023

Choose a reason for hiding this comment

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

Good question. An alternative is to have two different MethodInvoker types, but that has its own issues.

Also, I moved the fixed-length class to the "future" section so all that remains for not having to call Dispose is the zero-arg constructor for MethodInvoker for when the target method has no arguments. However, perhaps that should also be removed meaning there are only two ways to call the new APIs:

  1. The variable length mechanism with Dispose
  2. The newly added static, fixed-length invoke methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.