-
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
dont try to capture threadId for NativeAOT #108045
Conversation
Tagging subscribers to this area: @dotnet/gc |
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.
LGTM and thanks so much!
@@ -26685,7 +26685,7 @@ void gc_heap::add_to_hc_history_worker (hc_history* hist, int* current_index, hc | |||
current_hist->concurrent_p = (bool)settings.concurrent; | |||
current_hist->bgc_thread_running = (bool)bgc_thread_running; | |||
|
|||
#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) && !defined(_DEBUG) | |||
#if defined(TARGET_AMD64) && defined(TARGET_WINDOWS) && !defined(_DEBUG) && !defined(FEATURE_NATIVEAOT) | |||
int bgc_thread_os_id = 0; | |||
|
|||
if (bgc_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.
This magic 0x130 constant is going to break in number of different scenarios. For example, mix and match standalone GC scenarios are going to break too.
How long do we expect to keep it?
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'm planning to make this into a GCToEEInterface call in main and possibly backport to 9.0 in a servicing release.
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 think we should do the proper change now.
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'm only intending to use this with 9.0 so I'm fine not supporting mix/match standalone GC. other than coreclr actually changing the Thread object, how else can this break?
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.
should we also consider instrumenting under a special config?
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.
yep, we can put this under a config.
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.
ok I can add that, just this piece or the entire instrumentation change?
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 would only do the config for this. thanks!
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.
added a config check for it.
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.
LGTM.
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10967167214 |
/backport to release/9.0-rc2 |
Started backporting to release/9.0-rc2: https://github.com/dotnet/runtime/actions/runs/10967792225 |
* dont try to capture threadId for NativeAOT * add config to capture bgc threadid
Fix an issue while collecting threadId which might not work for NativeAOT. This was included as part of the recent instrumentation changes.