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 Array.IsSimpleCopy and CanAssignArray type to managed #104103

Merged
merged 15 commits into from
Jul 7, 2024
129 changes: 105 additions & 24 deletions src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ private static unsafe void CopyImpl(Array sourceArray, int sourceIndex, Array de
if ((uint)(destinationIndex + length) > destinationArray.NativeLength)
throw new ArgumentException(SR.Arg_LongerThanDestArray, nameof(destinationArray));

if (sourceArray.GetType() == destinationArray.GetType() || IsSimpleCopy(sourceArray, destinationArray))
ArrayAssignType assignType = ArrayAssignType.WrongType;

if (sourceArray.GetType() == destinationArray.GetType()
|| (assignType = CanAssignArrayType(sourceArray, destinationArray)) == ArrayAssignType.SimpleCopy)
{
MethodTable* pMT = RuntimeHelpers.GetMethodTable(sourceArray);

Expand All @@ -86,44 +89,50 @@ private static unsafe void CopyImpl(Array sourceArray, int sourceIndex, Array de
throw new ArrayTypeMismatchException(SR.ArrayTypeMismatch_ConstrainedCopy);

// Rare
CopySlow(sourceArray, sourceIndex, destinationArray, destinationIndex, length);
CopySlow(sourceArray, sourceIndex, destinationArray, destinationIndex, length, assignType);
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern bool IsSimpleCopy(Array sourceArray, Array destinationArray);
private static CorElementType GetNormalizedIntegralArrayElementType(CorElementType elementType)
{
Debug.Assert(elementType.IsPrimitiveType());

// Array Primitive types such as E_T_I4 and E_T_U4 are interchangeable
// Enums with interchangeable underlying types are interchangeable
// BOOL is NOT interchangeable with I1/U1, neither CHAR -- with I2/U2

// U1/U2/U4/U8/U
int shift = (0b0010_0000_0000_0000_1010_1010_0000 >> (int)elementType) & 1;
return (CorElementType)((int)elementType - shift);
}

// Reliability-wise, this method will either possibly corrupt your
// instance & might fail when called from within a CER, or if the
// reliable flag is true, it will either always succeed or always
// throw an exception with no side effects.
private static unsafe void CopySlow(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length)
private static unsafe void CopySlow(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length, ArrayAssignType assignType)
{
Debug.Assert(sourceArray.Rank == destinationArray.Rank);

void* srcTH = RuntimeHelpers.GetMethodTable(sourceArray)->ElementType;
void* destTH = RuntimeHelpers.GetMethodTable(destinationArray)->ElementType;
AssignArrayEnum r = CanAssignArrayType(srcTH, destTH);

if (r == AssignArrayEnum.AssignWrongType)
if (assignType == ArrayAssignType.WrongType)
throw new ArrayTypeMismatchException(SR.ArrayTypeMismatch_CantAssignType);

if (length > 0)
{
switch (r)
switch (assignType)
{
case AssignArrayEnum.AssignUnboxValueClass:
case ArrayAssignType.UnboxValueClass:
CopyImplUnBoxEachElement(sourceArray, sourceIndex, destinationArray, destinationIndex, length);
break;

case AssignArrayEnum.AssignBoxValueClassOrPrimitive:
case ArrayAssignType.BoxValueClassOrPrimitive:
CopyImplBoxEachElement(sourceArray, sourceIndex, destinationArray, destinationIndex, length);
break;

case AssignArrayEnum.AssignMustCast:
case ArrayAssignType.MustCast:
CopyImplCastCheckEachElement(sourceArray, sourceIndex, destinationArray, destinationIndex, length);
break;

case AssignArrayEnum.AssignPrimitiveWiden:
case ArrayAssignType.PrimitiveWiden:
CopyImplPrimitiveWiden(sourceArray, sourceIndex, destinationArray, destinationIndex, length);
break;

Expand All @@ -134,18 +143,90 @@ private static unsafe void CopySlow(Array sourceArray, int sourceIndex, Array de
}
}

// Must match the definition in arraynative.cpp
private enum AssignArrayEnum
private enum ArrayAssignType
{
AssignWrongType,
AssignMustCast,
AssignBoxValueClassOrPrimitive,
AssignUnboxValueClass,
AssignPrimitiveWiden,
SimpleCopy,
WrongType,
MustCast,
BoxValueClassOrPrimitive,
UnboxValueClass,
PrimitiveWiden,
}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Array_CanAssignArrayType")]
private static unsafe partial AssignArrayEnum CanAssignArrayType(void* srcTH, void* dstTH);
private static unsafe ArrayAssignType CanAssignArrayType(Array sourceArray, Array destinationArray)
{
TypeHandle srcTH = RuntimeHelpers.GetMethodTable(sourceArray)->GetArrayElementTypeHandle();
TypeHandle destTH = RuntimeHelpers.GetMethodTable(destinationArray)->GetArrayElementTypeHandle();

if (TypeHandle.AreSameType(srcTH, destTH)) // This check kicks for different array kind or dimensions
return ArrayAssignType.SimpleCopy;

if (!srcTH.IsTypeDesc && !destTH.IsTypeDesc)
{
MethodTable* pMTsrc = srcTH.AsMethodTable();
MethodTable* pMTdest = destTH.AsMethodTable();

// Value class boxing
if (pMTsrc->IsValueType && !pMTdest->IsValueType)
{
if (srcTH.CanCastTo(destTH))
return ArrayAssignType.BoxValueClassOrPrimitive;
else
return ArrayAssignType.WrongType;
}

// Value class unboxing.
if (!pMTsrc->IsValueType && pMTdest->IsValueType)
{
if (srcTH.CanCastTo(destTH))
return ArrayAssignType.UnboxValueClass;
else if (destTH.CanCastTo(srcTH)) // V extends IV. Copying from IV to V, or Object to V.
return ArrayAssignType.UnboxValueClass;
else
return ArrayAssignType.WrongType;
}

// Copying primitives from one type to another
if (pMTsrc->IsPrimitive && pMTdest->IsPrimitive)
{
CorElementType srcElType = pMTsrc->GetPrimitiveCorElementType();
CorElementType destElType = pMTdest->GetPrimitiveCorElementType();

if (GetNormalizedIntegralArrayElementType(srcElType) == GetNormalizedIntegralArrayElementType(destElType))
return ArrayAssignType.SimpleCopy;
else if (RuntimeHelpers.CanPrimitiveWiden(srcElType, destElType))
return ArrayAssignType.PrimitiveWiden;
else
return ArrayAssignType.WrongType;
}

// src Object extends dest
if (srcTH.CanCastTo(destTH))
return ArrayAssignType.SimpleCopy;

// dest Object extends src
if (destTH.CanCastTo(srcTH))
return ArrayAssignType.MustCast;

// class X extends/implements src and implements dest.
if (pMTdest->IsInterface)
return ArrayAssignType.MustCast;

// class X implements src and extends/implements dest
if (pMTsrc->IsInterface)
return ArrayAssignType.MustCast;
}
else
{
// Only pointers are valid for TypeDesc in array element

// Compatible pointers
if (srcTH.CanCastTo(destTH))
return ArrayAssignType.SimpleCopy;
}
Comment on lines +220 to +226
Copy link
Member Author

Choose a reason for hiding this comment

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

If any pointer type goes through the non-simple copy paths, it will result in the same fatal crash.


return ArrayAssignType.WrongType;
}

// Unboxes from an Object[] into a value class or primitive array.
private static unsafe void CopyImplUnBoxEachElement(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ public bool CanCompareBitsOrUseFastGetHashCode
/// <summary>
/// A type handle, which can wrap either a pointer to a <c>TypeDesc</c> or to a <see cref="MethodTable"/>.
/// </summary>
internal unsafe struct TypeHandle
internal readonly unsafe partial struct TypeHandle
{
// Subset of src\vm\typehandle.h

Expand Down Expand Up @@ -865,6 +865,29 @@ public static TypeHandle TypeHandleOf<T>()
{
return new TypeHandle((void*)RuntimeTypeHandle.ToIntPtr(typeof(T).TypeHandle));
}

public static bool AreSameType(TypeHandle left, TypeHandle right) => left.m_asTAddr == right.m_asTAddr;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool CanCastTo(TypeHandle destTH)
{
if (m_asTAddr == destTH.m_asTAddr)
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering whether this logic should live in CastHelpers.cs next to all other casting logic, so that it is not missed if there are any bug fixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

They look quite inconsistent...Methods in CastHelpers are HCalls in jithelpers.
BTW does it make sense to convert such methods to QCall, and move out of jithelpers since they are not directly used by JIT?

Copy link
Member

@jkotas jkotas Jul 7, 2024

Choose a reason for hiding this comment

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

does it make sense to convert such methods to QCall

Yes. We want to get rid of all FCalls with HELPER_METHOD_FRAME.

not directly used by JIT

Those are slow path for helpers used by the JIT. Fast paths of those helpers are either in assembly code or in C#.

The split between JIT helpers and other helpers is blurry. It is not unusual for the two to have overlapping logic. We even have methods that are used as JIT helpers, but they are used for other purposes as well. I do not have strong opinions about the best source file split. Anything we come up with will have some downsides.

This can be worked on in a follow up.


if (!IsTypeDesc && destTH.IsTypeDesc)
return false;

CastResult result = CastCache.TryGet(CastHelpers.s_table!, (nuint)m_asTAddr, (nuint)destTH.m_asTAddr);

if (result != CastResult.MaybeCast)
return result == CastResult.CanCast;

return CanCastTo_NoCacheLookup(m_asTAddr, destTH.m_asTAddr);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The first thing that the QCall is going to do is repeat the cache lookup...

Copy link
Member

@jkotas jkotas Jun 28, 2024

Choose a reason for hiding this comment

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

Also, the unmanaged TypeHandle.CanCastTo assumes that some cases are very cheap to check for and it does not bother to add them to the cache. These cases will be much slower here since we are always going to take the QCall transition for them. We should either check for them here and/or add them to cache (similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons).

It probably does not matter for the one caller added in this PR, but it may matter for future callers.

Copy link
Member Author

Choose a reason for hiding this comment

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

similar to how RuntimeTypeHandle::CanCastTo adds them to the cache for the same reasons

The methods should probably be combined at managed side. They are doing the exact same things.

Copy link
Member

Choose a reason for hiding this comment

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

Yes (it is fine to do it in a follow up PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

They are doing the exact same things.

In fact they aren't. RuntimeTypeHandle::CanCastTo allows T -> Nullable<T>.

The non-cached cases include nullables, COM and I(Dynamic)Castable interfaces. I don't think specially handling them is worthy, even for future callers.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you would either need to have a bool flag that controls the special handling or introduce two QCalls with similar implementation.

Copy link
Member

@jkotas jkotas Jun 29, 2024

Choose a reason for hiding this comment

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

Do you plan to do something about this one? (Unifying with RuntimeTypeHandle::CanCastTo should be separate PR, fixing CanCastTo_NoCacheLookup to avoid unnecessary cache lookup should be in this one.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, together with some other cases sharable between RuntimeTypeHandle and MethodTable, like type equivalence.

}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "TypeHandle_CanCastTo_NoCacheLookup")]
[return: MarshalAs(UnmanagedType.Bool)]
private static partial bool CanCastTo_NoCacheLookup(void* fromTypeHnd, void* toTypeHnd);
}

// Helper structs used for tail calls via helper.
Expand Down
160 changes: 0 additions & 160 deletions src/coreclr/classlibnative/bcltype/arraynative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,166 +47,6 @@ extern "C" PCODE QCALLTYPE Array_GetElementConstructorEntrypoint(QCall::TypeHand
return ctorEntrypoint;
}

// Returns whether you can directly copy an array of srcType into destType.
FCIMPL2(FC_BOOL_RET, ArrayNative::IsSimpleCopy, ArrayBase* pSrc, ArrayBase* pDst)
{
FCALL_CONTRACT;

_ASSERTE(pSrc != NULL);
_ASSERTE(pDst != NULL);

// This case is expected to be handled by the fast path
_ASSERTE(pSrc->GetMethodTable() != pDst->GetMethodTable());

TypeHandle srcTH = pSrc->GetMethodTable()->GetArrayElementTypeHandle();
TypeHandle destTH = pDst->GetMethodTable()->GetArrayElementTypeHandle();
if (srcTH == destTH) // This check kicks for different array kind or dimensions
FC_RETURN_BOOL(true);

if (srcTH.IsValueType())
{
// Value class boxing
if (!destTH.IsValueType())
FC_RETURN_BOOL(false);

const CorElementType srcElType = srcTH.GetVerifierCorElementType();
const CorElementType destElType = destTH.GetVerifierCorElementType();
_ASSERTE(srcElType < ELEMENT_TYPE_MAX);
_ASSERTE(destElType < ELEMENT_TYPE_MAX);

// Copying primitives from one type to another
if (CorTypeInfo::IsPrimitiveType_NoThrow(srcElType) && CorTypeInfo::IsPrimitiveType_NoThrow(destElType))
{
if (GetNormalizedIntegralArrayElementType(srcElType) == GetNormalizedIntegralArrayElementType(destElType))
FC_RETURN_BOOL(true);
}
}
else
{
// Value class unboxing
if (destTH.IsValueType())
FC_RETURN_BOOL(false);
}

TypeHandle::CastResult r = srcTH.CanCastToCached(destTH);
if (r != TypeHandle::MaybeCast)
{
FC_RETURN_BOOL(r);
}

struct
{
OBJECTREF src;
OBJECTREF dst;
} gc;

gc.src = ObjectToOBJECTREF(pSrc);
gc.dst = ObjectToOBJECTREF(pDst);

BOOL iRetVal = FALSE;

HELPER_METHOD_FRAME_BEGIN_RET_PROTECT(gc);
iRetVal = srcTH.CanCastTo(destTH);
HELPER_METHOD_FRAME_END();

FC_RETURN_BOOL(iRetVal);
}
FCIMPLEND


// Return values for CanAssignArrayType
enum AssignArrayEnum
{
AssignWrongType,
AssignMustCast,
AssignBoxValueClassOrPrimitive,
AssignUnboxValueClass,
AssignPrimitiveWiden,
};

// Returns an enum saying whether you can copy an array of srcType into destType.
static AssignArrayEnum CanAssignArrayType(const TypeHandle srcTH, const TypeHandle destTH)
{
CONTRACTL
{
THROWS;
GC_TRIGGERS;
PRECONDITION(!srcTH.IsNull());
PRECONDITION(!destTH.IsNull());
}
CONTRACTL_END;

_ASSERTE(srcTH != destTH); // Handled by fast path

// Value class boxing
if (srcTH.IsValueType() && !destTH.IsValueType())
{
if (srcTH.CanCastTo(destTH))
return AssignBoxValueClassOrPrimitive;
else
return AssignWrongType;
}

// Value class unboxing.
if (!srcTH.IsValueType() && destTH.IsValueType())
{
if (srcTH.CanCastTo(destTH))
return AssignUnboxValueClass;
else if (destTH.CanCastTo(srcTH)) // V extends IV. Copying from IV to V, or Object to V.
return AssignUnboxValueClass;
else
return AssignWrongType;
}

const CorElementType srcElType = srcTH.GetVerifierCorElementType();
const CorElementType destElType = destTH.GetVerifierCorElementType();
_ASSERTE(srcElType < ELEMENT_TYPE_MAX);
_ASSERTE(destElType < ELEMENT_TYPE_MAX);

// Copying primitives from one type to another
if (CorTypeInfo::IsPrimitiveType_NoThrow(srcElType) && CorTypeInfo::IsPrimitiveType_NoThrow(destElType))
{
_ASSERTE(srcElType != destElType); // Handled by fast path
if (InvokeUtil::CanPrimitiveWiden(destElType, srcElType))
return AssignPrimitiveWiden;
else
return AssignWrongType;
}

// dest Object extends src
_ASSERTE(!srcTH.CanCastTo(destTH)); // Handled by fast path

// src Object extends dest
if (destTH.CanCastTo(srcTH))
return AssignMustCast;

// class X extends/implements src and implements dest.
if (destTH.IsInterface() && srcElType != ELEMENT_TYPE_VALUETYPE)
return AssignMustCast;

// class X implements src and extends/implements dest
if (srcTH.IsInterface() && destElType != ELEMENT_TYPE_VALUETYPE)
return AssignMustCast;

return AssignWrongType;
}

extern "C" int QCALLTYPE Array_CanAssignArrayType(void* srcTH, void* destTH)
{
QCALL_CONTRACT;

INT32 ret = 0;

BEGIN_QCALL;

ret = CanAssignArrayType(TypeHandle::FromPtr(srcTH), TypeHandle::FromPtr(destTH));

END_QCALL;

return ret;
}


//
// This is a GC safe variant of the memmove intrinsic. It sets the cards, and guarantees that the object references in the GC heap are
// updated atomically.
Expand Down
Loading
Loading