dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

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

Open steveharter opened 3 years ago

steveharter commented 3 years ago

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.

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:

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 commented 3 years ago

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 commented 3 years ago

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

xoofx commented 3 years ago

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 commented 3 years ago

We have discussed that these APIs should be efficient and avoid allocating lots of managed memory: https://github.com/dotnet/runtime/issues/28001#issuecomment-442342820 . Avoiding any managed objects from System.Reflection is much more stringent and unlikely to be practical.

xoofx commented 3 years ago

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 commented 3 years ago

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 https://github.com/dotnet/runtime/issues/10057#issuecomment-377955723.

jkotas commented 3 years ago

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 https://github.com/dotnet/runtime/issues/26186 that depends on https://github.com/dotnet/csharplang/blob/master/proposals/low-level-struct-improvements.md that was cut for .NET 6.

davidfowl commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 - https://github.com/dotnet/runtime/issues/40021#issuecomment-666596692.

xoofx commented 3 years ago

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 commented 3 years ago

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.

davidfowl commented 3 years ago

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

rickbrew commented 2 years ago

I could also really use this in Paint.NET -- if you can believe it, I'm doing COM aggregation in C#. @tannergooding 's TerraFX.Interop.Windows package is very helpful for a lot of it, but my hand-rolled CCWs still need a way to calculate offsetof(field) in order to shift the this pointer for the InternalIUnknown implementation that is required for the aggregated objects. Right now I have a workaround but it requires additional boilerplate added to every CCW.

internal unsafe struct CBitmapLockCcw : IWICBitmapLock.Interface // also implements IUnknown.Interface
{
    ...
    private static readonly long offsetof_vtblInternalIUnknown;
    ...
    static CBitmapLockCcw()
    {
        // This is the workaround for not having `offsetof`
        CBitmapLockCcw self = default;
        offsetof_vtblInternalIUnknown = (byte*)&self.vtblInternalIUnknown - (byte*)&self;
    }

    private static CBitmapLockCcw* GetThisFromInternalIUnknownThis(CBitmapLockCcw* pInternalIUnknownThis)
    {
        return (CBitmapLockCcw*)((byte*)pInternalIUnknownThis - offsetof_vtblInternalIUnknown);
    }
    ...
    private IWICBitmapLock.Vtbl<CBitmapLockCcw>* vtblIWICBitmapLock;
    private IUnknown.Vtbl<CBitmapLockCcw>* vtblInternalIUnknown;
    private IUnknown* pUnkOuter;
    ...
    // Internal IUnknown
    public HRESULT InternalQueryInterface(Guid* riid, void** ppvObject)
    {
        if (riid == null || ppvObject == null)
        {
            return E_POINTER;
        }
        ...
    }

    [UnmanagedCallersOnly]
    private static int InternalQueryInterface(CBitmapLockCcw* pInternalIUnknownThis, Guid* riid, void** ppvObject)
    {
        return GetThisFromInternalIUnknownThis(pInternalIUnknownThis)->InternalQueryInterface(riid, ppvObject);
    }  
}
danmoseley commented 2 years ago

@steveharter in the Dependencies section above the only csharplang or roslyn issue I see there is https://github.com/dotnet/csharplang/pull/5602 which is already merged (and a design). Are there issues open that we can link there for our other dependencies on them?

IS4Code commented 1 year ago

I absolutely love the usage of TypedReference here, but I also think some sort of ByRef and ref struct wrapper, similar to System.Reflection.Pointer, might be nice to have in the future, for convenient handling of ref structs and references in general. It could be doable this way:

namespace System.Reflection;
public sealed class ByReference : IDisposable
{
    public TypedReference Value { get; private set; }
    private ByReference(TypedReference value) => Value = value;
    public static TResult Create<TArgs, TResult>(TypedReference value, Func<ByReference, TArgs, TResult> receiver, TArgs args) // also with delegate* and Action overloads
    {
        using var instance = new ByReference(value);
        return receiver(instance, args);
    }
    public void Dispose() => Value = default;
}

The idea here being that ByReference is usable only inside receiver, since it is disposed after the call and doesn't expose the reference anymore (well, it could happen with threading, but declaring it thread-unsafe might be all that's needed to "fix" that). Once there is a way for ref structs to be allowed as generic arguments (dotnet/csharplang#1148), a generic version like ByReference<ref T> might be useful too. This style of continuation passing could also be combined with await to get rid of the nesting.

This is not such a big improvement compared to being able to use TypedReference directly, but I really hope it also becomes something reflection could recognize, similarly to Pointer, since it can be easily incorporated into existing code (using Activator.CreateInstance, TypeDescriptor and dynamic).

DaZombieKiller commented 1 year ago

@IllidanS4 Your example would not work as written because TypedReference is a ref struct so it cannot be stored in a class. This basically looks like a variant of the pattern used in string.Create.

IS4Code commented 1 year ago

@IllidanS4 Your example would not work as written because TypedReference is a ref struct so it cannot be stored in a class.

@DaZombieKiller I am well aware of that; this is just to illustrate its function, sans the limitation of C#. I it doable by storing TypedReference* instead of TypedReference, but I am hoping for something standardized and recognized by reflection (even if it would have to be implemented using some magic).

Going a bit further, perhaps a custom interface would be ever better:

public interface IReflectionArgument
{
    TypedReference Value { get; }
}

So that anything that implements it could provide the actual value of the argument directly. The existing Pointer class could implement it, as well as the proposed ByReference.

DaZombieKiller commented 1 year ago

Do you have any examples of where and how this would be used? I'm having trouble seeing the benefits over just using TypedReference directly (unless I'm misunderstanding the purpose).

Since a TypedReference is a ref struct, ref escape rules should already prevent you from holding onto one beyond its lifetime. ByReference.Create seems to duplicate this, while also introducing the overhead of a delegate and try/finally block.

I could potentially see the advantage for storing a ref to a field inside a class, but that doesn't really require anything new other than the ability to get a ref to the field to begin with. For example, here's an implementation I've used before (based on how portable Span<T> worked):

public readonly struct InteriorRef<T>
{
    private readonly StrongBox<T> _pinnable;

    private readonly int _offset;

    public ref T Value => ref Unsafe.AddByteOffset(ref _pinnable.Value!, _offset);

    public InteriorRef(object obj, ref T reference)
    {
        ArgumentNullException.ThrowIfNull(obj);
        InteriorRef.ThrowIfInvalidReference(obj, ref reference);
        _pinnable = Unsafe.As<StrongBox<T>>(obj);
        _offset   = (int)Unsafe.ByteOffset(ref _pinnable.Value, ref reference);
    }

    public ref T GetPinnableReference()
    {
        return ref Value;
    }
}
IS4Code commented 1 year ago

Do you have any examples of where and how this would be used? I'm having trouble seeing the benefits over just using TypedReference directly (unless I'm misunderstanding the purpose).

@DaZombieKiller Perhaps I have not expressed myself clearly; the purpose is not to add a new way of handling references in general, but a way to bridge the gap between ref structs and standard reflection, to be able to incorporate these values even in code that uses the (soon to be) old-style reflection with object, to wrap them in something boxable (like Pointer.Box) but still safe to store and pass to any code, as long as the original ByReference.Create method is being executed.

So one scenario would be simply:

MethodInfo methodThatTakesSpanByte = ...;

Span<byte> arg = stackalloc byte[5];
ByReference.Create<ValueTuple, object>(__makeref(arg), (byref, _) => methodThatTakesSpanByte.Invoke(null, byref), default);
// Or perhaps simply, with an overload without TArgs
ByReference.Create(__makeref(arg), byref => methodThatTakesSpanByte.Invoke(null, byref));

Assuming MethodInfo.Invoke would understand an instance of ByReference just like it understands Pointer. Yes, this will also be possible with the new high-speed TypedReference-based reflection, but that would require getting around ref struct semantics


Another use of this, unrelated to reflection, would be allowing to use ref structs safely through lambdas, like this:

Span<int> arg = ...;
List<int> list = ...;
ByReference.Create(__makeref(arg), byref => list.Sum(i => __refvalue(byref.Value, Span<int>)[i]));
cyraid commented 1 year ago

Definitely being able to get offset of information would be nice. Ultimately, being able to set fields directly at startup (before instantiation) via a ptr would be nice (only knowing the instance ptr and field offset).

steveharter commented 1 year ago

Moving to 9.0. See comments at https://github.com/dotnet/runtime/issues/10057#issuecomment-1648580422

xoofx commented 10 months ago

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 🙂

AaronRobinsonMSFT commented 10 months ago

Instead of the above, I think a faster way to make traction here would be to look at a pattern like Unsafe.ByteOffset() and propose a new API that the JIT can lower safely.

xoofx commented 10 months ago

Instead of the above, I think a faster way to make traction here would be to look at a pattern like Unsafe.ByteOffset() and propose a new API that the JIT can lower safely.

How would such API work? Are you thinking of something like Unsafe.ByteOffset<Example>("Four") requiring the first parameter to be a const string?

AaronRobinsonMSFT commented 10 months ago

How would such API work? Are you thinking of something like Unsafe.ByteOffset<Example>("Four") requiring the first parameter to be a const string?

I was basing my suggestion on the following statement:

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 don't think permitting the JIT to "allow" that sort of operation is very tenable. Instead I think it would be much easier to take the pattern below and perform the operation. Basically, elide the new() and compute the result. It is easy to determine the default ctor is non-mutating and then unused, therefore can be elided. The call to ByteOffset() can then be pattern matched and the constant returned.

Example e = new();
ByteOffset(ref p.Four, ref p.Zero);

My only quibble was with permitting the current code, the idea itself seems reasonable to me.

xoofx commented 10 months ago

I don't think permitting the JIT to "allow" that sort of operation is very tenable. Instead I think it would be much easier to take the pattern below and perform the operation. Basically, elide the new() and compute the result. It is easy to determine the default ctor is non-mutating and then unused, therefore can be elided. The call to ByteOffset() can then be pattern matched and the constant returned.

Ok, thanks for the clarification, understood. I'm not sure if that would be more practical. We have cases where we would like to use this with e.g Class instances that could have a constructor. Removing the call to new() would be more laborious to detect and quite a stretch in terms of JIT optimization, compared to e.g allowing difference between 2 ldfla if both target is null.

jkotas commented 10 months ago

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 do not think we should be deviating from the speced behavior to make this pattern "work".

IS4Code commented 10 months ago

I don't think permitting the JIT to "allow" that sort of operation is very tenable. Instead I think it would be much easier to take the pattern below and perform the operation. Basically, elide the new() and compute the result. It is easy to determine the default ctor is non-mutating and then unused, therefore can be elided. The call to ByteOffset() can then be pattern matched and the constant returned.

Ok, thanks for the clarification, understood. I'm not sure if that would be more practical. We have cases where we would like to use this with e.g Class instances that could have a constructor. Removing the call to new() would be more laborious to detect and quite a stretch in terms of JIT optimization, compared to e.g allowing difference between 2 ldfla if both target is null.

@xoofx Why are constructors an issue when you have FormatterServices.GetSafeUninitializedObject? And in any case the offset is basically constant for the given members so you can pretty much cache the result and forget any expensive operation happened.

xoofx commented 10 months ago

@xoofx Why are constructors an issue when you have FormatterServices.GetSafeUninitializedObject? And in any case the offset is basically constant for the given members so you can pretty much cache the result and forget any expensive operation happened.

We cannot afford creating thousands of uninitialised objects just to get the offset of their fields. The offset of a field is an information fully available at JIT/AOT time by the compiler, that in the end resolves to a constant. I participated to these discussions already 5 years ago, foreseeing that we would have such issues when migrating to CoreCLR.

In the context of Unity Game Engine, we are actually starting to get some stuffs running with CoreCLR, which is great, but we are also starting to see teams that were relying on field offsets for fast serialization/animation systems getting blocked, and we don't have an easy solution for them right now.

I do not think we should be deviating from the speced behavior to make this pattern "work".

Understood. We will sync offline with your team to see how we could proceed. Thanks.

jkotas commented 10 months ago

We can consider adding UnsafeAccessorKind.FieldOffset. It would return the field offset when it is available and throw when it is not (e.g. for EnC added fields).

xoofx commented 10 months ago

We can consider adding UnsafeAccessorKind.FieldOffset. It would return the field offset when it is available and throw when it is not (e.g. for EnC added fields).

Oh, that's actually a pretty nice idea, I like it! 😍

AaronRobinsonMSFT commented 10 months ago

We can consider adding UnsafeAccessorKind.FieldOffset. It would return the field offset when it is available and throw when it is not (e.g. for EnC added fields).

@jkotas Are you suggesting the following? The assumption being Zero would need to be an instance field and not a static field.

[UnsafeAccessor(UnsafeAccessorKind.FieldOffset, Name="Zero")]
extern static nint GetZeroOffset(Example d);
xoofx commented 10 months ago

@jkotas Are you suggesting the following? The assumption being Zero would need to be an instance field and not a static field.

[UnsafeAccessor(UnsafeAccessorKind.FieldOffset, Name="Zero")]
extern static nint GetZeroOffset(Example d);

Yeah, I was starting to think how that would work. In the end, we would always pass a null reference for such method, but it might feel a bit awkward... Similarly, for struct, we should have to pass a default value, hoping that the JIT will discard the struct initialized on the stack (unless we could support that we could declare it by ref and pass a null ref?)

Ideally, that would be easier to have something like this:

[UnsafeAccessor(UnsafeAccessorKind.FieldOffset, Name="Zero")]
extern static nint GetZeroOffset<T>();

But it feels also a bit awkward as well, as that could be error prone. Thoughts?

AaronRobinsonMSFT commented 10 months ago

hoping that the JIT will discard the struct initialized on the stack (unless we could support that we could declare it by ref and pass a null ref?)

There are existing cases where the first argument is ignored (for example, UnsafeAccessorKind.StaticField) we can do the same thing here and permit a ref that accepts a null reference through Unsafe.NullRef<T>().

xoofx commented 10 months ago

Great. I can create a separate PR proposal for the small API change (adding the enum UnsafeAccessorKind.FieldOffset) and would be happy to implement it if that's accepted. 😊

AaronRobinsonMSFT commented 10 months ago

@xoofx I don't see why this would be much of an issue. Would be appreciative of any help in this area. The place to start is at:

https://github.com/dotnet/runtime/blob/a270140281a13ab82a4401dff3da6d27fe499087/src/coreclr/vm/prestub.cpp#L1431

xoofx commented 10 months ago

It looks like the GenerateAccessor is generating plain IL (e.g pCode->EmitLDFLDA(pCode->GetToken(cxt.TargetField));break;).

How do I generate IL at this level that could avoid the null check? (otherwise I'm back to the similar restrictions that we are hitting with existing unsafe workaround)

jkotas commented 10 months ago

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.

AaronRobinsonMSFT commented 10 months ago

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.

xoofx commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@xoofx It is in the referenced file, but it is a term that isn't obvious - https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/coreclr/testing.md#building-the-core_root

xoofx commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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>.