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

[API Proposal]: RuntimeFieldHandle.Offset #94976

Open
MichalPetryka opened this issue Nov 19, 2023 · 11 comments
Open

[API Proposal]: RuntimeFieldHandle.Offset #94976

MichalPetryka opened this issue Nov 19, 2023 · 11 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@MichalPetryka
Copy link
Contributor

Background and motivation

In some interop or unsafe manipulation scenarios being able to obtain field offset of a certain field without having to do ref arithmetics.
As such, exposing such offset on RuntimeFieldHandle seems like it would allow programmers to access it in a cheap way without writing code that can break depending on JIT optimizations.

The API would return a sentinel value of -1 for static fields and fields added by EnC to avoid exceptions in high performance code.

Together with #94975 this can supersede #93946.

API Proposal

namespace System;

public struct RuntimeFieldHandle
{
    public int Offset { get; }
}

API Usage

struct S
{
    public int a;
    public float b;
    public byte c;
    public double d;
}

Console.WriteLine(typeof(S).GetField("d").FieldHandle.Offset); // prints 16

Alternative Designs

#93946

Risks

This could make invalid code based on Offsets more common, same as #93946. This could confuse the runtimes and introduce unexpected crashes.

@MichalPetryka MichalPetryka added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 19, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 19, 2023
@teo-tsirpanis teo-tsirpanis added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 19, 2023
@ghost
Copy link

ghost commented Nov 19, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

In some interop or unsafe manipulation scenarios being able to obtain field offset of a certain field without having to do ref arithmetics.
As such, exposing such offset on RuntimeFieldHandle seems like it would allow programmers to access it in a cheap way without writing code that can break depending on JIT optimizations.

The API would return a sentinel value of -1 for static fields and fields added by EnC to avoid exceptions in high performance code.

Together with #94975 this can supersede #93946.

API Proposal

namespace System;

public struct RuntimeFieldHandle
{
    public int Offset { get; }
}

API Usage

struct S
{
    public int a;
    public float b;
    public byte c;
    public double d;
}

Console.WriteLine(typeof(S).GetField("d").FieldHandle.Offset); // prints 16

Alternative Designs

#93946

Risks

This could make invalid code based on Offsets more common, same as #93946. This could confuse the runtimes and introduce unexpected crashes.

Author: MichalPetryka
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@MichalStrehovsky
Copy link
Member

Together with #94975 this can supersede #93946.

Just a note that those are 100% non-reflection codepaths. This is pure reflection - might as well put this on FieldInfo.

@MichalPetryka
Copy link
Contributor Author

Just a note that those are 100% non-reflection codepaths. This is pure reflection

The runtime could fold accesses to Offset with ldtoken/proposed UnsafeAccessor so it could become "free reflection" like things like typeof(T).IsValueType are.

might as well put this on FieldInfo

I have three reasons why I think RuntimeFieldHandle is a better place:

  1. doing it on FieldInfo would require allocating a new instance to access it from handle and would make folding like I've described earlier harder.
  2. this is a lower level, runtime specific, concept so I feel like it's more appropriate to expose it here, just like GetFunctionPointer() is exposed on RuntimeMethodHandle, not MethodBase
  3. this property wouldn't make sense for custom, non runtime based FieldInfos and RuntimeFieldHandle is restricted to those.

@MichalStrehovsky
Copy link
Member

The runtime could fold accesses to Offset with ldtoken/proposed UnsafeAccessor so it could become "free reflection" like things like typeof(T).IsValueType are.

This would only get expanded if the JIT feels like it. It would probably not be expanded in Tier-0 for example. For Native AOT, the whole program analysis that we do before codegen cannot assume any particular codegen optimization so the whole program view will have "this is reflecting over a field". It's hard to clean that up afterwards both in terms of the field being visible from reflection, and in terms of the reflection infrastructure code to access fields.

@MichalPetryka
Copy link
Contributor Author

Would #83021 solve this issue?

@MichalStrehovsky
Copy link
Member

Would #83021 solve this issue?

It would solve the native AOT part of the problem, yes (provided the JIT expands it). It's not planned for .NET 9.

In general, I still don't see how this is better than #93946. This requires a lot of things to happen.

@yushroom
Copy link

yushroom commented Jan 19, 2024

@MichalStrehovsky
from discussion #97203
I'm a Unity game developer. I want to use NativeAOT to replace IL2cpp to improve performance. Unity engine runtime (both Mono and IL2CPP) relies heavily on field offset to access member of object. This proposal is really helpful.

@MichalStrehovsky
Copy link
Member

@yushroom would it be possible to add links to Unity docs about this?

I want to understand why you need managed offsets, as opposed to the native offsets (available using the existing Marshal.OffsetOf API). I would expect interop scenarios (such as interop with the Unity engine) to use interop APIs.

@hamarb123
Copy link
Contributor

hamarb123 commented Feb 18, 2024

I'm trying to implement Volatile.Read/Write/etc<T> for general T (non-atomic). To do this, I need to either be able to manually emit the applicable barrier (which I cannot do), or I need to be able to determine whether parts of a managed struct T are object or not.

It seems impossible to do on NAOT currently. On normal runtime I can get the size of a type with ILGen calls to sizeof and determine the field offset with ldflda and sub quite trivially (which would obviously be nice to be able to avoid) - this allows me to figure out whether the first and last sizeof(nuint) bytes are an object or not. The problem on NativeAOT is two-fold: firstly, I may have managed struct fields on my managed struct, so I need to be able to get the size from a Type, and secondly there is no way to get the field offsets, so I cannot see which parts of memory are managed.

I tried other ideas first (since I recall running into this issue before) of just boxing it and using reflection to set stuff and reading out all the parts which were managed (by only writing to either managed or unmanaged, and then comparing to 0), but the issues I faced here were: can't do it on inline arrays at all, padding (automatic or additional) cannot be written to with reflection so I cannot detect where it is, I cannot construct an object of any general type (therefore I cannot set it to the fields), and Marshal.OffsetOf won't even remotely do what I want.

The IL instructions, for which there are no built-in APIs, also don't work properly in a number of cases: #91530, but I feel like these APIs would be useful even if I didn't have the issue with implementing Volatile APIs that should work.

@jkotas
Copy link
Member

jkotas commented Feb 18, 2024

these APIs would be useful even if I didn't have the issue with implementing Volatile APIs that should work.

I do not see how these APIs can help you to do this efficiently and reliably with native AOT. If you are running into issues with volatile. prefix handling by the JIT, it would be best to fix the issue in the JIT. (Also, I expect that it would be much simpler fix than the work required to build the APIs proposed here.)

@hamarb123
Copy link
Contributor

hamarb123 commented Feb 18, 2024

I do not see how these APIs can help you to do this efficiently and reliably with native AOT.

Basically, I can implement them like this:

Write:

  • Write the first alignof(T) with Volatile. (this barrier allows these sub-"operations" to be reordered, and doesn't allow any to go earlier)
  • Write the rest of it normally

Read:

  • Read the rest of it normally
  • Read the last alignof(T) with Volatile. (this barrier allows these sub-"operations" to be reordered, and doesn't allow any to go later)

It's not perfect (it definitely could be faster - since the JIT doesn't combine all of the ops perfectly), but it's not too bad either. The problem on NativeAOT is that there is no way to determine for managed types whether those first & last alignof(T)s are managed or unmanaged.

If you are running into issues with volatile. prefix handling by the JIT, it would be best to fix the issue in the JIT. (Also, I expect that it would be much simpler fix than the work required to build the APIs proposed here.)

I'm happy for that to be done :) I just don't know how to do it - I would rather just use those, but for now that's not really an option.

@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jun 24, 2024
@stephentoub stephentoub added this to the Future milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

8 participants