Skip to content

Implement RuntimeHelpers.GetHashCode() happy path in C# #55273

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Implement RuntimeHelpers.GetHashCode() happy path in C# #55273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion ...eclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,32 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern void PrepareDelegate(Delegate d);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int GetHashCode(object? o)
{
if (o is not null)
{
ref IntPtr startOfDataRef = ref Unsafe.As<byte, IntPtr>(ref Unsafe.As<RawData>(o).Data);
ref IntPtr objectHeaderRef = ref Unsafe.Add(ref startOfDataRef, -2);
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 a GC hole. This byref will points to previous object. so it won't move together with the object that you are computing the hashcode for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch 😅
We were just about wondering whether the GC could track refs pointing to the object header, I wasn't completely sure but figured it might work given that the data was still part of the same object - guess I know now ahah
I've tried using fixed there to fix that but as expected that's pretty slow and loses virtually all performance improvements than the current solution, so not really worth it anymore. Will close the PR for now then.

While on the topic - @SingleAccretion found a GT_START_NONGC node in the emitter, and together with @EgorBo we were wondering whether it might make sense and/or be doable at all to introduce a new JIT intrinsic to be able to leverage that? Might make things like this possible without having to pin stuff and lose all performance gains? At the very least it sounds like a good learning opportunity so I thought I'd ask! Thanks! 😄

Copy link
Member

Choose a reason for hiding this comment

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

I do not think intrinsics for GT_START_NONGC make sense. It is so subtle and hard to get these things right. I have no problems with giving up the bit of performance that we would get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, yeah that makes perfect sense and it'd also be extremely niche anyway. Thanks! 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I've tried using fixed there to fix that but as expected that's pretty slow

Can the JIT do more optimizations around fixed to make it faster?

Copy link
Member

Choose a reason for hiding this comment

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

Codegen for "fixed" not much to optimize:

The spill to stack can be optimized. Nothing fundamental says that the pinned slot has to be on stack. The pinned value can be in register.

Copy link
Member

Choose a reason for hiding this comment

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

Codegen for "fixed" not much to optimize:

The tailcall optimization can be done as well, at least in theory. I guess it may be hard to do today since we do not know whether there is anything that matters pinned when we are deciding whether to tailcall. Maybe it can be helped by moving the pinning into a separate (inlineable) method to make it easier for the JIT to see that there is nothing actually pinned to block the tailcall optimization?

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 a similar issue for RuntimeHelpers.GetMethodTable. RuntimeHelpers.GetMethodTable cuts corners and I believe it does not compile into as efficient code as possible. I think it would be ok to introduce static T ReadAtByteOffset<T>(object o, int offset) intrinsic that would read T at given offset, without materializing o + offset as byref, as efficiently as possible. We can then use that intrinsic for both syncblock reading and GetMethodTable.

Copy link
Member

Choose a reason for hiding this comment

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

The spill to stack can be optimized. Nothing fundamental says that the pinned slot has to be on stack. The pinned value can be in register.

Ah, I didn't realize 👍

The tailcall optimization can be done as well

Yep, currently it's rejected with

Rejecting tail call in morph for call [000042]: Has Pinned Vars V01

Copy link
Member

Choose a reason for hiding this comment

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

NOTE: @Sergio0694 can't comment here since the thread is locked, but judging by Discord #lowlevel channel he is excited where it goes 🙂

uint syncBlockValue = Unsafe.As<IntPtr, ObjectHeader>(ref objectHeaderRef).SyncBlockValue;

const uint BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX = 0x08000000;
const uint BIT_SBLK_IS_HASHCODE = 0x04000000;
const uint BITS_IS_VALID_HASHCODE = BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX | BIT_SBLK_IS_HASHCODE;
const int HASHCODE_BITS = 26;
const uint MASK_HASHCODE = (1u << HASHCODE_BITS) - 1u;

if ((syncBlockValue & BITS_IS_VALID_HASHCODE) == BITS_IS_VALID_HASHCODE)
{
return unchecked((int)(syncBlockValue & MASK_HASHCODE));
}
}

return InternalGetHashCode(o);
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int GetHashCode(object? o);
private static extern int InternalGetHashCode(object? o);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern new bool Equals(object? o1, object? o2);
Expand Down Expand Up @@ -380,6 +404,15 @@ internal sealed class RawArrayData
public byte Data;
}

// Mapping for ObjHeader from src\coreclr\gc\env\gcenv.object.h
internal struct ObjectHeader
{
#if TARGET_64BIT
public uint Padding;
#endif
public uint SyncBlockValue;
}

// Subset of src\vm\methodtable.h
[StructLayout(LayoutKind.Explicit)]
internal unsafe struct MethodTable
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/classlibnative/bcltype/objectnative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ NOINLINE static INT32 GetHashCodeHelper(OBJECTREF objRef)
{
DWORD idx = 0;

FC_INNER_PROLOG(ObjectNative::GetHashCode);
FC_INNER_PROLOG(ObjectNative::InternalGetHashCode);

HELPER_METHOD_FRAME_BEGIN_RET_ATTRIB_1(Frame::FRAME_ATTR_EXACT_DEPTH|Frame::FRAME_ATTR_CAPTURE_DEPTH_2, objRef);

Expand All @@ -83,7 +83,7 @@ NOINLINE static INT32 GetHashCodeHelper(OBJECTREF objRef)

// Note that we obtain a sync block index without actually building a sync block.
// That's because a lot of objects are hashed, without requiring support for
FCIMPL1(INT32, ObjectNative::GetHashCode, Object* obj) {
FCIMPL1(INT32, ObjectNative::InternalGetHashCode, Object* obj) {

CONTRACTL
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/classlibnative/bcltype/objectnative.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class ObjectNative
// iff the Class object has already been created.
// If the Class object doesn't exist then you must call the GetClass() method.
static FCDECL1(Object*, GetObjectValue, Object* vThisRef);
static FCDECL1(INT32, GetHashCode, Object* vThisRef);
static FCDECL1(INT32, InternalGetHashCode, Object* vThisRef);
static FCDECL2(FC_BOOL_RET, Equals, Object *pThisRef, Object *pCompareRef);
static FCDECL1(Object*, AllocateUninitializedClone, Object* pObjUNSAFE);
static FCDECL1(Object*, GetClass, Object* pThis);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/ecalllist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ FCFuncStart(gRuntimeHelpers)
QCFuncElement("CompileMethod", ReflectionInvocation::CompileMethod)
QCFuncElement("PrepareMethod", ReflectionInvocation::PrepareMethod)
FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate)
FCFuncElement("GetHashCode", ObjectNative::GetHashCode)
FCFuncElement("InternalGetHashCode", ObjectNative::InternalGetHashCode)
FCFuncElement("Equals", ObjectNative::Equals)
FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone)
FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack)
Expand Down