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

Developers using reflection invoke should be able to use ref struct #45152

Open
4 of 7 tasks
steveharter opened this issue Nov 24, 2020 · 65 comments
Open
4 of 7 tasks

Developers using reflection invoke should be able to use ref struct #45152

steveharter opened this issue Nov 24, 2020 · 65 comments
Assignees
Labels
area-System.Reflection Cost:XL Work that requires one engineer more than 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries tenet-performance Performance related issue User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Nov 24, 2020

Support ref struct and "fast invoke" by adding new reflection APIs. The new APIs will be faster than the current object[] boxing approach by leveraging the existing System.TypedReference<T>.

TypedReference is a special type and is super-fast because since it is a ref struct with its own opcodes. By extending it with this feature, it provides alloc-free, stack-based “boxing” with support for all argument types (reference types, value types, pointers and ref structs [pending]) along with all modifiers (byval, in, out, ref, ref return). Currently reflection does not support passing or invoking a ref struct since it can’t be boxed to object; the new APIs are to support ref struct with new language features currently being investigated.

Example syntax (actual TBD):

MyClass obj=; 
MethodInfo methodInfo =;

int param1 = 42; // Pass an integer
Span<int> param2 = new Span<int>(new int[]{1, 2, 3}); // Pass a span (not possible with reflection today)

// Adding support for Span<TypedReference> requires the language and runtime asks below.
Span<TypedReference> parameters = stackalloc TypedReference[2];

// The 'byval', 'in', 'out' and 'ref' modifiers in the callee method's parameters all have the same caller syntax
parameters[0] = TypedReference.FromRef(ref param1);
parameters[1] = TypedReference.FromRef(ref param2);
methodInfo.GetInvoker().Invoke(returnValue: default, target: TypedReference.FromRef(ref obj), parameters);  
// The first call to this methodInfo will be slower; subsequent calls used cached IL when possible

// Shorter syntax using __makeref (C# specific)
parameters[0] = __makeref(param1);
parameters[1] = __makeref(param2);
methodInfo.Invoke(default, __makeref(obj), parameters));

Dependencies

The Roslyn and runtime dependencies below are required for the programming model above. These are listed in the order in which they need to be implemented.

Motivation

Reflection is ~20x slower than a Delegate call for a typical method. Many users including our own libraries use IL Emit instead which is non-trivial and error-prone. The expected gains are ~10x faster with no allocs; verified with a prototype. Internally, IL Emit is used but with a proposed slow-path fallback for AOT (non-emit) cases. The existing reflection invoke APIs may also layer on this.

In Scope 

APIs to invoke methods using TypedReference including passing a TypedReference collection. TypedReference must be treated as a normal ref struct (today it has nuances and special cases).

Support ref struct (passing and invoking).

Performance on par with existing ref emit scenarios:

  • Property get\set
  • Field get\set
  • Parameterized constructors and methods

To scope this feature, the minimum functionality that results in a win by allowing System.Text.Json to remove its dependency to System.Reflection.Emit for inbox scenarios.

Out of Scope 

This issue is an incremental improvement of reflection by adding new Invoke APIs and leveraging the existing TypedReference while requiring some runtime\Roslyn changes. Longer-term we should consider a more holistic runtime and Roslin support for reflection including JIT intrinsics and\or new "dynamic invoke" opcodes for performance along with perhaps C# auto-stack-boxing to\from a ref TypedReference.

Implementation

A design doc is forthcoming.

The implementation will likely cache the generated method on the corresponding MethodBase and MemberInfo objects.

100% backwards compat with the existing object[]-based Invoke APIs is not necessary but will be designed with laying in mind (e.g. parameter validation, special types like ReflectionPointer, the Binder pattern, CultureInfo for culture-aware methods) so that in theory the existing object[]-based Invoke APIs could layer on this new work.

This issue supersedes other reflection performance issues that overlap:

@davidfowl
Copy link
Member

This isn't just for JSON, what we've we doing should work for any framework that uses reflection today I assume. I'm hoping to see a deep analysis here of the scenarios that Would use these APIs

cc @pranavkm @SteveSandersonMS

@iSazonov
Copy link
Contributor

PowerShell could be benefit from this - it is based on reflection, emit and dynamic.

@xoofx
Copy link
Member

xoofx commented Nov 24, 2020

As mentioned in some of the previous issues, and to add a note on the requirements as there is a reference to Reflection.Emit that might make this not clear: the solution should not involve any managed objects from System.Reflection but go through only RuntimeXXXHandle that can work directly with ldtoken&co.

@jkotas
Copy link
Member

jkotas commented Nov 24, 2020

We have discussed that these APIs should be efficient and avoid allocating lots of managed memory: #28001 (comment) . Avoiding any managed objects from System.Reflection is much more stringent and unlikely to be practical.

@xoofx
Copy link
Member

xoofx commented Nov 24, 2020

Avoiding any managed objects from System.Reflection is much more stringent and unlikely to be practical.

Fair enough. I don't know the cases that would involve reflection, but as long as simple cases like accessing statically known e.g fields directly from IL via ldtoken, are working without, that's ok.

@davidfowl
Copy link
Member

How much work is this? Is there any way to make incremental progress in 6.0 towards it?

I'm writing up something on the code generation techniques used in ASP.NET Core and for the future reference I'd like an idea of how our APIs would need to evolve. The closest I've found is @jkotas 's comment here #10057 (comment).

@jkotas
Copy link
Member

jkotas commented May 16, 2021

We may be able to make some incremental progress on faster reflection in .NET by refactoring the internal implementation.

We won't be able to expose the new faster reflection APIs for .NET 6. For that, we need #26186 that depends on https://github.com/dotnet/csharplang/blob/master/proposals/low-level-struct-improvements.md that was cut for .NET 6.

@davidfowl
Copy link
Member

Got it. For .NET 7 then. I think I understand how to model the APIs we need to take advantage of this. Unfortunately this and async are at odds, but we can probably optimize the sync path with this and box in the async path. Still that would mean we need a way to create a TypedReference from this boxed value in a way that won't explode because the new API won't handle type coercion i.e a mix of this API needs to support a mix of boxed and unboxed values somehow. I can provide examples if needed.

@xoofx
Copy link
Member

xoofx commented May 26, 2021

Bumping again on the topic, more precisely for the support of offsetof and so by extension handleof(A.MyField) that we discussed here), can we add here that it might require a language + Roslyn support, so that we can build higher primitives/intrinsics around it? (e.g like Unsafe.OffsetOf(fieldToken)).

A scenario example for offsetof is when building for example an animation system that can animate dynamic (resolution based on data, not code) field values of a struct that is stored in an unmanaged memory buffer.

cc: @jaredpar (for the question about Language + Roslyn support)

@jaredpar
Copy link
Member

I'm not sure how the language can implement offsetof without some underlying runtime support as it's a value you can't determine until runtime. As for that and handleof I think it comes down to if there is enough benefit in adding it, particularly when it's just a think wrapper over .NET API calls.

@MichalStrehovsky
Copy link
Member

It would probably have to be through ldflda and managed pointer difference. There's an issue discussing that if we were to allow no.nullcheck ldflda, it would be doable with nice codegen - #40021 (comment).

@xoofx
Copy link
Member

xoofx commented May 27, 2021

I'm not sure how the language can implement offsetof without some underlying runtime support as it's a value you can't determine until runtime.

Yes, at native compile time (so runtime in the case of JIT) the value can be computed easily (it is part of the struct layout computation). So having an intrinsic like int RuntimeHelper.GetFieldOffset(RuntimeFieldHandle) could resolve to a constant in the IR of the compiler (if the runtime field handle is a const and coming from prior resolution with e.g handleof). Actually Unsafe.OffsetOf is probably a bad idea because we would need an intrinsic for it...

As for that and handleof I think it comes down to if there is enough benefit in adding it, particularly when it's just a think wrapper over .NET API calls.

Right the problem is that this is something we can do efficiently at the IL level already today (ldtoken) but I fail to see how we can express it with an API. We could maybe do something like RuntimeHelper.GetFieldOffset<MyStruct>("myfield") but it's no longer a ldtoken at the IL level, more work for the compiler (resolution from a string is quite bad as well, linear scan on fields...etc.)

It would probably have to be through ldflda and managed pointer difference.

Not sure, if ldflda is necessary here while we could implement it with an intrinsic directly.

@steveharter
Copy link
Member Author

steveharter commented Jul 13, 2021

This was prototyped for V6 but due to lack of language support around ref structs it must be deferred.

The prototype is based on TypedReference and is essentially as fast as IL Emit'd code (up to ~10x faster than standard reflection) while also supporting ref and out parameters and ref return values. Note that converting the existing object-based reflection code to IL Emit and not using TypedReference is ~3x faster, meaning it is worthwhile to extend the Emit-based approach to existing reflection as well.

@steveharter steveharter modified the milestones: 6.0.0, 7.0.0 Jul 13, 2021
@davidfowl
Copy link
Member

@steveharter if you start it now, we can finish it before .NET 7 preview 1 😄

@xoofx
Copy link
Member

xoofx commented Oct 24, 2023

Generate the IL that loads the constant and returns it. The IL is generated at runtime where all offsets are known. You need to be careful with R2R support under crossgen2 - the offset is not a constant for types outside the current version bubble.

Oh, right, I found that I have actually access to FieldDesc through the context, so I can fetch directly the offset. At least getting a minimum prototype should be hopefully not too complicated. Will open a PR once I have something working at least to verify it is the right direction.

@xoofx
Copy link
Member

xoofx commented Oct 24, 2023

For example, take a look at FieldDesc::GetOffset(). This should be enough to get a simple POC. There are many issues to consider though - EnC, crossgen2, blittability, etc.

hehe, yep, that's exactly I found! 😅

@xoofx
Copy link
Member

xoofx commented Oct 24, 2023

My apologies for this very basic question:

I have modified the src\tests\baseservices\compilerservices\UnsafeAccessors\UnsafeAccessorsTests.cs and I'm trying to run it as an individual test with the doc but the expected folder:

$Env:CORE_ROOT = '<repo_root>\artifacts\tests\coreclr\windows.<Arch>.<Configuration>\Tests\Core_Root'

does not contain corerun (and it looks like it needs to run xunit anyway).

So I'm not sure how to run the test easily. Any tip on how to run it?

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 24, 2023

The Core Root needs to be created. You can do this quickly by running the following:

.\src\tests\build.cmd %RUNTIME_ARCH% %RUNTIME_CONFIG% skipnative skipmanaged /p:LibrariesConfiguration=%LIBS_CONFIG%

@AaronRobinsonMSFT
Copy link
Member

@xoofx
Copy link
Member

xoofx commented Oct 24, 2023

Created a proof of concept PR #93946 with the Running Verify_AccessFieldOffsetClass test passing.

(Edit: The struggle I had above running the tests is likely that originally, I compiled clr+libs in release while I took the default - debug when compiling the tests 🤦)

@MichalPetryka
Copy link
Contributor

I feel like adding a property called Offset on RuntimeFieldHandle would make more sense here, the issue is though that the language doesn't expose fieldof today.

@jakobbotsch
Copy link
Member

It would probably have to be through ldflda and managed pointer difference. There's an issue discussing that if we were to allow no.nullcheck ldflda, it would be doable with nice codegen - #40021 (comment).

Hey, reviving the topic, I'm worried that this meta issue with a faster reflection is going to take more time, while we are specifically looking for a solution for field offset. Following the issue #40021 and testing it again with .NET 7 on sharplab here, for the following C# code:

using System;
public class C {

    // good codegen :)! ... but instant null ref ex
    public static unsafe int OffsetAsNull()
    {
        Example* p = null;
        return (int)(&p->Four - &p->Zero);
    }    
    
    public struct Example
    {
        public byte Zero;

        private byte _1, _2, _3;

        public byte Four;
    }
}

We are getting the following codegen, which has a NRE:

C.OffsetAsNull()
    L0000: movsx rax, byte ptr [0] // NRE
    L0009: mov eax, 4
    L000e: ret

Would it be possible, as a short term workaround, to check during JIT optimization for the presence of an offset calculation pattern and not generate any NRE? I'm more than willing to try to make a PR if such optimization could be accepted 🙂

We added JIT support for some of these patterns in .NET 8 in #81998. See e.g. this example: https://godbolt.org/z/or76frsWs

@DaZombieKiller
Copy link
Contributor

Shouldn't RuntimeHelpers.GetRawData be made public alongside the addition of UnsafeAccessorKind.FieldOffset? Otherwise there is no safe way to get the base reference for an arbitrary object. You'd have no way to actually use the offset in that scenario...

Without GetRawData, you need to do something like Unsafe.As<StrongBox<T>>(obj).Value, which has the potential to confuse the JIT because obj is not a StrongBox<T>.

@MichalPetryka
Copy link
Contributor

I feel like adding a property called Offset on RuntimeFieldHandle would make more sense here, the issue is though that the language doesn't expose fieldof today.

Actually, we could add UnsafeAccessor for Field and Method handles then instead to solve the lack of language features for fieldof and methodof and just expose the Offset as an intrinsic.

@Sergio0694
Copy link
Contributor

We were talking the other day about how IL allows overloading fields, and hence how you couldn't have an unsafe accessor returning eg. a ref readonly T to a type T having a base conversion from TField, because then you wouldn't be able to disambiguate which field exactly you're looking for. If you had a field accessor just returning a RuntimeFieldHandle, wouldn't you hit the same problem if multiple fields of different types with the same name were present in IL? 🤔

@MichalPetryka
Copy link
Contributor

We were talking the other day about how IL allows overloading fields, and hence how you couldn't have an unsafe accessor returning eg. a ref readonly T to a type T having a base conversion from TField, because then you wouldn't be able to disambiguate which field exactly you're looking for. If you had a field accessor just returning a RuntimeFieldHandle, wouldn't you hit the same problem if multiple fields of different types with the same name were present in IL? 🤔

It could be solved by adding a fake parameter for return type just like there is one for declaring type.

@jkotas
Copy link
Member

jkotas commented Oct 25, 2023

Shouldn't RuntimeHelpers.GetRawData be made public alongside the addition of UnsafeAccessorKind.FieldOffset? Otherwise there is no safe way to get the base reference for an arbitrary object. You'd have no way to actually use the offset in that scenario...

Yes, this would need to be thought through if the raw offsets are enabled for classes, and not just structs. Another problem with classes is that field offset is not always available. #28001 has related discussion.

@xoofx
Copy link
Member

xoofx commented Oct 25, 2023

Shouldn't RuntimeHelpers.GetRawData be made public alongside the addition of UnsafeAccessorKind.FieldOffset? Otherwise there is no safe way to get the base reference for an arbitrary object. You'd have no way to actually use the offset in that scenario...

Yes, this would need to be thought through if the raw offsets are enabled for classes, and not just structs. Another problem with classes is that field offset is not always available. #28001 has related discussion.

Couldn't we add instead a new intrinsic to Unsafe class that would make the usage of the offset quite more straightforward:

public static ref T AddByteOffset<T>(object obj, nint offset);
// ldarg.0, ldarg.1, add, ret

@hamarb123
Copy link
Contributor

hamarb123 commented Oct 25, 2023

public static ref T AddByteOffset<T>(object obj, nint offset);
// ldarg.0, ldarg.1, add, ret

I would think it couldn't be implemented like that, since it's invalid IL, but if there was a way to get the reference to field 0 from and object, and we did that after ldarg.0, it should work.

If we got this API (which I personally would like to get), it would be great if we also got the corresponding inverse API (should both of these be nuint? I think so personally, but it doesn't really matter to me - just makes sense since negative doesn't make sense for either of these):

public static object? GetObject<T>(ref T reference, out nuint offset);

@xoofx
Copy link
Member

xoofx commented Oct 25, 2023

I would think it couldn't be implemented like that, since it's invalid IL,

Just tested and calling the following code is working with .NET 7 JIT, but I haven't checked the specs in a while for such operation. One unknown I have with CoreCLR JIT/GC is what is seen after the add: is it an object ref, or is it a ref T. If it is the former, that could create GC corruption.

  .method public hidebysig static !!T& AddByteOffset<T>(object source, native int byteOffset) cil managed aggressiveinlining
  {
        .custom instance void System.Runtime.Versioning.NonVersionableAttribute::.ctor() = ( 01 00 00 00 )
        .maxstack 2
        ldarg.0
        ldarg.1
        add
        ret
  } // end of method Unsafe::AddByteOffset

@Sergio0694
Copy link
Contributor

"One unknown I have with CoreCLR JIT/GC is what is seen after the add: is it an object ref, or is it a ref T."

As far as I know, the type you declare in C#/IL doesn't matter. As far as the GC is concerned, that is simply "some GC pointer". In this case, it'll fall inside the data of an object, hence it's an interior pointer, so during mark the GC will consider the entire object as reachable, that's it. Shouldn't really cause any problems. In fact, reinterpreting the type of interior pointers (or GC refs in general) is quite common for various reasons.

@xoofx
Copy link
Member

xoofx commented Oct 25, 2023

Couldn't we add instead a new intrinsic to Unsafe class that would make the usage of the offset quite more straightforward

An additional benefit of providing ref T Unsafe.AddByteOffset<T>(object obj, nint offset); is that it makes it relatively similar when dealing with struct field offsetting, difference being that the struct field offsetting would require an additional ref cast.

@jkotas
Copy link
Member

jkotas commented Oct 25, 2023

Just tested and calling the following code is working with .NET 7 JIT, but I haven't checked the specs in a while for such operation.

This is invalid IL. If you run it on checked JIT, you will see all sorts of asserts. The first one is:

Assertion failed 'genActualType(op1->TypeGet()) != TYP_REF && genActualType(op2->TypeGet()) != TYP_REF : Possibly bad IL with CEE_add at offset 0002h (op1=ref op2=long stkDepth=0)' in 'Test:AddByteOffset[ubyte](System.Object,long):byref' during 'Importation' (IL size 4; hash 0xcdd16edf; Tier0)

It is likely that that the JIT can either crash or produce bad code when this gets method inlined into a callsite of a particular shape.

@xoofx
Copy link
Member

xoofx commented Oct 25, 2023

This is invalid IL. If you run it on checked JIT, you will see all sorts of asserts. The first one is:

Fair enough. We definitely don't want invalid IL 😅

So, exposing RuntimeHelpers.GetRawData would be the way and it would require to shift the offset with the method table in the UnsafeAccessor code in my PR, or could we provide a ref byte Unsafe.AsByteRef(object) { ldarg.0; ret; }?

I'm ok with RuntimeHelpers.GetRawData if it is the preferred way.

@hamarb123
Copy link
Contributor

My opinion is that the following set of APIs makes the most sense:

namespace System.Runtime.CompilerServices
{
    public static class RuntimeHelpers
    {
        public static ref byte GetRawData(object? o);
        public static object? GetObject(ref byte reference, out nuint offset);
    }
}

This provides the forward and reverse API to convert between byrefs and objects.

The lack of <T> reduces the number of generic instantiations we will get (if that still matters on some platforms? I can't recall).

Note object?: since we will need to check null anyway, I think it makes sense to check for null and give a null byref for GetRawData, instead of throwing, since it should be able to emit better code for that, and for GetObject it makes sense since reference could point to unmanaged/stack memory (in which case offset would equal the pointer).

When the documentation is written for these pair of APIs, we will need to consider their behaviour with strings and arrays - currently they would probably return a ref to the start of the length field - if this is how we want it to work (which probably makes sense), then we should document the offset from the length field to the first entry in these cases so people can use it correctly for these OR we could document that it's undefined to use these APIs and then try to determine/select what specific index it's at (this would allow us to change them to be 64 bit length in the future if needed), which could make sense since there are other APIs for working with these special cases in the "intended" way when indexing is desired.

@steveharter
Copy link
Member Author

Linking Roslyn issue dotnet/roslyn#68000 where you can't get an offset of a ref field.

@steveharter
Copy link
Member Author

We now have the PR at #93946 which adds UnsafeAccessorKind.FieldOffset (API issue pending).

But we need a champion to create the API issue for the RuntimeHelpers.GetRawData() and .GetObject() proposal above so we can move the discussion there.

@MichalStrehovsky
Copy link
Member

But we need a champion to create the API issue for the RuntimeHelpers.GetRawData()

I simply reopened #28001 since it already has a bunch of discussion and it's exactly that proposal. I cleared the milestone and marked untriaged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection Cost:XL Work that requires one engineer more than 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries tenet-performance Performance related issue User Story A single user-facing feature. Can be grouped under an epic.
Projects
No open projects
Development

No branches or pull requests