Improve windows hardware exception handling performance #74426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With my recent change that modified hardware exception handling so that the related managed exception is thrown directly from the vectored exception handler, the performance of handling such exceptions have regressed. Several exception handling dotnet/performance microbenchmarks have regressed upto 15%. The reason for the regression was the larger number of stack frames between the exception raising and the actual handler frame. With a recent change that @AntonLapounov has made to fix process corrupting exceptions handling, the regression went down to 8%. This change moves the location where we raise the exception down to the ClrVectoredExceptionHandlerShim, which means to the closest possible frame to the managed code. This gets rid of the regression completely.
src/coreclr/vm/excep.cpp
Outdated
| } | ||
|
|
||
| LONG retVal = 0; | ||
| VEH_ACTION retVal = VEH_NO_ACTION; |
There was a problem hiding this comment.
Nit: We could get rid of this local variable or move down this definition.
AntonLapounov
approved these changes
Aug 23, 2022
| @@ -240,7 +240,16 @@ VOID DECLSPEC_NORETURN RealCOMPlusThrowOM(); | |||
|
|
|||
| #endif // !defined(FEATURE_EH_FUNCLETS) | |||
|
|
|||
| LONG WINAPI CLRVectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo); | |||
| enum VEH_ACTION | |||
There was a problem hiding this comment.
Do these really need to be in the header? Adding definitions to the header implies they are used in other places whereas this are not.
There was a problem hiding this comment.
Thanks @AaronRobinsonMSFT, it is actually used in two sources, the excep.cpp and excepx86.cpp. However, I have forgotten to modify the excepx86.cpp to get the result of the CLRVectoredExceptionHandler call into VEH_ACTION, it was still using DWORD. I am updating the change.
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2927798654 |
|
@janvorli backporting to release/7.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Improve windows hardware exception handling performance
Using index info to reconstruct a base tree...
M src/coreclr/vm/excep.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/excep.cpp
CONFLICT (content): Merge conflict in src/coreclr/vm/excep.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Improve windows hardware exception handling performance
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
janvorli
added a commit
to janvorli/runtime
that referenced
this pull request
Aug 25, 2022
Port of dotnet#74426 to release/7.0 * Improve windows hardware exception handling performance With my recent change that modified hardware exception handling so that the related managed exception is thrown directly from the vectored exception handler, the performance of handling such exceptions have regressed. Several exception handling dotnet/performance microbenchmarks have regressed upto 15%. The reason for the regression was the larger number of stack frames between the exception raising and the actual handler frame. With a recent change that @AntonLapounov has made to fix process corrupting exceptions handling, the regression went down to 8%. This change moves the location where we raise the exception down to the ClrVectoredExceptionHandlerShim, which means to the closest possible frame to the managed code. This gets rid of the regression completely. Close dotnet#71069
carlossanlop
pushed a commit
that referenced
this pull request
Aug 25, 2022
…#74590) Port of #74426 to release/7.0 * Improve windows hardware exception handling performance With my recent change that modified hardware exception handling so that the related managed exception is thrown directly from the vectored exception handler, the performance of handling such exceptions have regressed. Several exception handling dotnet/performance microbenchmarks have regressed upto 15%. The reason for the regression was the larger number of stack frames between the exception raising and the actual handler frame. With a recent change that @AntonLapounov has made to fix process corrupting exceptions handling, the regression went down to 8%. This change moves the location where we raise the exception down to the ClrVectoredExceptionHandlerShim, which means to the closest possible frame to the managed code. This gets rid of the regression completely. Close #71069
|
Improvements on: |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With my recent change that modified hardware exception handling so that
the related managed exception is thrown directly from the vectored
exception handler, the performance of handling such exceptions have
regressed. Several exception handling dotnet/performance microbenchmarks
have regressed upto 15%.
The reason for the regression was the larger number of stack frames
between the exception raising and the actual handler frame. With a
recent change that @AntonLapounov has made to fix process corrupting
exceptions handling, the regression went down to 8%. This change moves
the location where we raise the exception down to the
ClrVectoredExceptionHandlerShim, which means to the closest possible
frame to the managed code.
This gets rid of the regression completely.
Close #71069