From 0dc940abe91c747c428113c72b2a84c96b4e209e Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 13 Apr 2021 09:48:28 -0700 Subject: [PATCH 1/4] Simplify non-generic ArrayEnumerator --- .../src/System/Array.CoreCLR.cs | 133 ++++-------------- .../classlibnative/bcltype/arraynative.cpp | 116 +++++++-------- .../classlibnative/bcltype/arraynative.h | 4 +- src/coreclr/vm/ecalllist.h | 2 +- .../System.Private.CoreLib.Shared.projitems | 2 +- .../src/System/Array.Enumerators.cs | 118 ++-------------- .../src/System/Array.cs | 79 ++++++++++- .../src/System/Array.Mono.cs | 9 ++ 8 files changed, 172 insertions(+), 291 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs index e661ca109c0d1..633b1544582d3 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs @@ -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 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; + 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(this).Length); diff --git a/src/coreclr/classlibnative/bcltype/arraynative.cpp b/src/coreclr/classlibnative/bcltype/arraynative.cpp index 50e652b5b66e5..c7a3a419b03eb 100644 --- a/src/coreclr/classlibnative/bcltype/arraynative.cpp +++ b/src/coreclr/classlibnative/bcltype/arraynative.cpp @@ -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(); @@ -1012,72 +1008,59 @@ 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(); + 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")); @@ -1085,8 +1068,11 @@ FCIMPL2(void, ArrayNative::SetValue, TypedByRef * target, Object* objUNSAFE) // 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(); } diff --git a/src/coreclr/classlibnative/bcltype/arraynative.h b/src/coreclr/classlibnative/bcltype/arraynative.h index c5be8403a8ef0..af926d8e42458 100644 --- a/src/coreclr/classlibnative/bcltype/arraynative.h +++ b/src/coreclr/classlibnative/bcltype/arraynative.h @@ -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. diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 9b84c7a8dd6b1..738e68557771f 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -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() diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 0246e7a0e3e4d..0ce2c27dde1aa 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -75,7 +75,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 9d886312c8e7c..ccf1af152e952 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -8,116 +8,14 @@ namespace System { internal sealed class ArrayEnumerator : IEnumerator, ICloneable - { - private readonly Array array; - private int index; - private readonly int endIndex; - private readonly int startIndex; // Save for Reset. - private readonly int[] _indices; // The current position in a multidim array - private bool _complete; - - internal ArrayEnumerator(Array array, int index, int count) - { - this.array = array; - this.index = index - 1; - startIndex = index; - endIndex = index + count; - _indices = new int[array.Rank]; - int checkForZero = 1; // Check for dimensions of size 0. - for (int i = 0; i < array.Rank; i++) - { - _indices[i] = array.GetLowerBound(i); - checkForZero *= array.GetLength(i); - } - // To make MoveNext simpler, decrement least significant index. - _indices[^1]--; - _complete = (checkForZero == 0); - } - - private void IncArray() - { - // This method advances us to the next valid array index, - // handling all the multiple dimension & bounds correctly. - // Think of it like an odometer in your car - we start with - // the last digit, increment it, and check for rollover. If - // it rolls over, we set all digits to the right and including - // the current to the appropriate lower bound. Do these overflow - // checks for each dimension, and if the most significant digit - // has rolled over it's upper bound, we're done. - // - int rank = array.Rank; - _indices[rank - 1]++; - for (int dim = rank - 1; dim >= 0; dim--) - { - if (_indices[dim] > array.GetUpperBound(dim)) - { - if (dim == 0) - { - _complete = true; - break; - } - for (int j = dim; j < rank; j++) - _indices[j] = array.GetLowerBound(j); - _indices[dim - 1]++; - } - } - } - - public object Clone() - { - return MemberwiseClone(); - } - - public bool MoveNext() - { - if (_complete) - { - index = endIndex; - return false; - } - index++; - IncArray(); - return !_complete; - } - - public object? Current - { - get - { - if (index < startIndex) ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumNotStarted(); - if (_complete) ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumEnded(); - return array.GetValue(_indices); - } - } - - public void Reset() - { - index = startIndex - 1; - int checkForZero = 1; - for (int i = 0; i < array.Rank; i++) - { - _indices[i] = array.GetLowerBound(i); - checkForZero *= array.GetLength(i); - } - _complete = (checkForZero == 0); - // To make MoveNext simpler, decrement least significant index. - _indices[^1]--; - } - } - - internal sealed class SZArrayEnumerator : IEnumerator, ICloneable { private readonly Array _array; - private int _index; - private readonly int _endIndex; // Cache Array.Length, since it's a little slow. + private nint _index; - internal SZArrayEnumerator(Array array) + internal ArrayEnumerator(Array array) { - Debug.Assert(array.Rank == 1 && array.GetLowerBound(0) == 0, "SZArrayEnumerator only works on single dimension arrays w/ a lower bound of zero."); - _array = array; _index = -1; - _endIndex = array.Length; } public object Clone() @@ -127,10 +25,11 @@ public object Clone() public bool MoveNext() { - if (_index < _endIndex) + nint length = (nint)_array.LongLength; + if (_index < length) { _index++; - return _index < _endIndex; + return _index < length; } return false; } @@ -139,11 +38,12 @@ public object? Current { get { - if (_index < 0) + nint index = _index; + if (index < 0) ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumNotStarted(); - if (_index >= _endIndex) + if (index >= (nint)_array.LongLength) ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumEnded(); - return _array.GetValue(_index); + return _array.InternalGetValue(index); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.cs b/src/libraries/System.Private.CoreLib/src/System/Array.cs index 85d353343aa6f..b073dc4bd995b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.cs @@ -124,6 +124,77 @@ public static void Copy(Array sourceArray, long sourceIndex, Array destinationAr Copy(sourceArray, isourceIndex, destinationArray, idestinationIndex, ilength); } +#if !MONO + // The various Get values... + public object? GetValue(params int[] indices) + { + if (indices == null) + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.indices); + if (Rank != indices.Length) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankIndices); + + return InternalGetValue(GetFlattenedIndex(new ReadOnlySpan(indices))); + } + + public unsafe object? GetValue(int index) + { + if (Rank != 1) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray); + + return InternalGetValue(GetFlattenedIndex(new ReadOnlySpan(&index, 1))); + } + + public object? GetValue(int index1, int index2) + { + if (Rank != 2) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray); + + return InternalGetValue(GetFlattenedIndex(stackalloc int[] { index1, index2 })); + } + + public object? GetValue(int index1, int index2, int index3) + { + if (Rank != 3) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray); + + return InternalGetValue(GetFlattenedIndex(stackalloc int[] { index1, index2, index3 })); + } + + public unsafe void SetValue(object? value, int index) + { + if (Rank != 1) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray); + + InternalSetValue(value, GetFlattenedIndex(new ReadOnlySpan(&index, 1))); + } + + public void SetValue(object? value, int index1, int index2) + { + if (Rank != 2) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray); + + InternalSetValue(value, GetFlattenedIndex(stackalloc int[] { index1, index2 })); + } + + public void SetValue(object? value, int index1, int index2, int index3) + { + if (Rank != 3) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray); + + InternalSetValue(value, GetFlattenedIndex(stackalloc int[] { index1, index2, index3 })); + } + + public void SetValue(object? value, params int[] indices) + { + if (indices == null) + ThrowHelper.ThrowArgumentNullException(ExceptionArgument.indices); + if (Rank != indices.Length) + ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_RankIndices); + + InternalSetValue(value, GetFlattenedIndex(new ReadOnlySpan(indices))); + } +#endif + public object? GetValue(long index) { int iindex = (int)index; @@ -2304,15 +2375,9 @@ private void InsertionSort(int lo, int hi) private static Span UnsafeArrayAsSpan(Array array, int adjustedIndex, int length) => new Span(ref Unsafe.As(ref array.GetRawArrayData()), array.Length).Slice(adjustedIndex, length); -#if !CORERT public IEnumerator GetEnumerator() { - int lowerBound = GetLowerBound(0); - if (Rank == 1 && lowerBound == 0) - return new SZArrayEnumerator(this); - else - return new ArrayEnumerator(this, lowerBound, Length); + return new ArrayEnumerator(this); } -#endif // !CORERT } } diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index bb4ba939087a5..b863fe0bfa57c 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -376,6 +376,15 @@ public static unsafe Array CreateInstance(Type elementType, int[] lengths, int[] [MethodImpl(MethodImplOptions.InternalCall)] private static extern unsafe void InternalCreate([NotNull] ref Array? result, IntPtr elementType, int rank, int* lengths, int* lowerBounds); + // CAUTION! No bounds checking! + internal object? InternalGetValue(nint index) + { + if (GetType().GetElementType()!.IsPointer) + throw new NotSupportedException(SR.NotSupported_Type); + + return GetValueImpl((int)index); + } + public object GetValue(int index) { if (Rank != 1) From bc399e12e6eec5fdb86f9c2c770a125b2b5b30b7 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Thu, 15 Apr 2021 19:22:06 -0700 Subject: [PATCH 2/4] Implement GetFlattenedIndex for Mono --- .../src/System/Array.cs | 2 - .../src/System/Array.Mono.cs | 88 ++++---------- src/mono/mono/metadata/icall-def-netcore.h | 2 - src/mono/mono/metadata/icall.c | 110 +----------------- 4 files changed, 22 insertions(+), 180 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.cs b/src/libraries/System.Private.CoreLib/src/System/Array.cs index b073dc4bd995b..d0bb4af5bdc4b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.cs @@ -124,7 +124,6 @@ public static void Copy(Array sourceArray, long sourceIndex, Array destinationAr Copy(sourceArray, isourceIndex, destinationArray, idestinationIndex, ilength); } -#if !MONO // The various Get values... public object? GetValue(params int[] indices) { @@ -193,7 +192,6 @@ public void SetValue(object? value, params int[] indices) InternalSetValue(value, GetFlattenedIndex(new ReadOnlySpan(indices))); } -#endif public object? GetValue(long index) { diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index b863fe0bfa57c..c56c972ad6157 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -3,6 +3,7 @@ using Internal.Runtime.CompilerServices; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; @@ -376,46 +377,38 @@ public static unsafe Array CreateInstance(Type elementType, int[] lengths, int[] [MethodImpl(MethodImplOptions.InternalCall)] private static extern unsafe void InternalCreate([NotNull] ref Array? result, IntPtr elementType, int rank, int* lengths, int* lowerBounds); - // CAUTION! No bounds checking! - internal object? InternalGetValue(nint index) + private unsafe nint GetFlattenedIndex(ReadOnlySpan indices) { - if (GetType().GetElementType()!.IsPointer) - throw new NotSupportedException(SR.NotSupported_Type); + // Checked by the caller + Debug.Assert(indices.Length == Rank); - return GetValueImpl((int)index); + nint flattenedIndex = 0; + for (int i = 0; i < indices.Length; i++) + { + int index = indices[i] - GetLowerBound(i); + int length = GetLength(i); + if ((uint)index >= (uint)length) + ThrowHelper.ThrowIndexOutOfRangeException(); + flattenedIndex = (length * flattenedIndex) + index; + } + Debug.Assert((nuint)flattenedIndex < (nuint)LongLength); + return flattenedIndex; } - public object GetValue(int index) + internal object? InternalGetValue(nint index) { - if (Rank != 1) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray); - - int lb = GetLowerBound(0); - if (index < lb || index > GetUpperBound(0)) - throw new IndexOutOfRangeException(SR.Argument_IndexOutOfArrayBounds); - if (GetType().GetElementType()!.IsPointer) throw new NotSupportedException(SR.NotSupported_Type); - return GetValueImpl(index - lb); - } - - public object GetValue(int index1, int index2) - { - if (Rank != 2) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray); - - int[] ind = { index1, index2 }; - return GetValue(ind); + return GetValueImpl((int)index); } - public object GetValue(int index1, int index2, int index3) + internal void InternalSetValue(object? value, nint index) { - if (Rank != 3) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray); + if (GetType().GetElementType()!.IsPointer) + throw new NotSupportedException(SR.NotSupported_Type); - int[] ind = { index1, index2, index3 }; - return GetValue(ind); + SetValueImpl(value, (int)index); } public void Initialize() @@ -432,39 +425,6 @@ private static int LastIndexOfImpl(T[] array, T value, int startIndex, int co return EqualityComparer.Default.LastIndexOf(array, value, startIndex, count); } - public void SetValue(object? value, int index) - { - if (Rank != 1) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need1DArray); - - int lb = GetLowerBound(0); - if (index < lb || index > GetUpperBound(0)) - throw new IndexOutOfRangeException(SR.Argument_IndexOutOfArrayBounds); - - if (GetType().GetElementType()!.IsPointer) - throw new NotSupportedException(SR.NotSupported_Type); - - SetValueImpl(value, index - lb); - } - - public void SetValue(object? value, int index1, int index2) - { - if (Rank != 2) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need2DArray); - - int[] ind = { index1, index2 }; - SetValue(value, ind); - } - - public void SetValue(object? value, int index1, int index2, int index3) - { - if (Rank != 3) - ThrowHelper.ThrowArgumentException(ExceptionResource.Arg_Need3DArray); - - int[] ind = { index1, index2, index3 }; - SetValue(value, ind); - } - public int GetUpperBound(int dimension) { return GetLowerBound(dimension) + GetLength(dimension) - 1; @@ -512,12 +472,6 @@ internal ref byte GetRawArrayData() [MethodImplAttribute(MethodImplOptions.InternalCall)] public extern int GetLowerBound(int dimension); - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public extern object GetValue(params int[] indices); - - [MethodImplAttribute(MethodImplOptions.InternalCall)] - public extern void SetValue(object? value, params int[] indices); - // CAUTION! No bounds checking! [MethodImplAttribute(MethodImplOptions.InternalCall)] private static extern void GetGenericValue_icall(ref Array self, int pos, out T value); diff --git a/src/mono/mono/metadata/icall-def-netcore.h b/src/mono/mono/metadata/icall-def-netcore.h index d1b44dc81c4e0..b7ef6d864e02e 100644 --- a/src/mono/mono/metadata/icall-def-netcore.h +++ b/src/mono/mono/metadata/icall-def-netcore.h @@ -40,12 +40,10 @@ HANDLES(ARRAY_4a, "GetCorElementTypeOfElementType", ves_icall_System_Array_GetCo NOHANDLES(ICALL(ARRAY_5, "GetGenericValue_icall", ves_icall_System_Array_GetGenericValue_icall)) HANDLES(ARRAY_6, "GetLength", ves_icall_System_Array_GetLength, gint32, 2, (MonoArray, gint32)) HANDLES(ARRAY_7, "GetLowerBound", ves_icall_System_Array_GetLowerBound, gint32, 2, (MonoArray, gint32)) -HANDLES(ARRAY_9, "GetValue", ves_icall_System_Array_GetValue, MonoObject, 2, (MonoArray, MonoArray)) HANDLES(ARRAY_10, "GetValueImpl", ves_icall_System_Array_GetValueImpl, MonoObject, 2, (MonoArray, guint32)) NOHANDLES(ICALL(ARRAY_10a, "InternalCreate", ves_icall_System_Array_InternalCreate)) HANDLES(ARRAY_10b, "IsValueOfElementType", ves_icall_System_Array_IsValueOfElementType, gint32, 2, (MonoArray, MonoObject)) NOHANDLES(ICALL(ARRAY_11, "SetGenericValue_icall", ves_icall_System_Array_SetGenericValue_icall)) -HANDLES(ARRAY_12, "SetValue", ves_icall_System_Array_SetValue, void, 3, (MonoArray, MonoObject, MonoArray)) HANDLES(ARRAY_13, "SetValueImpl", ves_icall_System_Array_SetValueImpl, void, 3, (MonoArray, MonoObject, guint32)) HANDLES(ARRAY_14, "SetValueRelaxedImpl", ves_icall_System_Array_SetValueRelaxedImpl, void, 3, (MonoArray, MonoObject, guint32)) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 94f36326b8b30..75d4daf00d916 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -205,57 +205,10 @@ ves_icall_System_Array_GetValueImpl (MonoArrayHandle array, guint32 pos, MonoErr return result; } -MonoObjectHandle -ves_icall_System_Array_GetValue (MonoArrayHandle arr, MonoArrayHandle indices, MonoError *error) -{ - MONO_CHECK_ARG_NULL_HANDLE (indices, NULL_HANDLE); - - MonoClass * const indices_class = mono_handle_class (indices); - MonoClass * const array_class = mono_handle_class (arr); - - g_assert (m_class_get_rank (indices_class) == 1); - - if (MONO_HANDLE_GETVAL (indices, bounds) || MONO_HANDLE_GETVAL (indices, max_length) != m_class_get_rank (array_class)) { - mono_error_set_argument (error, NULL, NULL); - return NULL_HANDLE; - } - - gint32 index = 0; - - if (!MONO_HANDLE_GETVAL (arr, bounds)) { - MONO_HANDLE_ARRAY_GETVAL (index, indices, gint32, 0); - if (index < 0 || index >= MONO_HANDLE_GETVAL (arr, max_length)) { - mono_error_set_index_out_of_range (error); - return NULL_HANDLE; - } - - return ves_icall_System_Array_GetValueImpl (arr, index, error); - } - - for (gint32 i = 0; i < m_class_get_rank (array_class); i++) { - MONO_HANDLE_ARRAY_GETVAL (index, indices, gint32, i); - if ((index < MONO_HANDLE_GETVAL (arr, bounds [i].lower_bound)) || - (index >= (mono_array_lower_bound_t)MONO_HANDLE_GETVAL (arr, bounds [i].length) + MONO_HANDLE_GETVAL (arr, bounds [i].lower_bound))) { - mono_error_set_index_out_of_range (error); - return NULL_HANDLE; - } - } - - MONO_HANDLE_ARRAY_GETVAL (index, indices, gint32, 0); - gint32 pos = index - MONO_HANDLE_GETVAL (arr, bounds [0].lower_bound); - for (gint32 i = 1; i < m_class_get_rank (array_class); i++) { - MONO_HANDLE_ARRAY_GETVAL (index, indices, gint32, i); - pos = pos * MONO_HANDLE_GETVAL (arr, bounds [i].length) + index - - MONO_HANDLE_GETVAL (arr, bounds [i].lower_bound); - } - - return ves_icall_System_Array_GetValueImpl (arr, pos, error); -} - void ves_icall_System_Array_SetValueImpl (MonoArrayHandle arr, MonoObjectHandle value, guint32 pos, MonoError *error) { - array_set_value_impl (arr, value, pos, FALSE, TRUE, error); + array_set_value_impl (arr, value, pos, TRUE, TRUE, error); } static inline void @@ -726,67 +679,6 @@ array_set_value_impl (MonoArrayHandle arr_handle, MonoObjectHandle value_handle, return; } -void -ves_icall_System_Array_SetValue (MonoArrayHandle arr, MonoObjectHandle value, - MonoArrayHandle idxs, MonoError *error) -{ - icallarray_print ("%s\n", __func__); - - MonoArrayBounds dim; - MonoClass *ac, *ic; - gint32 idx; - gint32 i, pos; - - error_init (error); - - if (MONO_HANDLE_IS_NULL (idxs)) { - mono_error_set_argument_null (error, "indices", ""); - return; - } - - ic = mono_handle_class (idxs); - ac = mono_handle_class (arr); - - g_assert (m_class_get_rank (ic) == 1); - if (mono_handle_array_has_bounds (idxs) || MONO_HANDLE_GETVAL (idxs, max_length) != m_class_get_rank (ac)) { - mono_error_set_argument (error, NULL, ""); - return; - } - - if (!mono_handle_array_has_bounds (arr)) { - MONO_HANDLE_ARRAY_GETVAL (idx, idxs, gint32, 0); - if (idx < 0 || idx >= MONO_HANDLE_GETVAL (arr, max_length)) { - mono_error_set_exception_instance (error, mono_get_exception_index_out_of_range ()); - return; - } - - array_set_value_impl (arr, value, idx, TRUE, TRUE, error); - return; - } - - gint32 ac_rank = m_class_get_rank (ac); - for (i = 0; i < ac_rank; i++) { - mono_handle_array_get_bounds_dim (arr, i, &dim); - MONO_HANDLE_ARRAY_GETVAL (idx, idxs, gint32, i); - if ((idx < dim.lower_bound) || - (idx >= (mono_array_lower_bound_t)dim.length + dim.lower_bound)) { - mono_error_set_exception_instance (error, mono_get_exception_index_out_of_range ()); - return; - } - } - - MONO_HANDLE_ARRAY_GETVAL (idx, idxs, gint32, 0); - mono_handle_array_get_bounds_dim (arr, 0, &dim); - pos = idx - dim.lower_bound; - for (i = 1; i < ac_rank; i++) { - mono_handle_array_get_bounds_dim (arr, i, &dim); - MONO_HANDLE_ARRAY_GETVAL (idx, idxs, gint32, i); - pos = pos * dim.length + idx - dim.lower_bound; - } - - array_set_value_impl (arr, value, pos, TRUE, TRUE, error); -} - void ves_icall_System_Array_InternalCreate (MonoArray *volatile* result, MonoType* type, gint32 rank, gint32* pLengths, gint32* pLowerBounds) { From b16953259669fbd7b8a8a7bb6b57fa1aeef2d3e7 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 20 Apr 2021 14:40:11 -0700 Subject: [PATCH 3/4] Feedback --- .../classlibnative/bcltype/arraynative.cpp | 5 +-- .../src/System/Array.Enumerators.cs | 31 ++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/coreclr/classlibnative/bcltype/arraynative.cpp b/src/coreclr/classlibnative/bcltype/arraynative.cpp index c7a3a419b03eb..b0236e918704b 100644 --- a/src/coreclr/classlibnative/bcltype/arraynative.cpp +++ b/src/coreclr/classlibnative/bcltype/arraynative.cpp @@ -1041,10 +1041,11 @@ FCIMPL3(void, ArrayNative::SetValue, ArrayBase* refThisUNSAFE, Object* objUNSAFE COMPlusThrow(kInvalidCastException,W("InvalidCast_StoreArrayElement")); HELPER_METHOD_FRAME_END(); + + // Refresh pData in case GC moved objects around + pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize(); } - // Refresh pData in case GC moved objects around - pData = refThis->GetDataPtr() + flattenedIndex * refThis->GetComponentSize(); SetObjectReference((OBJECTREF*)pData,obj); } else diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index ccf1af152e952..93cd76c30e9e1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -25,13 +25,13 @@ public object Clone() public bool MoveNext() { - nint length = (nint)_array.LongLength; - if (_index < length) + nint index = _index + 1; + if ((nuint)index >= (nuint)_array.LongLength) { - _index++; - return _index < length; + return false; } - return false; + _index = index; + return true; } public object? Current @@ -39,11 +39,21 @@ public object? Current get { nint index = _index; - if (index < 0) - ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumNotStarted(); - if (index >= (nint)_array.LongLength) - ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumEnded(); - return _array.InternalGetValue(index); + Array array = _array; + + if ((nuint)index >= (nuint)array.LongLength) + { + if (index < 0) + { + ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumNotStarted(); + } + else + { + ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumEnded(); + } + } + + return array.InternalGetValue(index); } } @@ -77,7 +87,6 @@ public bool MoveNext() int index = _index + 1; if ((uint)index >= (uint)_array.Length) { - _index = _array.Length; return false; } _index = index; From 7893de107022b8ea0d7e669bccbe87e866773cf2 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 20 Apr 2021 16:25:17 -0700 Subject: [PATCH 4/4] Fix test failures --- .../System.Private.CoreLib/src/System/Array.Enumerators.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs index 93cd76c30e9e1..da01cb5fe9e1a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs @@ -28,6 +28,7 @@ public bool MoveNext() nint index = _index + 1; if ((nuint)index >= (nuint)_array.LongLength) { + _index = (nint)_array.LongLength; return false; } _index = index; @@ -87,6 +88,7 @@ public bool MoveNext() int index = _index + 1; if ((uint)index >= (uint)_array.Length) { + _index = _array.Length; return false; } _index = index;