Skip to content

Recover some lost EH performance on Unix amd64 #90033

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

Merged
janvorli merged 5 commits into from
Aug 6, 2023
Merged

Recover some lost EH performance on Unix amd64 #90033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2453a3e
Improve EH performance on Unix amd64
janvorli Aug 4, 2023
65f3e4d
Fix gcc build and size for non-xstate case
janvorli Aug 4, 2023
0b11601
Fix yet another gcc break
janvorli Aug 4, 2023
d506672
Fix build break
janvorli Aug 5, 2023
9088b7a
Revert the gcc fixes and disable the warning
janvorli Aug 5, 2023
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
3 changes: 1 addition & 2 deletions eng/native/configurecompiler.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,14 +598,13 @@ if (CLR_CMAKE_HOST_UNIX)
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-Wno-stringop-overflow>)
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-Wno-restrict>)
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:-Wno-stringop-truncation>)
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-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($<$<COMPILE_LANGUAGE:CXX>:-Wno-placement-new>)

add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-Wno-class-memaccess>)
endif()

if (CMAKE_CXX_COMPILER_ID)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};

//----------------------------------------------------------------------------
//
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/pal/inc/pal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1531,6 +1531,14 @@ typedef struct _XMM_SAVE_AREA32 {

typedef struct DECLSPEC_ALIGN(16) _CONTEXT {

_CONTEXT() = default;

@jkotas jkotas Aug 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change goes against the spirit of the Win32 PAL. The Win32 PAL is meant to mirror the Windows SDK definitions. The Windows SDK definitions of this structure is POD struct without any C++. This makes it C++ struct and behave differently from the Windows SDK definitions that can lead to all sort of surprises.

@janvorli janvorli Aug 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jkotas the proper fix in the spirit of the Win32 PAL would be to implement CopyContext (and maybe InitializeContext too) and modify the whole codebase to use that instead of pure assignments of the CONTEXT structure. That sounded like too extensive change given the number of places we just assign CONTEXT structs by value in the runtime and the time I had left before the RC1 snapshot.

I'd be ok reverting this PR though and going for the cleaner solution if you prefer.

@jkotas jkotas Aug 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you are ok with this, I am ok with it too. I wanted to point this difference out since it is likely going to take somebody by surprise at some point.

If you would like to clean it up, it can wait for .NET 9.

@janvorli janvorli Aug 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great, I'll leave this checked in and clean it up in 9 then.

_CONTEXT(const _CONTEXT& ctx)
{
*this = ctx;
}

_CONTEXT& operator=(const _CONTEXT& ctx);

//
// Register parameter home addresses.
//
Expand Down
26 changes: 26 additions & 0 deletions src/coreclr/pal/src/thread/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, XStateFeaturesMask);
}

memcpy(this, &ctx, copySize);

@jakobbotsch jakobbotsch Aug 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it have better performance characteristics to do memcpy with a constant in each of the arms above? (Alternatively, copy the first part unconditionally, then the additional parts only conditionally)? I have not looked at codegen, and maybe the C++ compilers already do this optimization, but memcpy with a constant length is usually handled much more efficiently by the compilers.

@janvorli janvorli Aug 4, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had it that way originally (memcpy in each of the arms), but the compiler actually did compute the size first and then called memcpy once. I liked that, so I've transformed the source to the same way.


return *this;
}
#endif // HOST_AMD64
2 changes: 1 addition & 1 deletion src/coreclr/vm/exceptionhandling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,7 +1286,7 @@ bool FixNonvolatileRegisters(UINT_PTR uOriginalSP,
}
CONTRACTL_END;

CONTEXT _ctx = {0};
CONTEXT _ctx = {};

// Ctor will initialize it to NULL
REGDISPLAY regdisp;
Expand Down