-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Misc cleanup of VM threading code #108171
Conversation
jkotas
commented
Sep 24, 2024
- Delete special-casing of thread finalizer
- Factor out Get/SetApartment of unstarted threads
- Delete some dead code
- Fix comments
- Delete special-casing of thread finalizer - Factor out Get/SetApartment of unstarted threads - Delete some dead code - Fix comments
Tagging subscribers to this area: @mangod9 |
{ | ||
_mayNeedResetForThreadPool = true; | ||
} | ||
_mayNeedResetForThreadPool = true; |
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.
This was not handling the case of finalizer threads.
@@ -14,9 +14,6 @@ namespace System.Threading | |||
{ | |||
public sealed partial class Thread | |||
{ | |||
[ThreadStatic] |
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.
Dead code
LOG((LF_GC, LL_INFO10, "Finalizer thread starting...\n")); | ||
|
||
#if defined(FEATURE_COMINTEROP_APARTMENT_SUPPORT) && !defined(FEATURE_COMINTEROP) |
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.
The condition was always false.
@@ -4799,6 +4770,9 @@ Thread::ApartmentState Thread::SetApartment(ApartmentState state) | |||
} | |||
CONTRACTL_END; | |||
|
|||
// This method can be called on current thread only |
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.
Fixed the non-current thread callers to use SetApartmentOfUnstartedThread
Motivated by HMF->QCall conversion codereviews |
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
@jkotas, all test legs failed in runtime-coreclr libraries-jitstress/20240924.1. PTAL. |
I have looked at some of the failures. The failures are either asserts in the JIT
Or asserts/crashes in libraries that suggest bad codegen: |
Similar set of failures was hit in https://dev.azure.com/dnceng-public/public/_build/results?buildId=815402&view=ms.vss-test-web.build-test-results-tab that did not include this change. |
@EgorBo, PTAL. |
These failures should be fixed by #108169 |
- Delete special-casing of thread finalizer - Factor out Get/SetApartment of unstarted threads - Delete some dead code - Fix comments