-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[MonoVM] Reduce P/Invoke GC transition asserts in release builds #59029
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.
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
[MonoVM] Reduce P/Invoke GC transition asserts in release builds #59029
Conversation
|
Tagging subscribers to this area: Issue DetailsProfiling shows that large part of the GC transition overhead (~30%) in #58939 is caused by assert-style checks. Disabling them seems to be the best bang-for-the-buck option for reducing the overhead without fundamental changes to the code.
|
| static void | ||
| check_thread_state (MonoThreadInfo* info) | ||
| { | ||
| #ifdef ENABLE_CHECKED_BUILD_THREAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filipnavara I think this one should be ENABLE_CHECKED_BUILD_GC - I think other thread state machine stuff in "GC", too. (for example assert_gc_safe_mode).
I think we should also turn on the GC checked build in debug builds:
https://github.com/dotnet/runtime/blob/main/src/mono/CMakeLists.txt#L694-L695
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. But to be honest I can't tell the difference - looks like we use both defines for closely related things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly tried to match the asserts in the same file but I can change it. I will update the PR to enable it on debug builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking how the configuration options are set I came to the conclusion to change it to the general ENABLE_CHECKED_BUILD. That one is defined in all debug builds, the other ones can be selectively enabled.
|
On Mac Catalyst ARM64 the test case in #58939 goes from |
|
will you consider to backport to release/6.0? |
|
/backport to release/6.0-rc2 |
|
Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1246192307 |
…elease builds (#59269) Backport of #59029 Profiling shows that large part of the GC transition overhead (~30%) in #58939 is caused by assert-style checks. Disabling them seems to be the best bang-for-the-buck option for reducing the overhead without fundamental changes to the code. Co-authored-by: Filip Navara <[email protected]>
Profiling shows that large part of the GC transition overhead (~30%) in #58939 is caused by assert-style checks. Disabling them seems to be the best bang-for-the-buck option for reducing the overhead without fundamental changes to the code.