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

Simplify non-generic ArrayEnumerator #51351

Merged
merged 4 commits into from
Apr 21, 2021
Merged

Simplify non-generic ArrayEnumerator #51351

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0dc940a
Simplify non-generic ArrayEnumerator
jkotas Apr 13, 2021
bc399e1
Implement GetFlattenedIndex for Mono
jkotas Apr 16, 2021
b169532
Feedback
jkotas Apr 20, 2021
7893de1
Fix test failures
jkotas Apr 20, 2021
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
133 changes: 27 additions & 106 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 @@ -303,119 +303,40 @@ public static unsafe void Clear(Array array, int index, int length)
// GC.KeepAlive(array) not required. pMT kept alive via `ptr`
}

// The various Get values...
public unsafe object? GetValue(params int[] indices)
private unsafe nint GetFlattenedIndex(ReadOnlySpan<int> indices)
{
if (indices == null)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.indices);
if (Rank != indices.Length)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankIndices);

TypedReference elemref = default;
fixed (int* pIndices = &indices[0])
InternalGetReference(&elemref, indices.Length, pIndices);
return TypedReference.InternalToObject(&elemref);
}

public unsafe object? GetValue(int index)
{
if (Rank != 1)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray);

TypedReference elemref = default;
InternalGetReference(&elemref, 1, &index);
return TypedReference.InternalToObject(&elemref);
}

public unsafe object? GetValue(int index1, int index2)
{
if (Rank != 2)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray);

int* pIndices = stackalloc int[2];
pIndices[0] = index1;
pIndices[1] = index2;

TypedReference elemref = default;
InternalGetReference(&elemref, 2, pIndices);
return TypedReference.InternalToObject(&elemref);
}

public unsafe object? GetValue(int index1, int index2, int index3)
{
if (Rank != 3)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray);

int* pIndices = stackalloc int[3];
pIndices[0] = index1;
pIndices[1] = index2;
pIndices[2] = index3;

TypedReference elemref = default;
InternalGetReference(&elemref, 3, pIndices);
return TypedReference.InternalToObject(&elemref);
}

public unsafe void SetValue(object? value, int index)
{
if (Rank != 1)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray);

TypedReference elemref = default;
InternalGetReference(&elemref, 1, &index);
InternalSetValue(&elemref, value);
}
// Checked by the caller
Debug.Assert(indices.Length == Rank);

public unsafe void SetValue(object? value, int index1, int index2)
{
if (Rank != 2)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray);

int* pIndices = stackalloc int[2];
pIndices[0] = index1;
pIndices[1] = index2;

TypedReference elemref = default;
InternalGetReference(&elemref, 2, pIndices);
InternalSetValue(&elemref, value);
}

public unsafe void SetValue(object? value, int index1, int index2, int index3)
{
if (Rank != 3)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray);

int* pIndices = stackalloc int[3];
pIndices[0] = index1;
pIndices[1] = index2;
pIndices[2] = index3;

TypedReference elemref = default;
InternalGetReference(&elemref, 3, pIndices);
InternalSetValue(&elemref, value);
}

public unsafe void SetValue(object? value, params int[] indices)
{
if (indices == null)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.indices);
if (Rank != indices.Length)
ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankIndices);

TypedReference elemref = default;
fixed (int* pIndices = &indices[0])
InternalGetReference(&elemref, indices.Length, pIndices);
InternalSetValue(&elemref, value);
if (RuntimeHelpers.GetMethodTable(this)->IsMultiDimensionalArray)
{
ref int bounds = ref RuntimeHelpers.GetMultiDimensionalArrayBounds(this);
nint flattenedIndex = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think nuint makes a bit more sense than nint since we're talking about absolute offsets, and absolute offsets can never be negative. But if the convention is to use nint, so be it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I need signed int for the iterator. So this really becomes a question of where you choose to place the casts.

I prefer signed int in situations like this, both in C/C++ and C#. There has been a lot of written about signed vs. unsigned. For example, https://google.github.io/styleguide/cppguide.html#Integer_Types that is aligned with my thinking on this.

for (int i = 0; i < indices.Length; i++)
{
int index = indices[i] - Unsafe.Add(ref bounds, indices.Length + i);
int length = Unsafe.Add(ref bounds, i);
if ((uint)index >= (uint)length)
ThrowHelper.ThrowIndexOutOfRangeException();
flattenedIndex = (length * flattenedIndex) + index;
}
Debug.Assert((nuint)flattenedIndex < (nuint)LongLength);
return flattenedIndex;
}
else
{
int index = indices[0];
if ((uint)index >= (uint)LongLength)
ThrowHelper.ThrowIndexOutOfRangeException();
return index;
}
}

[MethodImpl(MethodImplOptions.InternalCall)]
// reference to TypedReference is banned, so have to pass result as pointer
private extern unsafe void InternalGetReference(void* elemRef, int rank, int* pIndices);
internal extern unsafe object? InternalGetValue(nint flattenedIndex);

// Ideally, we would like to use TypedReference.SetValue instead. Unfortunately, TypedReference.SetValue
// always throws not-supported exception
[MethodImpl(MethodImplOptions.InternalCall)]
private static extern unsafe void InternalSetValue(void* target, object? value);
private extern void InternalSetValue(object? value, nint flattenedIndex);

public int Length => checked((int)Unsafe.As<RawArrayData>(this).Length);

Expand Down
116 changes: 51 additions & 65 deletions src/coreclr/classlibnative/bcltype/arraynative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,54 +954,50 @@ Done: ;
}
FCIMPLEND


FCIMPL4(void, ArrayNative::GetReference, ArrayBase* refThisUNSAFE, TypedByRef* elemRef, INT32 rank, INT32* pIndices)
FCIMPL2(Object*, ArrayNative::GetValue, ArrayBase* refThisUNSAFE, INT_PTR flattenedIndex)
{
CONTRACTL {
FCALL_CHECK;
PRECONDITION(rank >= 0);
} CONTRACTL_END;

// FC_GC_POLL not necessary. We poll for GC in Array.Rank that's always called
// right before this function
FC_GC_POLL_NOT_NEEDED();
BASEARRAYREF refThis(refThisUNSAFE);

BASEARRAYREF refThis = (BASEARRAYREF) refThisUNSAFE;
TypeHandle arrayElementType = refThis->GetArrayElementTypeHandle();

_ASSERTE(rank == (INT32)refThis->GetRank());
// Legacy behavior
if (arrayElementType.IsTypeDesc())
{
CorElementType elemtype = arrayElementType.AsTypeDesc()->GetInternalCorElementType();
if (elemtype == ELEMENT_TYPE_PTR || elemtype == ELEMENT_TYPE_FNPTR)
FCThrowRes(kNotSupportedException, W("NotSupported_Type"));
}

SIZE_T Offset = 0;
const INT32 *pBoundsPtr = refThis->GetBoundsPtr();
_ASSERTE((SIZE_T)flattenedIndex < refThis->GetNumComponents());
void* pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize();
OBJECTREF Obj;

if (rank == 1)
MethodTable* pElementTypeMT = arrayElementType.GetMethodTable();
if (pElementTypeMT->IsValueType())
{
Offset = pIndices[0] - refThis->GetLowerBoundsPtr()[0];

// Bounds check each index
// Casting to unsigned allows us to use one compare for [0..limit-1]
if (((UINT32) Offset) >= ((UINT32) pBoundsPtr[0]))
FCThrowVoid(kIndexOutOfRangeException);
HELPER_METHOD_FRAME_BEGIN_RET_0();
Obj = pElementTypeMT->Box(pData);
HELPER_METHOD_FRAME_END();
}
else
{
// Avoid redundant computation in GetLowerBoundsPtr
const INT32 *pLowerBoundsPtr = pBoundsPtr + rank;
_ASSERTE(refThis->GetLowerBoundsPtr() == pLowerBoundsPtr);

SIZE_T Multiplier = 1;
Obj = ObjectToOBJECTREF(*((Object**)pData));
}

for (int i = rank; i >= 1; i--) {
INT32 curIndex = pIndices[i-1] - pLowerBoundsPtr[i-1];
return OBJECTREFToObject(Obj);
}
FCIMPLEND

// Bounds check each index
// Casting to unsigned allows us to use one compare for [0..limit-1]
if (((UINT32) curIndex) >= ((UINT32) pBoundsPtr[i-1]))
FCThrowVoid(kIndexOutOfRangeException);
FCIMPL3(void, ArrayNative::SetValue, ArrayBase* refThisUNSAFE, Object* objUNSAFE, INT_PTR flattenedIndex)
{
FCALL_CONTRACT;

Offset += curIndex * Multiplier;
Multiplier *= pBoundsPtr[i-1];
}
}
BASEARRAYREF refThis(refThisUNSAFE);
OBJECTREF obj(objUNSAFE);

TypeHandle arrayElementType = refThis->GetArrayElementTypeHandle();

Expand All @@ -1012,81 +1008,71 @@ FCIMPL4(void, ArrayNative::GetReference, ArrayBase* refThisUNSAFE, TypedByRef* e
if (elemtype == ELEMENT_TYPE_PTR || elemtype == ELEMENT_TYPE_FNPTR)
FCThrowResVoid(kNotSupportedException, W("NotSupported_Type"));
}
#ifdef _DEBUG
CorElementType elemtype = arrayElementType.GetInternalCorElementType();
_ASSERTE(elemtype != ELEMENT_TYPE_PTR && elemtype != ELEMENT_TYPE_FNPTR);
#endif

elemRef->data = refThis->GetDataPtr() + (Offset * refThis->GetComponentSize());
elemRef->type = arrayElementType;
}
FCIMPLEND

FCIMPL2(void, ArrayNative::SetValue, TypedByRef * target, Object* objUNSAFE)
{
FCALL_CONTRACT;

OBJECTREF obj = ObjectToOBJECTREF(objUNSAFE);
_ASSERTE((SIZE_T)flattenedIndex < refThis->GetNumComponents());

TypeHandle thTarget(target->type);
MethodTable* pElementTypeMT = arrayElementType.GetMethodTable();
PREFIX_ASSUME(NULL != pElementTypeMT);

MethodTable* pTargetMT = thTarget.AsMethodTable();
PREFIX_ASSUME(NULL != pTargetMT);
void* pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize();

if (obj == NULL)
{
// Null is the universal zero...
if (pTargetMT->IsValueType())
InitValueClass(target->data,pTargetMT);
if (pElementTypeMT->IsValueType())
InitValueClass(pData,pElementTypeMT);
else
ClearObjectReference((OBJECTREF*)target->data);
ClearObjectReference((OBJECTREF*)pData);
}
else
if (thTarget == TypeHandle(g_pObjectClass))
if (arrayElementType == TypeHandle(g_pObjectClass))
{
// Everything is compatible with Object
SetObjectReference((OBJECTREF*)target->data,(OBJECTREF)obj);
SetObjectReference((OBJECTREF*)pData,(OBJECTREF)obj);
}
else
if (!pTargetMT->IsValueType())
if (!pElementTypeMT->IsValueType())
{
if (ObjIsInstanceOfCached(OBJECTREFToObject(obj), thTarget) != TypeHandle::CanCast)
if (ObjIsInstanceOfCached(OBJECTREFToObject(obj), arrayElementType) != TypeHandle::CanCast)
{
// target->data is protected by the caller
HELPER_METHOD_FRAME_BEGIN_1(obj);
HELPER_METHOD_FRAME_BEGIN_2(refThis, obj);

if (!ObjIsInstanceOf(OBJECTREFToObject(obj), thTarget))
if (!ObjIsInstanceOf(OBJECTREFToObject(obj), arrayElementType))
COMPlusThrow(kInvalidCastException,W("InvalidCast_StoreArrayElement"));

HELPER_METHOD_FRAME_END();
}

SetObjectReference((OBJECTREF*)target->data,obj);
// Refresh pData in case GC moved objects around
pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize();
jkotas marked this conversation as resolved.
Show resolved Hide resolved
SetObjectReference((OBJECTREF*)pData,obj);
}
else
{
// value class or primitive type

if (!pTargetMT->UnBoxInto(target->data, obj))
if (!pElementTypeMT->UnBoxInto(pData, obj))
{
// target->data is protected by the caller
HELPER_METHOD_FRAME_BEGIN_1(obj);
HELPER_METHOD_FRAME_BEGIN_2(refThis, obj);

ARG_SLOT value = 0;

// Allow enum -> primitive conversion, disallow primitive -> enum conversion
TypeHandle thSrc = obj->GetTypeHandle();
CorElementType srcType = thSrc.GetVerifierCorElementType();
CorElementType targetType = thTarget.GetSignatureCorElementType();
CorElementType targetType = arrayElementType.GetSignatureCorElementType();

if (!InvokeUtil::IsPrimitiveType(srcType) || !InvokeUtil::IsPrimitiveType(targetType))
COMPlusThrow(kInvalidCastException, W("InvalidCast_StoreArrayElement"));

// Get a properly widened type
InvokeUtil::CreatePrimitiveValue(targetType,srcType,obj,&value);

// Refresh pData in case GC moved objects around
pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize();

UINT cbSize = CorTypeInfo::Size(targetType);
memcpyNoGCRefs(target->data, ArgSlotEndianessFixup(&value, cbSize), cbSize);
memcpyNoGCRefs(pData, ArgSlotEndianessFixup(&value, cbSize), cbSize);

HELPER_METHOD_FRAME_END();
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/classlibnative/bcltype/arraynative.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ class ArrayNative
static FCDECL4(Object*, CreateInstance, void* elementTypeHandle, INT32 rank, INT32* pLengths, INT32* pBounds);

// This method will return a TypedReference to the array element
static FCDECL4(void, GetReference, ArrayBase* refThisUNSAFE, TypedByRef* elemRef, INT32 rank, INT32* pIndices);
static FCDECL2(Object*, GetValue, ArrayBase* refThisUNSAFE, INT_PTR flattenedIndex);

// This set of methods will set a value in an array
static FCDECL2(void, SetValue, TypedByRef* target, Object* objUNSAFE);
static FCDECL3(void, SetValue, ArrayBase* refThisUNSAFE, Object* objUNSAFE, INT_PTR flattenedIndex);

// This method will initialize an array from a TypeHandle
// to a field.
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 @@ -686,7 +686,7 @@ FCFuncStart(gArrayFuncs)
FCFuncElement("IsSimpleCopy", ArrayNative::IsSimpleCopy)
FCFuncElement("CopySlow", ArrayNative::CopySlow)
FCFuncElement("InternalCreate", ArrayNative::CreateInstance)
FCFuncElement("InternalGetReference", ArrayNative::GetReference)
FCFuncElement("InternalGetValue", ArrayNative::GetValue)
FCFuncElement("InternalSetValue", ArrayNative::SetValue)
FCFuncEnd()

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\ArgumentOutOfRangeException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\ArithmeticException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Array.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Array.Enumerators.cs" Condition="'$(TargetsCoreRT)' != 'true'" />
<Compile Include="$(MSBuildThisFileDirectory)System\Array.Enumerators.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\ArraySegment.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\ArrayTypeMismatchException.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\AssemblyLoadEventArgs.cs" />
Expand Down
Loading