-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Do not reset size of TailCallArgBuffer #118365
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
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 bug in the tail call buffer implementation where the State field was being incorrectly set using a native integer operation instead of a 32-bit integer operation, causing unintended resets of the Size field and unnecessary buffer re-allocations.
Key changes:
- Changes
EmitSTIND_I()
toEmitSTIND_I4()
when setting the TailCallArgBuffer state to ensure only the 32-bit State field is modified
Tagging subscribers to this area: @mangod9 |
This should fix the stress problem hit by the CI. I have noticed that the thread in the bad state is always in the CFG probe called from There is still a corner case stress bug hidden somewhere. It is likely specific to combination of WinServer 2016 w/ CFG (and potentially Debug CRT). I suspect that it may be in special casing of CFG inside SetThreadContext in the OS. It should be rare enough that we can live with it. |
TailCallArgBuffer:State is 32-bit int. The code incorrectly used native int operation to set. It lead to TailCallArgBuffer::Size to be reset as well and needlessly re-allocating the tail call buffer. Fixes dotnet#118166
I have realized that we can call malloc/free in pre-emptive mode to make this even more robust, going to push an update in a bit. |
…ocation Move the fast path of the TailCallArgBuffer allocation helper to C# to avoid perf overload of switching to preemptive mode. Also, aggressively inline it to allow zero initialization to be unrolled by the JIT.
This is ready for review again |
TailCallArgBuffer:State is 32-bit int. The code incorrectly used native int operation to set. It lead to TailCallArgBuffer::Size to be reset as well and needlessly re-allocating the tail call buffer.
Fixes #118166