-
Notifications
You must be signed in to change notification settings - Fork 5k
[NativeAOT] Asynchronous thread suspension at GC safe points on Windows #70316
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
Conversation
if (!frameIterator.IsValid()) | ||
return false; | ||
|
||
frameIterator.CalculateCurrentMethodState(); |
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.
Why call CalculateCurrentMethodState
? frameIterator
is not used after this point.
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.
It was copied from InternalHijack
initially. I now think the entire frameIterator
deal is unnecessary here.
Hmm. tests seem to pass locally. |
You may want to think about how to stress test this before it gets merged. |
It was quite reproducible and simple and library tests catch the issues (I only ran smoke tests locally, so I did not see). GC decoder expects an off-by-one skew for nonleaf frames and we used to apply the skew unconditionally. Now that we have leaf frames to scan, we need to be more intentional with that. |
Running library tests in a loop seems to be a good stress for this feature. The tests themselves and xunit present all kinds of situations for stackwalks. I have run the entire set 30 times in Release and Debug each. Then we hit an assert in:
I think the assert is bogus. We are in native code in coop mode, we certainly can scan our own stack. Besides, suspension may happen right after we check anyways. I am going to remove the assert. Otherwise, I think this PR is good to merge. |
Also This is fairly rare (I saw it just once) and completely unrelated to this change since it is entirely in managed code. |
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.
Thank you!
Thanks!! |
@LakshanF hangs in GC suspension should be fixed on Windows x64 now. |
Support for asynchronous suspension at GC safe points on Windows.
Contributes to: #67805