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

Convert Array.IsSimpleCopy and CanAssignArray type to managed #104103

Merged
merged 15 commits into from
Jul 7, 2024
Merged

Convert Array.IsSimpleCopy and CanAssignArray type to managed #104103

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ea09fe2
Revert "Push back changes around IsSimpleCopy and CanAssignArrayType"
huoyaoyuan Jun 11, 2024
afd067b
Reduce use range of CorElementType
huoyaoyuan Jun 25, 2024
057c04a
Rearrange TypeDesc and TypeHandle usage
huoyaoyuan Jun 25, 2024
0770882
Fix
huoyaoyuan Jun 25, 2024
0cd6e54
Add test for pointer array
huoyaoyuan Jun 25, 2024
dd93e95
Rearrange test
huoyaoyuan Jun 28, 2024
423c94b
Remove comment in test
huoyaoyuan Jun 29, 2024
0ef5370
NativeAot compat
huoyaoyuan Jun 29, 2024
4b6d830
Mono compat
huoyaoyuan Jun 29, 2024
f8b8355
Disable test on mono
huoyaoyuan Jun 29, 2024
5f8783b
Try fix nativeaot
huoyaoyuan Jun 29, 2024
88beba7
Merge branch 'main' into array-assign-type
huoyaoyuan Jul 6, 2024
f95296f
Don't lookup cast cache in QCall
huoyaoyuan Jul 6, 2024
d48f6f4
Fix GC mode
huoyaoyuan Jul 7, 2024
4db934b
Update QCall name
huoyaoyuan Jul 7, 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
129 changes: 105 additions & 24 deletions src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs
View file
Open in desktop
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
19 changes: 18 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 @@ -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,23 @@ 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)
{
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 •
edited
Loading

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 •
edited
Loading

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")]
[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
View file
Open in desktop
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
4 changes: 0 additions & 4 deletions src/coreclr/classlibnative/bcltype/arraynative.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,15 @@
#ifndef _ARRAYNATIVE_H_
#define _ARRAYNATIVE_H_

#include "fcall.h"
#include "qcall.h"

class ArrayNative
{
public:
static FCDECL1(INT32, GetCorElementTypeOfElementType, ArrayBase* arrayUNSAFE);

static FCDECL2(FC_BOOL_RET, IsSimpleCopy, ArrayBase* pSrc, ArrayBase* pDst);
};

extern "C" void QCALLTYPE Array_CreateInstance(QCall::TypeHandle pTypeHnd, INT32 rank, INT32* pLengths, INT32* pBounds, BOOL createFromArrayType, QCall::ObjectHandleOnStack retArray);
extern "C" PCODE QCALLTYPE Array_GetElementConstructorEntrypoint(QCall::TypeHandle pArrayTypeHnd);
extern "C" INT32 QCALLTYPE Array_CanAssignArrayType(void* srcTH, void* destTH);

#endif // _ARRAYNATIVE_H_
Loading
Loading