-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix GetThreadStaticsBase to be included in R2R images #123812
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
Fix GetThreadStaticsBase to be included in R2R images #123812
Conversation
…LocalData This change adds a new JIT helper to handle thread static access for DirectOnThreadLocalData in ReadyToRun images. The new helper returns the address of the thread local data structure, which is used to access the pNativeThread field. This eliminates the need for the BypassReadyToRun attribute on GetThreadStaticsBase, allowing it to be included in R2R images and improving performance on platforms like iOS that rely on composite-r2r with interpreter fallback. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
…ALBASE Update the CrossGen2 ReadyToRun compiler to handle the new CORINFO_HELP_READYTORUN_THREADLOCALBASE helper. This helper is used to get the address of the thread local data structure for DirectOnThreadLocalData. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Add the new GetThreadLocalBase helper to the ReadyToRunHelper enum and map it to the CORINFO_HELP_READYTORUN_THREADLOCALBASE helper in the ReadyToRun constants and helper mapping tables. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Corrected the comment to accurately state that the helper returns the address of the thread local data structure, not just the pThread field. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
davidwrighton
left a comment
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.
@copilot, please adjust the change to respond to this feedback.
src/coreclr/tools/Common/Internal/Runtime/ReadyToRunConstants.cs
Outdated
Show resolved
Hide resolved
…helper - Changed CORINFO_HELP_READYTORUN_THREADLOCALBASE to CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASE as a normal JIT helper - Renamed JIT_GetThreadLocalBase to JIT_GetDirectOnThreadLocalDataNonGCThreadStaticBase - Fixed implementation to return address of pThread field instead of entire ThreadLocalData structure - Reverted all ReadyToRun constant changes (readytorun.h, readytorunhelpers.h, ReadyToRunConstants.cs) - Removed ReadyToRun compiler awareness of this helper Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Added the CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASE helper definition that was missing from jithelpers.h, causing enum mismatch errors. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
JIT_GetDirectOnThreadLocalDataNonGCThreadStaticBase should be a JITHELPER, not a DYNAMICJITHELPER. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
The CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASE helper should not be used during R2R compilation and should throw RequiresRuntimeJitException. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Addressed all feedback in commits 6388684, f699f05, 5964bd5, and c6772e4. Changes made:
|
Added CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASE to the CorInfoHelpFunc enum in the tools directory. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Made the comment more precise to clarify that this helper returns the address of the pThread field which serves as the storage location for DirectOnThreadLocalData.pNativeThread thread static field. Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
…://github.com/dotnet/runtime into copilot/fix-getthreadstaticsbase-in-r2r-image
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.
Pull request overview
This PR fixes a critical performance issue where GetThreadStaticsBase bypassed ReadyToRun compilation, causing 35x performance degradation on iOS with composite-r2r and interpreter fallback. The solution introduces a new JIT helper that provides direct access to thread-local storage for the DirectOnThreadLocalData type, enabling R2R compilation of GetThreadStaticsBase.
Changes:
- Added
CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASEJIT helper with implementationJIT_GetDirectOnThreadLocalDataNonGCThreadStaticBaseto return the address of the pThread field in thread-local storage - Modified
getHelperForStaticBasein prestub.cpp to detect DirectOnThreadLocalData MethodTable and return the new helper for non-GC thread static base fixups - Removed
BypassReadyToRunAttributefromGetThreadStaticsBasein Thread.CoreCLR.cs to enable R2R compilation - Updated JIT interface version GUID to reflect the interface change
- Added the new helper to RequiresRuntimeJitException list in ReadyToRun compiler to ensure it's used at runtime, not during compilation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/prestub.cpp | Added special case detection for DirectOnThreadLocalData to use the new helper during R2R fixup resolution |
| src/coreclr/vm/jithelpers.cpp | Implemented the new helper function that returns the address of pThread field in thread-local storage |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Added new helper to RequiresRuntimeJitException list to prevent R2R compilation of the helper itself |
| src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs | Added new helper enum value to match corinfo.h |
| src/coreclr/inc/jithelpers.h | Defined the new helper as a JITHELPER mapped to the implementation function |
| src/coreclr/inc/jiteeversionguid.h | Updated JIT interface version GUID to reflect the interface change |
| src/coreclr/inc/corinfo.h | Added new CorInfoHelpFunc enum value for the DirectOnThreadLocalData helper |
| src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs | Removed BypassReadyToRunAttribute and associated comment to enable R2R compilation of GetThreadStaticsBase |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…foImpl.ReadyToRun.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
GetThreadStaticsBasewas bypassing ReadyToRun compilation to avoid recursive initialization dependency. This caused all thread static field access on iOS (composite-r2r with interpreter fallback) to execute through the interpreter, resulting in 35x performance degradation.Changes
Added
CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASEhelper as a normal JIT helper to enable R2R compilation ofGetThreadStaticsBase:JIT_GetDirectOnThreadLocalDataNonGCThreadStaticBasereturns&((ThreadLocalData*)&t_ThreadStatics)->pThread- the address of the pThread field within thread-local storage, which serves as the storage location for DirectOnThreadLocalData.pNativeThreadgetHelperForStaticBaseto detectDirectOnThreadLocalDataMethodTable and use the new helper forREADYTORUN_FIXUP_ThreadStaticBaseNonGCBypassReadyToRunAttributefromGetThreadStaticsBaseThe new helper provides direct access to the pThread field in thread local storage without triggering recursive initialization, allowing
GetThreadStaticsBaseto be compiled into R2R images.Testing
Microsoft Reviewers
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.