From c70c01d4107e0cdff444f64ef85fa6e2297ad0ff Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Thu, 7 Nov 2019 21:00:36 -0800 Subject: [PATCH] Revert "Change BulkMoveWithWriteBarrier to be GC suspension friendly (#27642)" This reverts commit 5e1ef698774c433f70795e194e2554ad6f5b7d6f. --- .../src/System/Buffer.CoreCLR.cs | 49 +------------------ .../src/System/Object.CoreCLR.cs | 28 +---------- .../RuntimeHelpers.CoreCLR.cs | 18 ------- src/classlibnative/bcltype/objectnative.cpp | 31 +++++++++--- src/classlibnative/bcltype/objectnative.h | 2 +- src/vm/ecalllist.h | 4 +- 6 files changed, 29 insertions(+), 103 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Buffer.CoreCLR.cs b/src/System.Private.CoreLib/src/System/Buffer.CoreCLR.cs index 527b92c2bd99..0dc6a571c952 100644 --- a/src/System.Private.CoreLib/src/System/Buffer.CoreCLR.cs +++ b/src/System.Private.CoreLib/src/System/Buffer.CoreCLR.cs @@ -10,10 +10,8 @@ #pragma warning disable SA1121 // explicitly using type aliases instead of built-in types #if BIT64 using nuint = System.UInt64; -using nint = System.UInt64; #else using nuint = System.UInt32; -using nint = System.UInt32; #endif namespace System @@ -39,53 +37,8 @@ internal static unsafe void _ZeroMemory(ref byte b, nuint byteLength) [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] private static extern unsafe void __ZeroMemory(void* b, nuint byteLength); - // The maximum block size to for __BulkMoveWithWriteBarrier FCall. This is required to avoid GC starvation. - private const uint BulkMoveWithWriteBarrierChunk = 0x4000; - - internal static void BulkMoveWithWriteBarrier(ref byte destination, ref byte source, nuint byteCount) - { - if (byteCount <= BulkMoveWithWriteBarrierChunk) - __BulkMoveWithWriteBarrier(ref destination, ref source, byteCount); - else - _BulkMoveWithWriteBarrier(ref destination, ref source, byteCount); - } - - // Non-inlinable wrapper around the loop for copying large blocks in chunks - [MethodImpl(MethodImplOptions.NoInlining)] - private static void _BulkMoveWithWriteBarrier(ref byte destination, ref byte source, nuint byteCount) - { - Debug.Assert(byteCount > BulkMoveWithWriteBarrierChunk); - - if (Unsafe.AreSame(ref destination, ref source)) - return; - - if ((nuint)(nint)Unsafe.ByteOffset(ref destination, ref source) >= byteCount) - { - // Copy forwards - do - { - byteCount -= BulkMoveWithWriteBarrierChunk; - __BulkMoveWithWriteBarrier(ref destination, ref source, BulkMoveWithWriteBarrierChunk); - destination = ref Unsafe.AddByteOffset(ref destination, BulkMoveWithWriteBarrierChunk); - source = ref Unsafe.AddByteOffset(ref source, BulkMoveWithWriteBarrierChunk); - } - while (byteCount > BulkMoveWithWriteBarrierChunk); - } - else - { - // Copy backwards - do - { - byteCount -= BulkMoveWithWriteBarrierChunk; - __BulkMoveWithWriteBarrier(ref Unsafe.AddByteOffset(ref destination, byteCount), ref Unsafe.AddByteOffset(ref source, byteCount), BulkMoveWithWriteBarrierChunk); - } - while (byteCount > BulkMoveWithWriteBarrierChunk); - } - __BulkMoveWithWriteBarrier(ref destination, ref source, byteCount); - } - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void __BulkMoveWithWriteBarrier(ref byte destination, ref byte source, nuint byteCount); + internal static extern void BulkMoveWithWriteBarrier(ref byte destination, ref byte source, nuint byteCount); [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] private static extern unsafe void __Memmove(byte* dest, byte* src, nuint len); diff --git a/src/System.Private.CoreLib/src/System/Object.CoreCLR.cs b/src/System.Private.CoreLib/src/System/Object.CoreCLR.cs index db3d6935cdd6..aceff3c3a6b8 100644 --- a/src/System.Private.CoreLib/src/System/Object.CoreCLR.cs +++ b/src/System.Private.CoreLib/src/System/Object.CoreCLR.cs @@ -4,15 +4,6 @@ using System.Runtime.CompilerServices; -using Internal.Runtime.CompilerServices; - -#pragma warning disable SA1121 // explicitly using type aliases instead of built-in types -#if BIT64 -using nuint = System.UInt64; -#else -using nuint = System.UInt32; -#endif - namespace System { public partial class Object @@ -25,22 +16,7 @@ public partial class Object // object. This is always a shallow copy of the instance. The method is protected // so that other object may only call this method on themselves. It is intended to // support the ICloneable interface. - protected unsafe object MemberwiseClone() - { - object clone = RuntimeHelpers.AllocateUninitializedClone(this); - - // copy contents of "this" to the clone - - nuint byteCount = RuntimeHelpers.GetRawObjectDataSize(clone); - ref byte src = ref this.GetRawData(); - ref byte dst = ref clone.GetRawData(); - - if (RuntimeHelpers.GetMethodTable(clone)->ContainsGCPointers) - Buffer.BulkMoveWithWriteBarrier(ref dst, ref src, byteCount); - else - Buffer.Memmove(ref dst, ref src, byteCount); - - return clone; - } + [MethodImpl(MethodImplOptions.InternalCall)] + protected extern object MemberwiseClone(); } } diff --git a/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 5753585c288e..e6e9211950ce 100644 --- a/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -149,9 +149,6 @@ public static int OffsetToStringData [MethodImpl(MethodImplOptions.InternalCall)] private static extern object GetUninitializedObjectInternal(Type type); - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern object AllocateUninitializedClone(object obj); - /// true if given type is reference type or value type that contains references [Intrinsic] public static bool IsReferenceOrContainsReferences() @@ -192,21 +189,6 @@ internal static int EnumCompareTo(T x, T y) where T : struct, Enum internal static ref byte GetRawData(this object obj) => ref Unsafe.As(obj).Data; - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal static unsafe nuint GetRawObjectDataSize(object obj) - { - MethodTable* pMT = GetMethodTable(obj); - - // See comment on RawArrayData for details - nuint rawSize = pMT->BaseSize - (nuint)(2 * sizeof(IntPtr)); - if (pMT->HasComponentSize) - rawSize += (uint)Unsafe.As(obj).Length * (nuint)pMT->ComponentSize; - - GC.KeepAlive(obj); // Keep MethodTable alive - - return rawSize; - } - [Intrinsic] internal static ref byte GetRawSzArrayData(this Array array) => ref Unsafe.As(array).Data; diff --git a/src/classlibnative/bcltype/objectnative.cpp b/src/classlibnative/bcltype/objectnative.cpp index e285eeb69ecf..45fbfe089024 100644 --- a/src/classlibnative/bcltype/objectnative.cpp +++ b/src/classlibnative/bcltype/objectnative.cpp @@ -221,19 +221,22 @@ FCIMPL1(Object*, ObjectNative::GetClass, Object* pThis) } FCIMPLEND -FCIMPL1(Object*, ObjectNative::AllocateUninitializedClone, Object* pObjUNSAFE) +FCIMPL1(Object*, ObjectNative::Clone, Object* pThisUNSAFE) { FCALL_CONTRACT; - // Delegate error handling to managed side (it will throw NullRefenceException) - if (pObjUNSAFE == NULL) - return NULL; + OBJECTREF refClone = NULL; + OBJECTREF refThis = ObjectToOBJECTREF(pThisUNSAFE); - OBJECTREF refClone = ObjectToOBJECTREF(pObjUNSAFE); + if (refThis == NULL) + FCThrow(kNullReferenceException); + + HELPER_METHOD_FRAME_BEGIN_RET_2(refClone, refThis); - HELPER_METHOD_FRAME_BEGIN_RET_1(refClone); + // ObjectNative::Clone() ensures that the source and destination are always in + // the same context. - MethodTable* pMT = refClone->GetMethodTable(); + MethodTable* pMT = refThis->GetMethodTable(); // assert that String has overloaded the Clone() method _ASSERTE(pMT != g_pStringClass); @@ -242,13 +245,25 @@ FCIMPL1(Object*, ObjectNative::AllocateUninitializedClone, Object* pObjUNSAFE) #endif // FEATURE_UTF8STRING if (pMT->IsArray()) { - refClone = DupArrayForCloning((BASEARRAYREF)refClone); + refClone = DupArrayForCloning((BASEARRAYREF)refThis); } else { // We don't need to call the because we know // that it has been called....(It was called before this was created) refClone = AllocateObject(pMT); } + SIZE_T cb = refThis->GetSize() - sizeof(ObjHeader); + + // copy contents of "this" to the clone + if (pMT->ContainsPointers()) + { + memmoveGCRefs(OBJECTREFToObject(refClone), OBJECTREFToObject(refThis), cb); + } + else + { + memcpyNoGCRefs(OBJECTREFToObject(refClone), OBJECTREFToObject(refThis), cb); + } + HELPER_METHOD_FRAME_END(); return OBJECTREFToObject(refClone); diff --git a/src/classlibnative/bcltype/objectnative.h b/src/classlibnative/bcltype/objectnative.h index 0762298b5832..2cccb39eda60 100644 --- a/src/classlibnative/bcltype/objectnative.h +++ b/src/classlibnative/bcltype/objectnative.h @@ -32,7 +32,7 @@ class ObjectNative static FCDECL1(Object*, GetObjectValue, Object* vThisRef); static FCDECL1(INT32, GetHashCode, Object* vThisRef); static FCDECL2(FC_BOOL_RET, Equals, Object *pThisRef, Object *pCompareRef); - static FCDECL1(Object*, AllocateUninitializedClone, Object* pObjUNSAFE); + static FCDECL1(Object*, Clone, Object* pThis); static FCDECL1(Object*, GetClass, Object* pThis); static FCDECL3(FC_BOOL_RET, WaitTimeout, CLR_BOOL exitContext, INT32 Timeout, Object* pThisUNSAFE); static FCDECL1(void, Pulse, Object* pThisUNSAFE); diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h index a3396f92bb49..a85894923276 100644 --- a/src/vm/ecalllist.h +++ b/src/vm/ecalllist.h @@ -92,6 +92,7 @@ FCFuncEnd() FCFuncStart(gObjectFuncs) FCIntrinsic("GetType", ObjectNative::GetClass, CORINFO_INTRINSIC_Object_GetType) + FCFuncElement("MemberwiseClone", ObjectNative::Clone) FCFuncEnd() FCFuncStart(gStringFuncs) @@ -727,7 +728,7 @@ FCFuncEnd() FCFuncStart(gBufferFuncs) FCFuncElement("IsPrimitiveTypeArray", Buffer::IsPrimitiveTypeArray) QCFuncElement("__ZeroMemory", Buffer::Clear) - FCFuncElement("__BulkMoveWithWriteBarrier", Buffer::BulkMoveWithWriteBarrier) + FCFuncElement("BulkMoveWithWriteBarrier", Buffer::BulkMoveWithWriteBarrier) QCFuncElement("__Memmove", Buffer::MemMove) FCFuncEnd() @@ -911,7 +912,6 @@ FCFuncStart(gRuntimeHelpers) FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate) FCFuncElement("GetHashCode", ObjectNative::GetHashCode) FCFuncElement("Equals", ObjectNative::Equals) - FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone) FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack) FCFuncElement("TryEnsureSufficientExecutionStack", ReflectionInvocation::TryEnsureSufficientExecutionStack) FCFuncElement("GetUninitializedObjectInternal", ReflectionSerialization::GetUninitializedObject)