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

Convert fast path of ValueType.GetHashCode to managed #97590

Merged
merged 19 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,9 @@ internal static unsafe bool ObjectHasComponentSize(object obj)
[return: MarshalAs(UnmanagedType.Bool)]
internal static unsafe partial bool AreTypesEquivalent(MethodTable* pMTa, MethodTable* pMTb);

[LibraryImport(QCall, EntryPoint = "MethodTable_GetTypeID")]
internal static unsafe partial uint GetTypeID(MethodTable* pMT);

/// <summary>
/// Allocate memory that is associated with the <paramref name="type"/> and
/// will be freed if and when the <see cref="Type"/> is unloaded.
Expand Down Expand Up @@ -467,7 +470,13 @@ internal unsafe struct MethodTable

// Additional conditional fields (see methodtable.h).
// m_pModule
// m_pAuxiliaryData

/// <summary>
/// A pointer to auxiliary data that is cold for method table.
/// </summary>
[FieldOffset(AuxiliaryDataOffset)]
public MethodTableAuxiliaryData* AuxiliaryData;

// union {
// m_pEEClass (pointer to the EE class)
// m_pCanonMT (pointer to the canonical method table)
Expand Down Expand Up @@ -528,6 +537,12 @@ internal unsafe struct MethodTable

private const int ParentMethodTableOffset = 0x10 + DebugClassNamePtr;

#if TARGET_64BIT
private const int AuxiliaryDataOffset = 0x20 + DebugClassNamePtr;
#else
private const int AuxiliaryDataOffset = 0x18 + DebugClassNamePtr;
#endif

#if TARGET_64BIT
private const int ElementTypeOffset = 0x30 + DebugClassNamePtr;
#else
Expand Down Expand Up @@ -615,6 +630,28 @@ public TypeHandle GetArrayElementTypeHandle()
public extern uint GetNumInstanceFieldBytes();
}

// Subset of src\vm\methodtable.h
[StructLayout(LayoutKind.Explicit)]
internal unsafe struct MethodTableAuxiliaryData
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this is the desired direction to expose this, but it's relatively simple.

{
[FieldOffset(0)]
private uint Flags;

private const uint enum_flag_CanCompareBitsOrUseFastGetHashCode = 0x0001; // Is any field type or sub field type overrode Equals or GetHashCode
private const uint enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode = 0x0002; // Whether we have checked the overridden Equals or GetHashCode

public bool HasCheckedCanCompareBitsOrUseFastGetHashCode => (Flags & enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode) != 0;

public bool CanCompareBitsOrUseFastGetHashCode
{
get
{
Debug.Assert(HasCheckedCanCompareBitsOrUseFastGetHashCode);
return (Flags & enum_flag_CanCompareBitsOrUseFastGetHashCode) != 0;
}
}
}

/// <summary>
/// A type handle, which can wrap either a pointer to a <c>TypeDesc</c> or to a <see cref="MethodTable"/>.
/// </summary>
Expand Down
64 changes: 58 additions & 6 deletions src/coreclr/System.Private.CoreLib/src/System/ValueType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System
{
[Serializable]
[TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
public abstract class ValueType
public abstract partial class ValueType
{
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification = "Trimmed fields don't make a difference for equality")]
Expand All @@ -36,7 +37,7 @@ public override unsafe bool Equals([NotNullWhen(true)] object? obj)

// if there are no GC references in this object we can avoid reflection
// and do a fast memcmp
if (CanCompareBits(this))
if (CanCompareBitsOrUseFastGetHashCode(RuntimeHelpers.GetMethodTable(obj))) // MethodTable kept alive by access to object below
{
return SpanHelpers.SequenceEqual(
ref RuntimeHelpers.GetRawData(this),
Expand Down Expand Up @@ -66,8 +67,23 @@ ref RuntimeHelpers.GetRawData(obj),
return true;
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern bool CanCompareBits(object obj);
// Return true if the valuetype does not contain pointer, is tightly packed,
// does not have floating point number field and does not override Equals method.
private static unsafe bool CanCompareBitsOrUseFastGetHashCode(MethodTable* pMT)
{
MethodTableAuxiliaryData* pAuxData = pMT->AuxiliaryData;

if (pAuxData->HasCheckedCanCompareBitsOrUseFastGetHashCode)
{
return pAuxData->CanCompareBitsOrUseFastGetHashCode;
}

return CanCompareBitsOrUseFastGetHashCodeHelper(pMT);
}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "MethodTable_CanCompareBitsOrUseFastGetHashCode")]
[return: MarshalAs(UnmanagedType.Bool)]
private static unsafe partial bool CanCompareBitsOrUseFastGetHashCodeHelper(MethodTable* pMT);

/*=================================GetHashCode==================================
**Action: Our algorithm for returning the hashcode is a little bit complex. We look
Expand All @@ -79,8 +95,44 @@ ref RuntimeHelpers.GetRawData(obj),
**Arguments: None.
**Exceptions: None.
==============================================================================*/
[MethodImpl(MethodImplOptions.InternalCall)]
public extern override int GetHashCode();
public override unsafe int GetHashCode()
{
// The default implementation of GetHashCode() for all value types.
// Note that this implementation reveals the value of the fields.
// So if the value type contains any sensitive information it should
// implement its own GetHashCode().

MethodTable* pMT = RuntimeHelpers.GetMethodTable(this);

// We don't want to expose the method table pointer in the hash code
// Let's use the typeID instead.
uint typeID = RuntimeHelpers.GetTypeID(pMT);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this something we still care about? I assume GetType().GetHashCode() would be slower.

Copy link
Member

Choose a reason for hiding this comment

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

I think running the methodtable pointer through HashCode would be good enough. If you do that, the multiplication on the next won't be necessary.


// To get less colliding and more evenly distributed hash codes,
// we munge the class index with two big prime numbers
int hashCode = (int)(typeID * 711650207 + 2506965631U);

if (CanCompareBitsOrUseFastGetHashCode(pMT))
{
// this is a struct with no refs and no "strange" offsets
HashCode hash = default;
uint size = pMT->GetNumInstanceFieldBytes();
hash.AddBytes(MemoryMarshal.CreateReadOnlySpan(ref this.GetRawData(), (int)size));
hashCode ^= hash.ToHashCode();
}
else
{
object obj = this;
hashCode ^= RegularGetValueTypeHashCode(pMT, ObjectHandleOnStack.Create(ref obj));
}

GC.KeepAlive(this);
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be unnecessary

Copy link
Member

Choose a reason for hiding this comment

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

Right

return hashCode;
}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ValueType_RegularGetValueTypeHashCode")]
[SuppressGCTransition]
Copy link
Member Author

Choose a reason for hiding this comment

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

The unmanaged slow path manipulates the object reference in COOP mode

Copy link
Member

Choose a reason for hiding this comment

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

It can also throw and call back into managed code. It is not ok to use SuppressGCTransition with that.

SuppressGCTransition can be only used for leaf methods.

Copy link
Member

Choose a reason for hiding this comment

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

If you would like to make it more managed, you can change the helper to return the strategy to use to compute the hashcode:

  • Call object.GetHashCode for object reference at given offset
  • Call double.GetHashCode for field at given offset
  • Call float.GetHashCode for field at given offset
  • Hash N bytes from given offset (this can also cover the case where CanCompareBitsOrUseFastGetHashCode was not computed)

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jan 27, 2024

Choose a reason for hiding this comment

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

Got some chanllenge when handling the recursive case.
How to pass a reference pointing to the middle of an object to QCall? There's a leaf method GetFieldTypeHandleThrowing marked as throwing, so it looks unsuitable for FCall since we are moving away from HELPER_METHOD_FRAME.
Or, will it really throw when loading a field handle of already boxed type? Is there any alternate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, P/Invoke generator can pin the ref byte. Is it reliable/performant for object fields? Can GCPROTECT be omitted then?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would work better if you return field offset from the QCall and compute the byref on the managed side. Nothing to GC protect, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the recursive value type case, it requires a reference pointing to the field, which is not a top level object. Passing object handle+offset does work, but looks a bit confusing if more levels are involved.

Copy link
Member

Choose a reason for hiding this comment

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

Compound offset like this is not uncommon. It is frequently used e.g. by JITed code.

My preference is to have less manually managed code. Less GC mode switches in VM and less manually managed code that runs in a cooperative mode is goodness.

private static unsafe partial int RegularGetValueTypeHashCode(MethodTable* pMT, ObjectHandleOnStack obj);

public override string? ToString()
{
Expand Down
119 changes: 38 additions & 81 deletions src/coreclr/vm/comutilnative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1681,45 +1681,21 @@ BOOL CanCompareBitsOrUseFastGetHashCode(MethodTable* mt)
return canCompareBitsOrUseFastGetHashCode;
}

NOINLINE static FC_BOOL_RET CanCompareBitsHelper(MethodTable* mt, OBJECTREF objRef)
extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable * mt)
{
FC_INNER_PROLOG(ValueTypeHelper::CanCompareBits);

_ASSERTE(mt != NULL);
_ASSERTE(objRef != NULL);
QCALL_CONTRACT;

BOOL ret = FALSE;

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

ret = CanCompareBitsOrUseFastGetHashCode(mt);

HELPER_METHOD_FRAME_END();
FC_INNER_EPILOG();
END_QCALL;

FC_RETURN_BOOL(ret);
return ret;
}

// Return true if the valuetype does not contain pointer, is tightly packed,
// does not have floating point number field and does not override Equals method.
FCIMPL1(FC_BOOL_RET, ValueTypeHelper::CanCompareBits, Object* obj)
{
FCALL_CONTRACT;

_ASSERTE(obj != NULL);
MethodTable* mt = obj->GetMethodTable();

if (mt->HasCheckedCanCompareBitsOrUseFastGetHashCode())
{
FC_RETURN_BOOL(mt->CanCompareBitsOrUseFastGetHashCode());
}

OBJECTREF objRef(obj);

FC_INNER_RETURN(FC_BOOL_RET, CanCompareBitsHelper(mt, objRef));
}
FCIMPLEND

static INT32 FastGetValueTypeHashCodeHelper(MethodTable *mt, void *pObjRef)
{
CONTRACTL
Expand Down Expand Up @@ -1842,65 +1818,25 @@ static INT32 RegularGetValueTypeHashCode(MethodTable *mt, void *pObjRef)
return hashCode;
}

// The default implementation of GetHashCode() for all value types.
// Note that this implementation reveals the value of the fields.
// So if the value type contains any sensitive information it should
// implement its own GetHashCode().
FCIMPL1(INT32, ValueTypeHelper::GetHashCode, Object* objUNSAFE)
extern "C" INT32 QCALLTYPE ValueType_RegularGetValueTypeHashCode(MethodTable * pMT, QCall::ObjectHandleOnStack objHandle)
{
FCALL_CONTRACT;

if (objUNSAFE == NULL)
FCThrow(kNullReferenceException);

OBJECTREF obj = ObjectToOBJECTREF(objUNSAFE);
VALIDATEOBJECTREF(obj);

INT32 hashCode = 0;
MethodTable *pMT = objUNSAFE->GetMethodTable();

// We don't want to expose the method table pointer in the hash code
// Let's use the typeID instead.
UINT32 typeID = pMT->LookupTypeID();
if (typeID == TypeIDProvider::INVALID_TYPE_ID)
CONTRACTL
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
{
// If the typeID has yet to be generated, fall back to GetTypeID
// This only needs to be done once per MethodTable
HELPER_METHOD_FRAME_BEGIN_RET_1(obj);
typeID = pMT->GetTypeID();
HELPER_METHOD_FRAME_END();
}
THROWS;
GC_TRIGGERS;
MODE_COOPERATIVE;
} CONTRACTL_END;

// To get less colliding and more evenly distributed hash codes,
// we munge the class index with two big prime numbers
hashCode = typeID * 711650207 + 2506965631U;
INT32 ret = 0;

BOOL canUseFastGetHashCodeHelper = FALSE;
if (pMT->HasCheckedCanCompareBitsOrUseFastGetHashCode())
{
canUseFastGetHashCodeHelper = pMT->CanCompareBitsOrUseFastGetHashCode();
}
else
{
HELPER_METHOD_FRAME_BEGIN_RET_1(obj);
canUseFastGetHashCodeHelper = CanCompareBitsOrUseFastGetHashCode(pMT);
HELPER_METHOD_FRAME_END();
}
BEGIN_QCALL;

if (canUseFastGetHashCodeHelper)
{
hashCode ^= FastGetValueTypeHashCodeHelper(pMT, obj->UnBox());
}
else
{
HELPER_METHOD_FRAME_BEGIN_RET_1(obj);
hashCode ^= RegularGetValueTypeHashCode(pMT, obj->UnBox());
HELPER_METHOD_FRAME_END();
}
ret = RegularGetValueTypeHashCode(pMT, objHandle.Get()->UnBox());

return hashCode;
END_QCALL;

return ret;
}
FCIMPLEND

FCIMPL1(UINT32, MethodTableNative::GetNumInstanceFieldBytes, MethodTable* mt)
{
Expand All @@ -1924,6 +1860,27 @@ extern "C" BOOL QCALLTYPE MethodTable_AreTypesEquivalent(MethodTable* mta, Metho
return bResult;
}

extern "C" UINT32 QCALLTYPE MethodTable_GetTypeID(MethodTable * mt)
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
{
QCALL_CONTRACT;

UINT32 typeID = 0;

BEGIN_QCALL;

typeID = mt->LookupTypeID();
if (typeID == TypeIDProvider::INVALID_TYPE_ID)
{
// If the typeID has yet to be generated, fall back to GetTypeID
// This only needs to be done once per MethodTable
typeID = mt->GetTypeID();
}

END_QCALL;

return typeID;
}

static MethodTable * g_pStreamMT;
static WORD g_slotBeginRead, g_slotEndRead;
static WORD g_slotBeginWrite, g_slotEndWrite;
Expand Down
9 changes: 3 additions & 6 deletions src/coreclr/vm/comutilnative.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,18 +245,15 @@ class COMInterlocked

extern "C" void QCALLTYPE Interlocked_MemoryBarrierProcessWide();

class ValueTypeHelper {
public:
static FCDECL1(FC_BOOL_RET, CanCompareBits, Object* obj);
static FCDECL1(INT32, GetHashCode, Object* objRef);
};

class MethodTableNative {
public:
static FCDECL1(UINT32, GetNumInstanceFieldBytes, MethodTable* mt);
};

extern "C" BOOL QCALLTYPE MethodTable_AreTypesEquivalent(MethodTable* mta, MethodTable* mtb);
extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable* mt);
extern "C" UINT32 QCALLTYPE MethodTable_GetTypeID(MethodTable* mt);
extern "C" INT32 QCALLTYPE ValueType_RegularGetValueTypeHashCode(MethodTable * pMT, QCall::ObjectHandleOnStack objHandle);
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved

class StreamNative {
public:
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/vm/ecalllist.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,6 @@ FCFuncStart(gStringFuncs)
FCDynamicSig(COR_CTOR_METHOD_NAME, &gsig_IM_PtrSByt_Int_Int_Encoding_RetVoid, ECall::CtorSBytePtrStartLengthEncodingManaged)
FCFuncEnd()

FCFuncStart(gValueTypeFuncs)
FCFuncElement("CanCompareBits", ValueTypeHelper::CanCompareBits)
FCFuncElement("GetHashCode", ValueTypeHelper::GetHashCode)
FCFuncEnd()

FCFuncStart(gDiagnosticsDebugger)
FCFuncElement("BreakInternal", DebugDebugger::Break)
FCFuncElement("get_IsAttached", DebugDebugger::IsDebuggerAttached)
Expand Down Expand Up @@ -660,7 +655,6 @@ FCClassElement("Thread", "System.Threading", gThreadFuncs)
FCClassElement("ThreadPool", "System.Threading", gThreadPoolFuncs)
FCClassElement("Type", "System", gSystem_Type)
FCClassElement("TypedReference", "System", gTypedReferenceFuncs)
FCClassElement("ValueType", "System", gValueTypeFuncs)
#ifdef FEATURE_COMINTEROP
FCClassElement("Variant", "System", gVariantFuncs)
#endif
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/vm/qcallentrypoints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ static const Entry s_QCall[] =
DllImportEntry(QCall_GetGCHandleForTypeHandle)
DllImportEntry(QCall_FreeGCHandleForTypeHandle)
DllImportEntry(MethodTable_AreTypesEquivalent)
DllImportEntry(MethodTable_CanCompareBitsOrUseFastGetHashCode)
DllImportEntry(MethodTable_GetTypeID)
DllImportEntry(ValueType_RegularGetValueTypeHashCode)
DllImportEntry(RuntimeTypeHandle_MakePointer)
DllImportEntry(RuntimeTypeHandle_MakeByRef)
DllImportEntry(RuntimeTypeHandle_MakeSZArray)
Expand Down
Loading