From ddd03fe266c8e17cc46b5c9d67d2bb58f4810411 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 28 Aug 2024 16:18:20 -0700 Subject: [PATCH 1/3] Convert stream override checks to QCalls and reduce the check frequency. --- .../System.Private.CoreLib.csproj | 1 + .../src/System/IO/Stream.CoreCLR.cs | 41 +++++++++++++ .../RuntimeHelpers.CoreCLR.cs | 24 ++++++++ .../src/System.Private.CoreLib.csproj | 5 +- .../src/System/IO/Stream.NativeAot.cs | 18 ++++++ src/coreclr/vm/comutilnative.cpp | 50 ++++++--------- src/coreclr/vm/comutilnative.h | 8 +-- src/coreclr/vm/ecalllist.h | 6 -- src/coreclr/vm/methodtable.h | 61 +++++++++++-------- src/coreclr/vm/qcallentrypoints.cpp | 1 + .../src/System/IO/Stream.cs | 10 +-- .../System.Private.CoreLib.csproj | 1 + .../src/System/IO/Stream.Mono.cs | 18 ++++++ 13 files changed, 167 insertions(+), 77 deletions(-) create mode 100644 src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs create mode 100644 src/coreclr/nativeaot/System.Private.CoreLib/src/System/IO/Stream.NativeAot.cs create mode 100644 src/mono/System.Private.CoreLib/src/System/IO/Stream.Mono.cs diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj index 4ffdcd15afb5fa..e027d8d7d757af 100644 --- a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -140,6 +140,7 @@ + diff --git a/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs new file mode 100644 index 00000000000000..5ca1ff3340f734 --- /dev/null +++ b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs @@ -0,0 +1,41 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +namespace System.IO +{ + public abstract unsafe partial class Stream + { + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "StreamNative_HasOverriddenSlow")] + [return: MarshalAs(UnmanagedType.Bool)] + private static partial bool HasOverriddenSlow(MethodTable* pMT, [MarshalAs(UnmanagedType.Bool)] bool isRead); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool HasOverriddenReadSlow(MethodTable* pMT) => HasOverriddenSlow(pMT, isRead: true); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool HasOverriddenWriteSlow(MethodTable* pMT) => HasOverriddenSlow(pMT, isRead: false); + + private bool HasOverriddenBeginEndRead() + { + MethodTable* pMT = RuntimeHelpers.GetMethodTable(this); + bool res = pMT->AuxiliaryData->HasCheckedStreamOverride + ? pMT->AuxiliaryData->IsStreamOverriddenRead + : HasOverriddenReadSlow(pMT); + GC.KeepAlive(this); + return res; + } + + private bool HasOverriddenBeginEndWrite() + { + MethodTable* pMT = RuntimeHelpers.GetMethodTable(this); + bool res = pMT->AuxiliaryData->HasCheckedStreamOverride + ? pMT->AuxiliaryData->IsStreamOverriddenWrite + : HasOverriddenWriteSlow(pMT); + GC.KeepAlive(this); + return res; + } + } +} diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 3af1ecf75c4573..77311a0242172b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -813,6 +813,10 @@ internal unsafe struct MethodTableAuxiliaryData private const uint enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode = 0x0002; // Whether we have checked the overridden Equals or GetHashCode private const uint enum_flag_CanCompareBitsOrUseFastGetHashCode = 0x0004; // Is any field type or sub field type overridden Equals or GetHashCode + private const uint enum_flag_HasCheckedStreamOverride = 0x0400; + private const uint enum_flag_StreamOverriddenRead = 0x0800; + private const uint enum_flag_StreamOverriddenWrite = 0x1000; + public bool HasCheckedCanCompareBitsOrUseFastGetHashCode => (Flags & enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode) != 0; public bool CanCompareBitsOrUseFastGetHashCode @@ -824,6 +828,26 @@ public bool CanCompareBitsOrUseFastGetHashCode } } + public bool HasCheckedStreamOverride => (Flags & enum_flag_HasCheckedStreamOverride) != 0; + + public bool IsStreamOverriddenRead + { + get + { + Debug.Assert(HasCheckedStreamOverride); + return (Flags & enum_flag_StreamOverriddenRead) != 0; + } + } + + public bool IsStreamOverriddenWrite + { + get + { + Debug.Assert(HasCheckedStreamOverride); + return (Flags & enum_flag_StreamOverriddenWrite) != 0; + } + } + public RuntimeType? ExposedClassObject { get diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj index fc97632716fd2a..40c9c951a59c72 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj @@ -174,6 +174,7 @@ + @@ -210,7 +211,7 @@ - + @@ -286,7 +287,7 @@ Interop\Windows\Ole32\Interop.CoGetContextToken.cs - + Interop\Windows\OleAut32\Interop.VariantClear.cs diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/IO/Stream.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/IO/Stream.NativeAot.cs new file mode 100644 index 00000000000000..b8633b200ab678 --- /dev/null +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/IO/Stream.NativeAot.cs @@ -0,0 +1,18 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.IO +{ + public abstract partial class Stream + { + [Intrinsic] + [MethodImpl(MethodImplOptions.InternalCall)] + private extern bool HasOverriddenBeginEndRead(); + + [Intrinsic] + [MethodImpl(MethodImplOptions.InternalCall)] + private extern bool HasOverriddenBeginEndWrite(); + } +} diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 82a855f8864a87..789f86483d3e56 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1898,46 +1898,36 @@ static bool HasOverriddenStreamMethod(MethodTable * pMT, WORD slot) return true; } -FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndRead, Object *stream) +extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL isRead) { - FCALL_CONTRACT; + QCALL_CONTRACT; + _ASSERTE(pMT != NULL); - if (stream == NULL) - FC_RETURN_BOOL(TRUE); + BOOL readOverride = FALSE; + BOOL writeOverride = FALSE; - if (g_pStreamMT == NULL || g_slotBeginRead == 0 || g_slotEndRead == 0) + BEGIN_QCALL; + + // Check for needed details + if (g_pStreamMT == NULL + || g_slotBeginRead == 0 + || g_slotEndRead == 0 + || g_slotBeginWrite == 0 + || g_slotEndWrite == 0) { - HELPER_METHOD_FRAME_BEGIN_RET_1(stream); g_pStreamMT = CoreLibBinder::GetClass(CLASS__STREAM); g_slotBeginRead = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_READ)->GetSlot(); g_slotEndRead = CoreLibBinder::GetMethod(METHOD__STREAM__END_READ)->GetSlot(); - HELPER_METHOD_FRAME_END(); - } - - MethodTable * pMT = stream->GetMethodTable(); - - FC_RETURN_BOOL(HasOverriddenStreamMethod(pMT, g_slotBeginRead) || HasOverriddenStreamMethod(pMT, g_slotEndRead)); -} -FCIMPLEND - -FCIMPL1(FC_BOOL_RET, StreamNative::HasOverriddenBeginEndWrite, Object *stream) -{ - FCALL_CONTRACT; - - if (stream == NULL) - FC_RETURN_BOOL(TRUE); - - if (g_pStreamMT == NULL || g_slotBeginWrite == 0 || g_slotEndWrite == 0) - { - HELPER_METHOD_FRAME_BEGIN_RET_1(stream); - g_pStreamMT = CoreLibBinder::GetClass(CLASS__STREAM); g_slotBeginWrite = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_WRITE)->GetSlot(); g_slotEndWrite = CoreLibBinder::GetMethod(METHOD__STREAM__END_WRITE)->GetSlot(); - HELPER_METHOD_FRAME_END(); } - MethodTable * pMT = stream->GetMethodTable(); + // Check the current type and update state. + readOverride = HasOverriddenStreamMethod(pMT, g_slotBeginRead) || HasOverriddenStreamMethod(pMT, g_slotEndRead); + writeOverride = HasOverriddenStreamMethod(pMT, g_slotBeginWrite) || HasOverriddenStreamMethod(pMT, g_slotEndWrite); + pMT->GetAuxiliaryDataForWrite()->SetStreamOverrideState(readOverride, writeOverride); + + END_QCALL; - FC_RETURN_BOOL(HasOverriddenStreamMethod(pMT, g_slotBeginWrite) || HasOverriddenStreamMethod(pMT, g_slotEndWrite)); + return isRead ? readOverride : writeOverride; } -FCIMPLEND diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index f69fcbb4b9b85e..605df8e6063237 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -264,12 +264,8 @@ extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodT extern "C" BOOL QCALLTYPE TypeHandle_CanCastTo_NoCacheLookup(void* fromTypeHnd, void* toTypeHnd); extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, MethodTable** fieldMT); -class StreamNative { -public: - static FCDECL1(FC_BOOL_RET, HasOverriddenBeginEndRead, Object *stream); - static FCDECL1(FC_BOOL_RET, HasOverriddenBeginEndWrite, Object *stream); -}; - BOOL CanCompareBitsOrUseFastGetHashCode(MethodTable* mt); +extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL isRead); + #endif // _COMUTILNATIVE_H_ diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 7dc5e664dfd423..282e1acc2a17a0 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -458,11 +458,6 @@ FCFuncStart(gGCHandleFuncs) FCFuncElement("InternalCompareExchange", MarshalNative::GCHandleInternalCompareExchange) FCFuncEnd() -FCFuncStart(gStreamFuncs) - FCFuncElement("HasOverriddenBeginEndRead", StreamNative::HasOverriddenBeginEndRead) - FCFuncElement("HasOverriddenBeginEndWrite", StreamNative::HasOverriddenBeginEndWrite) -FCFuncEnd() - FCFuncStart(gComAwareWeakReferenceFuncs) FCFuncElement("HasInteropInfo", ComAwareWeakReferenceNative::HasInteropInfo) FCFuncEnd() @@ -509,7 +504,6 @@ FCClassElement("RuntimeTypeHandle", "System", gCOMTypeHandleFuncs) FCClassElement("Signature", "System", gSignatureNative) FCClassElement("StackTrace", "System.Diagnostics", gDiagnosticsStackTrace) -FCClassElement("Stream", "System.IO", gStreamFuncs) FCClassElement("String", "System", gStringFuncs) FCClassElement("StubHelpers", "System.StubHelpers", gStubHelperFuncs) FCClassElement("Thread", "System.Threading", gThreadFuncs) diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index cfd395d660829e..9dc9d977793bd8 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -326,30 +326,23 @@ struct MethodTableAuxiliaryData // TO BE UPDATED IN ORDER TO ENSURE THAT METHODTABLES DUPLICATED FOR GENERIC INSTANTIATIONS // CARRY THE CORRECT INITIAL FLAGS. - enum_flag_Initialized = 0x0001, - enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode = 0x0002, // Whether we have checked the overridden Equals or GetHashCode - enum_flag_CanCompareBitsOrUseFastGetHashCode = 0x0004, // Is any field type or sub field type overridden Equals or GetHashCode - + enum_flag_Initialized = 0x0001, + enum_flag_HasCheckedCanCompareBitsOrUseFastGetHashCode = 0x0002, // Whether we have checked the overridden Equals or GetHashCode + enum_flag_CanCompareBitsOrUseFastGetHashCode = 0x0004, // Is any field type or sub field type overridden Equals or GetHashCode + enum_flag_IsTlsIndexAllocated = 0x0008, enum_flag_HasApproxParent = 0x0010, -#ifdef _DEBUG - // The MethodTable is in the right state to be published, and will be inevitably. - // Currently DEBUG only as it does not affect behavior in any way in a release build - enum_flag_IsPublished = 0x0020, -#endif + enum_flag_MayHaveOpenInterfaceInInterfaceMap = 0x0020, enum_flag_IsNotFullyLoaded = 0x0040, enum_flag_DependenciesLoaded = 0x0080, // class and all dependencies loaded up to CLASS_LOADED_BUT_NOT_VERIFIED enum_flag_IsInitError = 0x0100, enum_flag_IsStaticDataAllocated = 0x0200, // When this is set, if the class can be marked as initialized without any further code execution it will be. - // unum_unused = 0x0400, - enum_flag_IsTlsIndexAllocated = 0x0800, - enum_flag_MayHaveOpenInterfaceInInterfaceMap = 0x1000, - // enum_unused = 0x2000, - -#ifdef _DEBUG - enum_flag_ParentMethodTablePointerValid = 0x4000, - enum_flag_HasInjectedInterfaceDuplicates = 0x8000, -#endif + enum_flag_HasCheckedStreamOverride = 0x0400, + enum_flag_StreamOverriddenRead = 0x0800, + enum_flag_StreamOverriddenWrite = 0x1000, + // unused enum = 0x2000, + // unused enum = 0x4000, + // unused enum = 0x8000, }; union { @@ -369,6 +362,17 @@ struct MethodTableAuxiliaryData RUNTIMETYPEHANDLE m_hExposedClassObject; #ifdef _DEBUG + enum + { + // The MethodTable is in the right state to be published, and will be inevitably. + // Currently DEBUG only as it does not affect behavior in any way in a release build + enum_flagDebug_IsPublished = 0x2000, + enum_flagDebug_ParentMethodTablePointerValid = 0x4000, + enum_flagDebug_HasInjectedInterfaceDuplicates = 0x8000, + + }; + DWORD m_dwFlagsDebug; + // to avoid verify same method table too many times when it's not changing, we cache the GC count // on which the method table is verified. When fast GC STRESS is turned on, we only verify the MT if // current GC count is bigger than the number. Note most thing which will invalidate a MT will require a @@ -403,13 +407,13 @@ struct MethodTableAuxiliaryData { LIMITED_METHOD_DAC_CONTRACT; - return (m_dwFlags & enum_flag_ParentMethodTablePointerValid); + return (m_dwFlagsDebug & enum_flagDebug_ParentMethodTablePointerValid); } inline void SetParentMethodTablePointerValid() { LIMITED_METHOD_CONTRACT; - m_dwFlags |= enum_flag_ParentMethodTablePointerValid; + m_dwFlagsDebug |= enum_flagDebug_ParentMethodTablePointerValid; } #endif @@ -485,6 +489,15 @@ struct MethodTableAuxiliaryData } #endif + inline void SetStreamOverrideState(BOOL read, BOOL write) + { + LONG streamOverride = + enum_flag_HasCheckedStreamOverride + | (read ? enum_flag_StreamOverriddenRead : 0) + | (write ? enum_flag_StreamOverriddenWrite : 0); + InterlockedOr((LONG*)&m_dwFlags, streamOverride); + } + inline RUNTIMETYPEHANDLE GetExposedClassObjectHandle() const { LIMITED_METHOD_CONTRACT; @@ -515,7 +528,7 @@ struct MethodTableAuxiliaryData void SetIsPublished() { LIMITED_METHOD_CONTRACT; - m_dwFlags |= (MethodTableAuxiliaryData::enum_flag_IsPublished); + m_dwFlagsDebug |= (MethodTableAuxiliaryData::enum_flagDebug_IsPublished); } #endif @@ -524,7 +537,7 @@ struct MethodTableAuxiliaryData bool IsPublished() const { LIMITED_METHOD_CONTRACT; - return (VolatileLoad(&m_dwFlags) & enum_flag_IsPublished); + return (VolatileLoad(&m_dwFlagsDebug) & enum_flagDebug_IsPublished); } #endif // _DEBUG @@ -3048,12 +3061,12 @@ public : inline BOOL Debug_HasInjectedInterfaceDuplicates() const { LIMITED_METHOD_CONTRACT; - return (GetAuxiliaryData()->m_dwFlags & MethodTableAuxiliaryData::enum_flag_HasInjectedInterfaceDuplicates) != 0; + return (GetAuxiliaryData()->m_dwFlagsDebug & MethodTableAuxiliaryData::enum_flagDebug_HasInjectedInterfaceDuplicates) != 0; } inline void Debug_SetHasInjectedInterfaceDuplicates() { LIMITED_METHOD_CONTRACT; - GetAuxiliaryDataForWrite()->m_dwFlags |= MethodTableAuxiliaryData::enum_flag_HasInjectedInterfaceDuplicates; + GetAuxiliaryDataForWrite()->m_dwFlagsDebug |= MethodTableAuxiliaryData::enum_flagDebug_HasInjectedInterfaceDuplicates; } #endif // _DEBUG diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 215e2d34ef69a9..eca35de81c941a 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -108,6 +108,7 @@ static const Entry s_QCall[] = DllImportEntry(MethodTable_CanCompareBitsOrUseFastGetHashCode) DllImportEntry(TypeHandle_CanCastTo_NoCacheLookup) DllImportEntry(ValueType_GetHashCodeStrategy) + DllImportEntry(StreamNative_HasOverriddenSlow) DllImportEntry(RuntimeTypeHandle_MakePointer) DllImportEntry(RuntimeTypeHandle_MakeByRef) DllImportEntry(RuntimeTypeHandle_MakeSZArray) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs index 1af85eed92d91e..4a4c00efd30137 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs @@ -11,7 +11,7 @@ namespace System.IO { - public abstract class Stream : MarshalByRefObject, IDisposable, IAsyncDisposable + public abstract partial class Stream : MarshalByRefObject, IDisposable, IAsyncDisposable { public static readonly Stream Null = new NullStream(); @@ -447,14 +447,6 @@ private async ValueTask ReadAtLeastAsyncCore(Memory buffer, int minim return totalRead; } - [Intrinsic] - [MethodImpl(MethodImplOptions.InternalCall)] - private extern bool HasOverriddenBeginEndRead(); - - [Intrinsic] - [MethodImpl(MethodImplOptions.InternalCall)] - private extern bool HasOverriddenBeginEndWrite(); - private Task BeginEndReadAsync(byte[] buffer, int offset, int count) { if (!HasOverriddenBeginEndRead()) diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj index d7983bef487860..8f6fbf18070cd1 100644 --- a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj +++ b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj @@ -186,6 +186,7 @@ + diff --git a/src/mono/System.Private.CoreLib/src/System/IO/Stream.Mono.cs b/src/mono/System.Private.CoreLib/src/System/IO/Stream.Mono.cs new file mode 100644 index 00000000000000..b8633b200ab678 --- /dev/null +++ b/src/mono/System.Private.CoreLib/src/System/IO/Stream.Mono.cs @@ -0,0 +1,18 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.IO +{ + public abstract partial class Stream + { + [Intrinsic] + [MethodImpl(MethodImplOptions.InternalCall)] + private extern bool HasOverriddenBeginEndRead(); + + [Intrinsic] + [MethodImpl(MethodImplOptions.InternalCall)] + private extern bool HasOverriddenBeginEndWrite(); + } +} From cb49e6a5b70a5297f3e41fc2c6b0c7613e1c4341 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 28 Aug 2024 21:11:05 -0700 Subject: [PATCH 2/3] Feedback. --- .../src/System/IO/Stream.CoreCLR.cs | 14 ++--- src/coreclr/vm/comutilnative.cpp | 52 +++++++------------ src/coreclr/vm/methodtable.h | 1 - 3 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs index 5ca1ff3340f734..fe0bb26751f754 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs @@ -12,12 +12,6 @@ public abstract unsafe partial class Stream [return: MarshalAs(UnmanagedType.Bool)] private static partial bool HasOverriddenSlow(MethodTable* pMT, [MarshalAs(UnmanagedType.Bool)] bool isRead); - [MethodImpl(MethodImplOptions.NoInlining)] - private static bool HasOverriddenReadSlow(MethodTable* pMT) => HasOverriddenSlow(pMT, isRead: true); - - [MethodImpl(MethodImplOptions.NoInlining)] - private static bool HasOverriddenWriteSlow(MethodTable* pMT) => HasOverriddenSlow(pMT, isRead: false); - private bool HasOverriddenBeginEndRead() { MethodTable* pMT = RuntimeHelpers.GetMethodTable(this); @@ -26,6 +20,10 @@ private bool HasOverriddenBeginEndRead() : HasOverriddenReadSlow(pMT); GC.KeepAlive(this); return res; + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool HasOverriddenReadSlow(MethodTable* pMT) + => HasOverriddenSlow(pMT, isRead: true); } private bool HasOverriddenBeginEndWrite() @@ -36,6 +34,10 @@ private bool HasOverriddenBeginEndWrite() : HasOverriddenWriteSlow(pMT); GC.KeepAlive(this); return res; + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool HasOverriddenWriteSlow(MethodTable* pMT) + => HasOverriddenSlow(pMT, isRead: false); } } } diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 789f86483d3e56..056a760cdb2c6d 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1873,29 +1873,25 @@ extern "C" BOOL QCALLTYPE TypeHandle_CanCastTo_NoCacheLookup(void* fromTypeHnd, return ret; } -static MethodTable * g_pStreamMT; -static WORD g_slotBeginRead, g_slotEndRead; -static WORD g_slotBeginWrite, g_slotEndWrite; - -static bool HasOverriddenStreamMethod(MethodTable * pMT, WORD slot) +static bool HasOverriddenStreamMethod(MethodTable* streamMT, MethodTable* pMT, WORD slot) { - CONTRACTL{ - THROWS; - GC_NOTRIGGER; - MODE_ANY; + CONTRACTL + { + STANDARD_VM_CHECK; + PRECONDITION(streamMT != NULL); + PRECONDITION(pMT != NULL); } CONTRACTL_END; PCODE actual = pMT->GetRestoredSlot(slot); - PCODE base = g_pStreamMT->GetRestoredSlot(slot); - if (actual == base) - return false; + PCODE base = streamMT->GetRestoredSlot(slot); - // If CoreLib is JITed, the slots can be patched and thus we need to compare the actual MethodDescs - // to detect match reliably - if (MethodTable::GetMethodDescForSlotAddress(actual) == MethodTable::GetMethodDescForSlotAddress(base)) + // If the PCODEs match, then there is no override. + if (actual == base) return false; - return true; + // If CoreLib is JITed, the slots can be patched and thus we need to compare + // the actual MethodDescs to detect match reliably. + return MethodTable::GetMethodDescForSlotAddress(actual) != MethodTable::GetMethodDescForSlotAddress(base); } extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL isRead) @@ -1908,23 +1904,15 @@ extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL BEGIN_QCALL; - // Check for needed details - if (g_pStreamMT == NULL - || g_slotBeginRead == 0 - || g_slotEndRead == 0 - || g_slotBeginWrite == 0 - || g_slotEndWrite == 0) - { - g_pStreamMT = CoreLibBinder::GetClass(CLASS__STREAM); - g_slotBeginRead = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_READ)->GetSlot(); - g_slotEndRead = CoreLibBinder::GetMethod(METHOD__STREAM__END_READ)->GetSlot(); - g_slotBeginWrite = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_WRITE)->GetSlot(); - g_slotEndWrite = CoreLibBinder::GetMethod(METHOD__STREAM__END_WRITE)->GetSlot(); - } + MethodTable* pStreamMT = CoreLibBinder::GetClass(CLASS__STREAM); + WORD slotBeginRead = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_READ)->GetSlot(); + WORD slotEndRead = CoreLibBinder::GetMethod(METHOD__STREAM__END_READ)->GetSlot(); + WORD slotBeginWrite = CoreLibBinder::GetMethod(METHOD__STREAM__BEGIN_WRITE)->GetSlot(); + WORD slotEndWrite = CoreLibBinder::GetMethod(METHOD__STREAM__END_WRITE)->GetSlot(); - // Check the current type and update state. - readOverride = HasOverriddenStreamMethod(pMT, g_slotBeginRead) || HasOverriddenStreamMethod(pMT, g_slotEndRead); - writeOverride = HasOverriddenStreamMethod(pMT, g_slotBeginWrite) || HasOverriddenStreamMethod(pMT, g_slotEndWrite); + // Check the current MethodTable for Stream overrides and set state on the MethodTable. + readOverride = HasOverriddenStreamMethod(pStreamMT, pMT, slotBeginRead) || HasOverriddenStreamMethod(pStreamMT, pMT, slotEndRead); + writeOverride = HasOverriddenStreamMethod(pStreamMT, pMT, slotBeginWrite) || HasOverriddenStreamMethod(pStreamMT, pMT, slotEndWrite); pMT->GetAuxiliaryDataForWrite()->SetStreamOverrideState(readOverride, writeOverride); END_QCALL; diff --git a/src/coreclr/vm/methodtable.h b/src/coreclr/vm/methodtable.h index 9dc9d977793bd8..d4823538b11aa7 100644 --- a/src/coreclr/vm/methodtable.h +++ b/src/coreclr/vm/methodtable.h @@ -369,7 +369,6 @@ struct MethodTableAuxiliaryData enum_flagDebug_IsPublished = 0x2000, enum_flagDebug_ParentMethodTablePointerValid = 0x4000, enum_flagDebug_HasInjectedInterfaceDuplicates = 0x8000, - }; DWORD m_dwFlagsDebug; From 76ee421a02c8002ca28134d1598b3032c11b362d Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 28 Aug 2024 21:43:17 -0700 Subject: [PATCH 3/3] Feedback --- .../System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs | 2 +- src/coreclr/vm/comutilnative.cpp | 2 +- src/coreclr/vm/comutilnative.h | 2 +- src/coreclr/vm/qcallentrypoints.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs index fe0bb26751f754..18e9ca018f4218 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs @@ -8,7 +8,7 @@ namespace System.IO { public abstract unsafe partial class Stream { - [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "StreamNative_HasOverriddenSlow")] + [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Stream_HasOverriddenSlow")] [return: MarshalAs(UnmanagedType.Bool)] private static partial bool HasOverriddenSlow(MethodTable* pMT, [MarshalAs(UnmanagedType.Bool)] bool isRead); diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 056a760cdb2c6d..a4f0eca49a3307 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1894,7 +1894,7 @@ static bool HasOverriddenStreamMethod(MethodTable* streamMT, MethodTable* pMT, W return MethodTable::GetMethodDescForSlotAddress(actual) != MethodTable::GetMethodDescForSlotAddress(base); } -extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL isRead) +extern "C" BOOL QCALLTYPE Stream_HasOverriddenSlow(MethodTable* pMT, BOOL isRead) { QCALL_CONTRACT; _ASSERTE(pMT != NULL); diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 605df8e6063237..a0cea8d190d597 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -266,6 +266,6 @@ extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall: BOOL CanCompareBitsOrUseFastGetHashCode(MethodTable* mt); -extern "C" BOOL QCALLTYPE StreamNative_HasOverriddenSlow(MethodTable* pMT, BOOL isRead); +extern "C" BOOL QCALLTYPE Stream_HasOverriddenSlow(MethodTable* pMT, BOOL isRead); #endif // _COMUTILNATIVE_H_ diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index dc70b76e6db1fc..c594c74f4334d8 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -110,7 +110,7 @@ static const Entry s_QCall[] = DllImportEntry(MethodTable_CanCompareBitsOrUseFastGetHashCode) DllImportEntry(TypeHandle_CanCastTo_NoCacheLookup) DllImportEntry(ValueType_GetHashCodeStrategy) - DllImportEntry(StreamNative_HasOverriddenSlow) + DllImportEntry(Stream_HasOverriddenSlow) DllImportEntry(RuntimeTypeHandle_MakePointer) DllImportEntry(RuntimeTypeHandle_MakeByRef) DllImportEntry(RuntimeTypeHandle_MakeSZArray)