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.

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

Jump to bottom

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Merged
merged 17 commits into from
Feb 26, 2024
Merged

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d108638
Equals
huoyaoyuan Jan 29, 2024
5c827a5
GetHashCode
huoyaoyuan Jan 29, 2024
581bb9c
AllocateUninitializedClone
huoyaoyuan Jan 29, 2024
d3b9dcc
GetClass
huoyaoyuan Jan 29, 2024
62fa673
Use GetTypeFromHandleUnsafe instead to reduce code duplication
huoyaoyuan Jan 29, 2024
4cc5699
Merge branch 'main'
huoyaoyuan Feb 22, 2024
6a56bd6
Revert change of GetType
huoyaoyuan Feb 22, 2024
fb27520
Merge diff for AllocateUninitializedClone
huoyaoyuan Feb 22, 2024
8c45bd3
Compare object content in native code
huoyaoyuan Feb 23, 2024
84ff6d1
Fix return
huoyaoyuan Feb 23, 2024
46e9b7c
Revert changes around GetHashCode
huoyaoyuan Feb 23, 2024
1bac399
Remove unused functions
huoyaoyuan Feb 23, 2024
e041418
Do not compare padding in Equals
huoyaoyuan Feb 24, 2024
e7bbdd4
Fix assert touching OBJECTREF in preemptive mode
huoyaoyuan Feb 25, 2024
2d46bc0
Add test coverage for Equals
huoyaoyuan Feb 25, 2024
633f525
Fix FCall method binding
huoyaoyuan Feb 26, 2024
f2a0fc7
Add more test cases
huoyaoyuan Feb 26, 2024
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
15 changes: 12 additions & 3 deletions src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace System
Expand All @@ -9,8 +10,14 @@ public partial class Object
{
// Returns a Type object which represent this object instance.
[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
public extern Type GetType();
public unsafe Type GetType()
Copy link
Member

@EgorBo EgorBo Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

First of all, it needs NoInlining on it. It's a jit intrinsic and if it gets inlined it might ruin some opts.
Also, it feels like it's potentially doing more work prior calling runtime helper - are there any benchmarks? Presumably, GetType() is perf critical. I was thinking of optimizing Object.GetType in jit here #88631

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 helper is also doing unnecessary check for null and TypeDesc. With NoInling, the performance is likely to regress a lot.

It's not so clear about the desired approach - expanding in JIT or exposing more fields to managed code? Would GetHashCode also benefit from this?

Copy link
Member

@EgorBo EgorBo Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The helper is also doing unnecessary check for null and TypeDesc. With NoInling, the performance is likely to regress a lot.

I don't think I follow, I meant NoInlining on top of GetType() you can check JIT codebase for NI_System_Object_GetType intrinsics - all those opts won't work if it gets inlined (JIT will have to special case GetTypeFromHandleUnsafe)

Would GetHashCode also benefit from this?

Object.GetHashCode can also be optimized on C# side like this #55273, it just needs a jit intrinsic to guarantee GC-safe access for object's header (exterior pointer). or can be expanded in JIT as well.

Copy link
Member

Choose a reason for hiding this comment

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

It's a jit intrinsic and if it gets inlined it might ruin some opts.

JIT intrinsics take precedence over inlining, no? Many JIT intrinsics are inlineable, but we do not have them marked as no inlining.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we use similar managed implementation in native AOT. If this has a problem, the native AOT implementation has the same problem.

Copy link
Member

Choose a reason for hiding this comment

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

JIT intrinsics take precedence over inlining, no? Many JIT intrinsics are inlineable, but we do not have them marked as no inlining.

Nope, JIT is free to inline them. In 95% of cases it's not a problem because most intrinsic-related optimizations happen directly in importer prior inlining. There are only a few which are special and GetObject is one of those
E.g.

    case NI_System_Array_Clone:
    case NI_System_Collections_Generic_Comparer_get_Default:
    case NI_System_Collections_Generic_EqualityComparer_get_Default:
    case NI_System_Object_MemberwiseClone:
    case NI_System_Threading_Thread_get_CurrentThread:
    case NI_System_Object_GetType:
    case NI_System_Text_UTF8Encoding_UTF8EncodingSealed_ReadUtf8:
    case NI_System_SpanHelpers_SequenceEqual:
    case NI_System_Buffer_Memmove:

Some of them are just too big to be inlined anyway, internal calls or it's just not a big deal if jit inlines them.

Copy link
Member Author

Choose a reason for hiding this comment

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

With AuxiliaryData being exposed in #97590, all the necessary information for fast path seems to be exposed to managed code. Non-collectible RuntimeType objects are now allocated on FOH so it will always be GC safe.

I'm concerned about the code being fragile and depend more implementation details like the FOH optimization. It's also hard to do optimization for collectible types.

Copy link
Member

Choose a reason for hiding this comment

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

Can we disable the inlining for these in the JIT instead if the JIT does not want them inlined?

Disabled inlining has secondary effect on code quality (forces stackwalkable frame, etc.). It will be hard to avoid perf regressions if the fast path has to be marked as disabled inlining.

Copy link
Member

Choose a reason for hiding this comment

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

Can we disable the inlining for these in the JIT instead if the JIT does not want them inlined?

Yeah, it can it be done

{
// Throws NullReferenceException as expected
MethodTable* pMT = RuntimeHelpers.GetMethodTable(this);
Type type = Type.GetTypeFromHandleUnsafe((IntPtr)pMT);
GC.KeepAlive(this);
return type;
}

// Returns a new object instance that is a memberwise copy of this
// object. This is always a shallow copy of the instance. The method is protected
Expand All @@ -19,7 +26,9 @@ public partial class Object
[Intrinsic]
protected internal unsafe object MemberwiseClone()
{
object clone = RuntimeHelpers.AllocateUninitializedClone(this);
object clone = this;
RuntimeHelpers.AllocateUninitializedClone(ObjectHandleOnStack.Create(ref clone));
Debug.Assert(clone != this);

// copy contents of "this" to the clone

Expand Down
50 changes: 44 additions & 6 deletions ...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 @@ -125,8 +125,21 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern void PrepareDelegate(Delegate d);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern int GetHashCode(object? o);
public static int GetHashCode(object? o)
{
if (o is null)
return 0;

int hashCode = TryGetHashCode(o);
if (hashCode != 0)
return hashCode;

object objRef = o;
return GetHashCodeHelper(ObjectHandleOnStack.Create(ref objRef));
}

[LibraryImport(QCall, EntryPoint = "ObjectNative_GetHashCodeHelper")]
private static partial int GetHashCodeHelper(ObjectHandleOnStack objHandle);

/// <summary>
/// If a hash code has been assigned to the object, it is returned. Otherwise zero is
Expand All @@ -139,8 +152,33 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern int TryGetHashCode(object o);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern new bool Equals(object? o1, object? o2);
public static new unsafe bool Equals(object? o1, object? o2)
Copy link
Member

Choose a reason for hiding this comment

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

We do not seem to have any test coverage for this method. Could you please add some?

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 how to express the mentioned issue about padding bits.

Copy link
Member

Choose a reason for hiding this comment

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

If we had any regular test coverage for this API, we would likely see the tests fail intermittently due to the padding issue.

I do not think you need to bother with adding a test that fails deterministically due the padding issue.

{
// Compare by ref for normal classes, by value for value types.

if (ReferenceEquals(o1, o2))
return true;

if (o1 is null || o2 is null)
return false;

MethodTable* pMT = GetMethodTable(o1);

// If it's not a value class, don't compare by value
if (!pMT->IsValueType)
return false;

// Make sure they are the same type.
if (pMT != GetMethodTable(o2))
return false;

// Compare the contents
uint size = pMT->GetNumInstanceFieldBytes();
return SpanHelpers.SequenceEqual(
ref GetRawData(o1),
ref GetRawData(o2),
size);
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 really not sure if this is GC safe for object field.

Copy link
Member

Choose a reason for hiding this comment

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

I presume it needs a nogc region for that

Copy link
Member

Choose a reason for hiding this comment

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

Right, this does not work. It needs to stay as FCall or QCall w/ SuppressGCTransition.

}

[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
public static int OffsetToStringData
Expand Down Expand Up @@ -199,8 +237,8 @@ public static object GetUninitializedObject(
[LibraryImport(QCall, EntryPoint = "ReflectionSerialization_GetUninitializedObject")]
private static partial void GetUninitializedObject(QCallTypeHandle type, ObjectHandleOnStack retObject);

[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern object AllocateUninitializedClone(object obj);
[LibraryImport(QCall, EntryPoint = "ObjectNative_AllocateUninitializedClone")]
internal static partial void AllocateUninitializedClone(ObjectHandleOnStack objHandle);

/// <returns>true if given type is reference type or value type that contains references</returns>
[Intrinsic]
Expand Down
186 changes: 33 additions & 153 deletions src/coreclr/classlibnative/bcltype/objectnative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,67 +20,19 @@
#include "eeconfig.h"


NOINLINE static INT32 GetHashCodeHelper(OBJECTREF objRef)
extern "C" INT32 QCALLTYPE ObjectNative_GetHashCodeHelper(QCall::ObjectHandleOnStack objHandle)
{
DWORD idx = 0;

FC_INNER_PROLOG(ObjectNative::GetHashCode);

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

idx = objRef->GetHashCodeEx();

HELPER_METHOD_FRAME_END();
FC_INNER_EPILOG();
return idx;
}

// 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) {

CONTRACTL
{
FCALL_CHECK;
INJECT_FAULT(FCThrow(kOutOfMemoryException););
}
CONTRACTL_END;

VALIDATEOBJECT(obj);
QCALL_CONTRACT;

if (obj == 0)
return 0;
INT32 ret = 0;

OBJECTREF objRef(obj);

{
DWORD bits = objRef->GetHeader()->GetBits();
BEGIN_QCALL;

if (bits & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX)
{
if (bits & BIT_SBLK_IS_HASHCODE)
{
// Common case: the object already has a hash code
return bits & MASK_HASHCODE;
}
else
{
// We have a sync block index. This means if we already have a hash code,
// it is in the sync block, otherwise we generate a new one and store it there
SyncBlock *psb = objRef->PassiveGetSyncBlock();
if (psb != NULL)
{
DWORD hashCode = psb->GetHashCode();
if (hashCode != 0)
return hashCode;
}
}
}
}
GCX_COOP();
ret = objHandle.Get()->GetHashCodeEx();

FC_INNER_RETURN(INT32, GetHashCodeHelper(objRef));
END_QCALL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
END_QCALL;
END_QCALL;
return ret;

}
FCIMPLEND

FCIMPL1(INT32, ObjectNative::TryGetHashCode, Object* obj) {

Expand Down Expand Up @@ -123,128 +75,56 @@ FCIMPL1(INT32, ObjectNative::TryGetHashCode, Object* obj) {
}
FCIMPLEND

//
// Compare by ref for normal classes, by value for value types.
//
// <TODO>@todo: it would be nice to customize this method based on the
// defining class rather than doing a runtime check whether it is
// a value type.</TODO>
//

FCIMPL2(FC_BOOL_RET, ObjectNative::Equals, Object *pThisRef, Object *pCompareRef)
{
CONTRACTL
{
FCALL_CHECK;
INJECT_FAULT(FCThrow(kOutOfMemoryException););
}
CONTRACTL_END;

if (pThisRef == pCompareRef)
FC_RETURN_BOOL(TRUE);

// Since we are in FCALL, we must handle NULL specially.
if (pThisRef == NULL || pCompareRef == NULL)
FC_RETURN_BOOL(FALSE);

MethodTable *pThisMT = pThisRef->GetMethodTable();

// If it's not a value class, don't compare by value
if (!pThisMT->IsValueType())
FC_RETURN_BOOL(FALSE);

// Make sure they are the same type.
if (pThisMT != pCompareRef->GetMethodTable())
FC_RETURN_BOOL(FALSE);

// Compare the contents (size - vtable - sync block index).
DWORD dwBaseSize = pThisMT->GetBaseSize();
if(pThisMT == g_pStringClass)
dwBaseSize -= sizeof(WCHAR);
BOOL ret = memcmp(
(void *) (pThisRef+1),
(void *) (pCompareRef+1),
dwBaseSize - sizeof(Object) - sizeof(int)) == 0;

FC_GC_POLL_RET();

FC_RETURN_BOOL(ret);
}
FCIMPLEND

NOINLINE static Object* GetClassHelper(OBJECTREF objRef)
extern "C" void QCALLTYPE ObjectNative_GetClassHelper(MethodTable* pMT, QCall::ObjectHandleOnStack ret)
{
FC_INNER_PROLOG(ObjectNative::GetClass);
_ASSERTE(objRef != NULL);
TypeHandle typeHandle = objRef->GetTypeHandle();
OBJECTREF refType = NULL;
QCALL_CONTRACT;

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

refType = typeHandle.GetManagedClassObject();
GCX_COOP();
ret.Set(pMT->GetManagedClassObject());

HELPER_METHOD_FRAME_END();
FC_INNER_EPILOG();
return OBJECTREFToObject(refType);
END_QCALL;
}

// This routine is called by the Object.GetType() routine. It is a major way to get the System.Type
FCIMPL1(Object*, ObjectNative::GetClass, Object* pThis)
FCIMPL1(Object*, ObjectNative::GetClassIfExists, MethodTable* pMT)
{
CONTRACTL
{
FCALL_CHECK;
INJECT_FAULT(FCThrow(kOutOfMemoryException););
}
CONTRACTL_END;

OBJECTREF objRef = ObjectToOBJECTREF(pThis);
if (objRef != NULL)
{
MethodTable* pMT = objRef->GetMethodTable();
OBJECTREF typePtr = pMT->GetManagedClassObjectIfExists();
if (typePtr != NULL)
{
return OBJECTREFToObject(typePtr);
}
}
else
FCThrow(kNullReferenceException);
FCALL_CONTRACT;

FC_INNER_RETURN(Object*, GetClassHelper(objRef));
OBJECTREF typePtr = pMT->GetManagedClassObjectIfExists();
return OBJECTREFToObject(typePtr);
}
FCIMPLEND

FCIMPL1(Object*, ObjectNative::AllocateUninitializedClone, Object* pObjUNSAFE)
extern "C" void QCALLTYPE ObjectNative_AllocateUninitializedClone(QCall::ObjectHandleOnStack objHandle)
{
FCALL_CONTRACT;

// Delegate error handling to managed side (it will throw NullReferenceException)
if (pObjUNSAFE == NULL)
return NULL;
QCALL_CONTRACT;

OBJECTREF refClone = ObjectToOBJECTREF(pObjUNSAFE);
_ASSERTE(objHandle.Get() != NULL); // Should be handled at managed side

HELPER_METHOD_FRAME_BEGIN_RET_1(refClone);
BEGIN_QCALL;

GCX_COOP();
OBJECTREF refClone = objHandle.Get();
MethodTable* pMT = refClone->GetMethodTable();

// assert that String has overloaded the Clone() method
_ASSERTE(pMT != g_pStringClass);

if (pMT->IsArray()) {
refClone = DupArrayForCloning((BASEARRAYREF)refClone);
} else {

if (pMT->IsArray())
{
objHandle.Set(DupArrayForCloning((BASEARRAYREF)refClone));
}
else
{
// We don't need to call the <cinit> because we know
// that it has been called....(It was called before this was created)
refClone = AllocateObject(pMT);
objHandle.Set(AllocateObject(pMT));
}

HELPER_METHOD_FRAME_END();

return OBJECTREFToObject(refClone);
END_QCALL;
}
FCIMPLEND

extern "C" BOOL QCALLTYPE Monitor_Wait(QCall::ObjectHandleOnStack pThis, INT32 Timeout)
{
Expand Down
6 changes: 2 additions & 4 deletions src/coreclr/classlibnative/bcltype/objectnative.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,12 @@ class ObjectNative
{
public:

static FCDECL1(INT32, GetHashCode, Object* vThisRef);
static FCDECL1(INT32, TryGetHashCode, Object* vThisRef);
static FCDECL2(FC_BOOL_RET, Equals, Object *pThisRef, Object *pCompareRef);
static FCDECL1(Object*, AllocateUninitializedClone, Object* pObjUNSAFE);
static FCDECL1(Object*, GetClass, Object* pThis);
static FCDECL1(FC_BOOL_RET, IsLockHeld, Object* pThisUNSAFE);
};

extern "C" INT32 QCALLTYPE ObjectNative_GetHashCodeHelper(QCall::ObjectHandleOnStack objHandle);
extern "C" void QCALLTYPE ObjectNative_AllocateUninitializedClone(QCall::ObjectHandleOnStack objHandle);
extern "C" BOOL QCALLTYPE Monitor_Wait(QCall::ObjectHandleOnStack pThis, INT32 Timeout);
extern "C" void QCALLTYPE Monitor_Pulse(QCall::ObjectHandleOnStack pThis);
extern "C" void QCALLTYPE Monitor_PulseAll(QCall::ObjectHandleOnStack pThis);
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/vm/ecalllist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,6 @@ FCFuncStart(gEnumFuncs)
FCFuncElement("InternalBoxEnum", ReflectionEnum::InternalBoxEnum)
FCFuncEnd()

FCFuncStart(gObjectFuncs)
FCFuncElement("GetType", ObjectNative::GetClass)
FCFuncEnd()

FCFuncStart(gStringFuncs)
FCDynamic("FastAllocateString", ECall::FastAllocateString)
FCDynamicSig(COR_CTOR_METHOD_NAME, &gsig_IM_ArrChar_RetVoid, ECall::CtorCharArrayManaged)
Expand Down Expand Up @@ -471,10 +467,7 @@ FCFuncStart(gRuntimeHelpers)
FCFuncElement("InitializeArray", ArrayNative::InitializeArray)
FCFuncElement("GetSpanDataFrom", ArrayNative::GetSpanDataFrom)
FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate)
FCFuncElement("GetHashCode", ObjectNative::GetHashCode)
FCFuncElement("TryGetHashCode", ObjectNative::TryGetHashCode)
FCFuncElement("Equals", ObjectNative::Equals)
FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone)
FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack)
FCFuncElement("TryEnsureSufficientExecutionStack", ReflectionInvocation::TryEnsureSufficientExecutionStack)
FCFuncElement("AllocTailCallArgBuffer", TailCallHelp::AllocTailCallArgBuffer)
Expand Down Expand Up @@ -642,7 +635,6 @@ FCClassElement("MngdSafeArrayMarshaler", "System.StubHelpers", gMngdSafeArrayMar
#endif // FEATURE_COMINTEROP
FCClassElement("ModuleHandle", "System", gCOMModuleHandleFuncs)
FCClassElement("Monitor", "System.Threading", gMonitorFuncs)
FCClassElement("Object", "System", gObjectFuncs)

FCClassElement("RuntimeAssembly", "System.Reflection", gRuntimeAssemblyFuncs)
FCClassElement("RuntimeFieldHandle", "System", gCOMFieldHandleNewFuncs)
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/qcallentrypoints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ static const Entry s_QCall[] =
DllImportEntry(GetFileLoadExceptionMessage)
DllImportEntry(FileLoadException_GetMessageForHR)
DllImportEntry(Interlocked_MemoryBarrierProcessWide)
DllImportEntry(ObjectNative_GetHashCodeHelper)
DllImportEntry(ObjectNative_AllocateUninitializedClone)
DllImportEntry(Monitor_Wait)
DllImportEntry(Monitor_Pulse)
DllImportEntry(Monitor_PulseAll)
Expand Down