From 2453a3e01c5500c182bcddf5ed40e013f519064c Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 4 Aug 2023 18:44:02 +0200 Subject: [PATCH 1/5] Improve EH performance on Unix amd64 Few months ago, a change to enable AVX512 on Unix was merged in. That change has enlarged the CONTEXT structure significantly and it was found that it has caused 6..17% regressions in exception handling microbenchmarks. This change gets some of the lost performance back by adding custom copy constructor and assignment operator to the CONTEXT structure and preventing copying of the AVX / AVX512 stuff if the source context doesn't have it. I have observed 3..9% gain on my machine. Close #84308 --- src/coreclr/debug/ee/debugger.cpp | 2 +- src/coreclr/pal/inc/pal.h | 8 ++++++++ src/coreclr/pal/src/thread/context.cpp | 26 ++++++++++++++++++++++++++ src/coreclr/vm/exceptionhandling.cpp | 2 +- 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 073d758c81b87c..7581c0c68311a9 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -6539,7 +6539,7 @@ HRESULT Debugger::LaunchDebuggerForUser(Thread * pThread, EXCEPTION_POINTERS * p // once and leave them set without the risk of clobbering something we care about. JIT_DEBUG_INFO Debugger::s_DebuggerLaunchJitInfo = {0}; EXCEPTION_RECORD Debugger::s_DebuggerLaunchJitInfoExceptionRecord = {0}; -CONTEXT Debugger::s_DebuggerLaunchJitInfoContext = {0}; +CONTEXT Debugger::s_DebuggerLaunchJitInfoContext = {}; //---------------------------------------------------------------------------- // diff --git a/src/coreclr/pal/inc/pal.h b/src/coreclr/pal/inc/pal.h index 7bc67f1f805a36..92455bc8a491b0 100644 --- a/src/coreclr/pal/inc/pal.h +++ b/src/coreclr/pal/inc/pal.h @@ -1531,6 +1531,14 @@ typedef struct _XMM_SAVE_AREA32 { typedef struct DECLSPEC_ALIGN(16) _CONTEXT { + _CONTEXT() = default; + _CONTEXT(const _CONTEXT& ctx) + { + *this = ctx; + } + + _CONTEXT& operator=(const _CONTEXT& ctx); + // // Register parameter home addresses. // diff --git a/src/coreclr/pal/src/thread/context.cpp b/src/coreclr/pal/src/thread/context.cpp index 07afeea40d486c..425970b12e8489 100644 --- a/src/coreclr/pal/src/thread/context.cpp +++ b/src/coreclr/pal/src/thread/context.cpp @@ -1880,3 +1880,29 @@ DBG_FlushInstructionCache( #endif return TRUE; } + +#ifdef HOST_AMD64 +CONTEXT& CONTEXT::operator=(const CONTEXT& ctx) +{ + size_t copySize; + if (ctx.ContextFlags & CONTEXT_XSTATE & CONTEXT_AREA_MASK) + { + if ((ctx.XStateFeaturesMask & XSTATE_MASK_AVX512) == XSTATE_MASK_AVX512) + { + copySize = sizeof(CONTEXT); + } + else + { + copySize = offsetof(CONTEXT, KMask0); + } + } + else + { + copySize = offsetof(CONTEXT, Ymm0H); + } + + memcpy(this, &ctx, copySize); + + return *this; +} +#endif // HOST_AMD64 diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 2e93bebc8ccf6c..cec1175c9c022e 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -1286,7 +1286,7 @@ bool FixNonvolatileRegisters(UINT_PTR uOriginalSP, } CONTRACTL_END; - CONTEXT _ctx = {0}; + CONTEXT _ctx = {}; // Ctor will initialize it to NULL REGDISPLAY regdisp; From 65f3e4d88634b827f4e1910839838aab799fc49d Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 4 Aug 2023 21:11:46 +0200 Subject: [PATCH 2/5] Fix gcc build and size for non-xstate case --- src/coreclr/pal/src/exception/seh-unwind.cpp | 2 +- src/coreclr/pal/src/thread/context.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index 8afe1ffb3db6fa..0ef8f04d43f9bb 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -939,7 +939,7 @@ RaiseException(IN DWORD dwExceptionCode, } // Capture the context of RaiseException. - ZeroMemory(contextRecord, sizeof(CONTEXT)); + new (contextRecord) CONTEXT(); contextRecord->ContextFlags = CONTEXT_FULL; CONTEXT_CaptureContext(contextRecord); diff --git a/src/coreclr/pal/src/thread/context.cpp b/src/coreclr/pal/src/thread/context.cpp index 425970b12e8489..a9ed8c269a89c7 100644 --- a/src/coreclr/pal/src/thread/context.cpp +++ b/src/coreclr/pal/src/thread/context.cpp @@ -1898,7 +1898,7 @@ CONTEXT& CONTEXT::operator=(const CONTEXT& ctx) } else { - copySize = offsetof(CONTEXT, Ymm0H); + copySize = offsetof(CONTEXT, XStateFeaturesMask); } memcpy(this, &ctx, copySize); From 0b11601902236530659a3fa26d8ca3412f106d43 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 4 Aug 2023 21:32:50 +0200 Subject: [PATCH 3/5] Fix yet another gcc break --- src/coreclr/pal/src/thread/context.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/pal/src/thread/context.cpp b/src/coreclr/pal/src/thread/context.cpp index a9ed8c269a89c7..ea03b3546a57b8 100644 --- a/src/coreclr/pal/src/thread/context.cpp +++ b/src/coreclr/pal/src/thread/context.cpp @@ -495,7 +495,7 @@ CONTEXT_GetThreadContext( ERROR("GetThreadContext on a thread other than the current " "thread is returning TRUE\n"); flags = lpContext->ContextFlags; - memset(lpContext, 0, sizeof(*lpContext)); + new (lpContext) CONTEXT(); lpContext->ContextFlags = flags; ret = TRUE; goto EXIT; From d5066722a0eb520db9690ce2309acd1840c58e89 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 5 Aug 2023 21:51:13 +0200 Subject: [PATCH 4/5] Fix build break --- src/coreclr/pal/src/thread/context.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/pal/src/thread/context.cpp b/src/coreclr/pal/src/thread/context.cpp index ea03b3546a57b8..b9d3d1bd1d5cd0 100644 --- a/src/coreclr/pal/src/thread/context.cpp +++ b/src/coreclr/pal/src/thread/context.cpp @@ -29,6 +29,7 @@ SET_DEFAULT_DEBUG_CHANNEL(THREAD); // some headers have code with asserts, so do #endif #include #include +#include extern PGET_GCMARKER_EXCEPTION_CODE g_getGcMarkerExceptionCode; From 9088b7ab86b3bb4df265b80886d60db49fbb1884 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 5 Aug 2023 23:38:40 +0200 Subject: [PATCH 5/5] Revert the gcc fixes and disable the warning The zeroing of CONTEXT structure is used at too many places and it is valid, so rather than trying to modify all the places by using placement new, just disable the warning for gcc. --- eng/native/configurecompiler.cmake | 3 +-- src/coreclr/pal/src/exception/seh-unwind.cpp | 2 +- src/coreclr/pal/src/thread/context.cpp | 3 +-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index a10297b7d6c666..183811018533aa 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -598,14 +598,13 @@ if (CLR_CMAKE_HOST_UNIX) add_compile_options($<$:-Wno-stringop-overflow>) add_compile_options($<$:-Wno-restrict>) add_compile_options($<$:-Wno-stringop-truncation>) + add_compile_options($<$:-Wno-class-memaccess>) if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 12.0) # this warning is only reported by g++ 11 in debug mode when building # src/coreclr/vm/stackingallocator.h. It is a false-positive, fixed in g++ 12. # see: https://github.com/dotnet/runtime/pull/69188#issuecomment-1136764770 add_compile_options($<$:-Wno-placement-new>) - - add_compile_options($<$:-Wno-class-memaccess>) endif() if (CMAKE_CXX_COMPILER_ID) diff --git a/src/coreclr/pal/src/exception/seh-unwind.cpp b/src/coreclr/pal/src/exception/seh-unwind.cpp index 0ef8f04d43f9bb..8afe1ffb3db6fa 100644 --- a/src/coreclr/pal/src/exception/seh-unwind.cpp +++ b/src/coreclr/pal/src/exception/seh-unwind.cpp @@ -939,7 +939,7 @@ RaiseException(IN DWORD dwExceptionCode, } // Capture the context of RaiseException. - new (contextRecord) CONTEXT(); + ZeroMemory(contextRecord, sizeof(CONTEXT)); contextRecord->ContextFlags = CONTEXT_FULL; CONTEXT_CaptureContext(contextRecord); diff --git a/src/coreclr/pal/src/thread/context.cpp b/src/coreclr/pal/src/thread/context.cpp index b9d3d1bd1d5cd0..a9ed8c269a89c7 100644 --- a/src/coreclr/pal/src/thread/context.cpp +++ b/src/coreclr/pal/src/thread/context.cpp @@ -29,7 +29,6 @@ SET_DEFAULT_DEBUG_CHANNEL(THREAD); // some headers have code with asserts, so do #endif #include #include -#include extern PGET_GCMARKER_EXCEPTION_CODE g_getGcMarkerExceptionCode; @@ -496,7 +495,7 @@ CONTEXT_GetThreadContext( ERROR("GetThreadContext on a thread other than the current " "thread is returning TRUE\n"); flags = lpContext->ContextFlags; - new (lpContext) CONTEXT(); + memset(lpContext, 0, sizeof(*lpContext)); lpContext->ContextFlags = flags; ret = TRUE; goto EXIT;