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

Move static helpers to managed #108167

Merged
merged 30 commits into from
Oct 30, 2024

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Sep 24, 2024

Move the statics helpers from native code, occasionally using a HELPER_METHOD_FRAME, to managed code using QCalls for P/Invoke transition

Particularly notable parts of the change

  1. Thread static lookup relies on the jit thread static optimization pathway. I needed to build a parallel helper call enum value for the case where we need to use the optimized helper path, but didn't actually have the ability to optimize in the JIT.
  2. This change maintains the volatile read of the normal static values, through a custom JIT intrinsic used only in this codepath.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@davidwrighton davidwrighton marked this pull request as ready for review October 25, 2024 22:04
[StructLayout(LayoutKind.Sequential)]
internal unsafe ref struct DynamicStaticsInfo
{
internal const int ISCLASSINITED = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called ISCLASSNOTINITED to match the name in C++?

Comment on lines 19 to 21
[MethodImpl(MethodImplOptions.InternalCall)]
[Intrinsic]
private static extern ref byte VolatileReadAsByref(ref IntPtr address);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[MethodImpl(MethodImplOptions.InternalCall)]
[Intrinsic]
private static extern ref byte VolatileReadAsByref(ref IntPtr address);
[Intrinsic]
private static extern ref byte VolatileReadAsByref(ref IntPtr address) => ref VolatileReadAsByref(ref address);

Make it must-expand intrinsic and delete the FCall?

Comment on lines 884 to 885
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern ref byte MaskStaticsPointer(ref byte staticsPtr);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern ref byte MaskStaticsPointer(ref byte staticsPtr);
public static ref byte MaskStaticsPointer(ref byte staticsPtr)
{
fixed (byte* p = staticsPtr)
{
return ref *(byte*)((nuint)p & ~(nuint)ISCLASSNOTINITED;
}
}

Can this be implemented like this in C# to avoid FCall? It is not on any perf critical path, so a few extra instructions from pinning are not a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I had it on the critical path initially, and planned to make it an intrinsic to make it just work with it written as an fcall as a correct stopgap... then I realized I didn't need it on the critical path, and didn't move on from the FCALL.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static ref byte GetObjectAsRefByte(object obj)
{
return ref Unsafe.Add(ref Unsafe.As<RawData>(obj)._data, -sizeof(MethodTable*));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return ref Unsafe.Add(ref Unsafe.As<RawData>(obj)._data, -sizeof(MethodTable*));
return ref Unsafe.Subtract(ref Unsafe.As<RawData>(obj)._data, sizeof(MethodTable*));

/// <summary>
/// Given a statics pointer in the DynamicStaticsInfo, get the actual statics pointer.
/// </summary>
public static ref byte MaskStaticsPointer(ref byte staticsPtr)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This can be private method in StaticsHelpers. It is only used in that type.

// Thread static helpers

[StructLayout(LayoutKind.Sequential)]
private sealed class RawData
Copy link
Member

Choose a reason for hiding this comment

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

We have RawData and GetRawData in RuntimeHelpers.CoreCLR.cs that this should be able to use instead of defining a local clone.

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

Successfully merging this pull request may close these issues.

3 participants