-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Improve tailcallstress testing #41059
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
Improve tailcallstress testing #41059
Conversation
|
runtime-coreclr jitstress: https://dev.azure.com/dnceng/public/_build/results?buildId=778817&view=results |
|
@AndyAyersMS @dotnet/jit-contrib PTAL |
|
@erozenfeld test changes LGTM. |
AndyAyersMS
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.
Thanks, LGTM.
I was thinking we should make tail call stress be modal and perhaps still try and use fast tail calls in some cases (perhaps randomly under jitstress, etc). But that can wait.
1. Dispatch all tail calls under TailCallStress via helpers. That increases our coverage since valid non-tail-prefixed calls are dispatched as fast calls in normal non-stress mode. 2. Don't attempt to tail call from methods that have a localloc unless there is an explicit tail prefix. Note that we already disallowed fast tail calls from such methods so this change only affects tailcallstress mode. 3. Fix a bug in TestInvokeDOPAndCancel. As the test was written this assert was firing under tailcallstress: https://github.com/dotnet/runtime/blob/480c49b2419ab4a0b34bfd86754abc2f17079c77/src/libraries/System.Threading.Tasks.Parallel/tests/ParallelForTests.cs#L1074 When this call to `cts.Cancel`: https://github.com/dotnet/runtime/blob/480c49b2419ab4a0b34bfd86754abc2f17079c77/src/libraries/System.Threading.Tasks.Parallel/tests/ParallelForTests.cs#L1065 is dispatched via helpers, it takes much longer than when it's dispatched via a fast tail call (we have to jit a couple of IL stubs). Because of that, it's possible that all other actions complete on the other thread before cancellation is completed.
d7f1a3e to
25f0a72
Compare
|
I investigated the The stack trace is: #40698 (comment) The test was attempting to launch a thread with a 64kb stack and it was failing with StackOverflow. On my machine it was failing with StackOverflow even without |
|
|
kouvel
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.
Thanks!
Dispatch all tail calls under TailCallStress via helpers. That
increases our coverage since valid non-tail-prefixed calls are
dispatched as fast calls in normal non-stress mode.
Don't attempt to tail call from methods that have a localloc
unless there is an explicit tail prefix. Note that we already disallowed
fast tail calls from such methods so this change only affects
tailcallstress mode.
Fix a bug in TestInvokeDOPAndCancel. As the test was written
this assert was firing under tailcallstress:
runtime/src/libraries/System.Threading.Tasks.Parallel/tests/ParallelForTests.cs
Line 1074 in 480c49b
When this call to
cts.Cancel:runtime/src/libraries/System.Threading.Tasks.Parallel/tests/ParallelForTests.cs
Line 1065 in 480c49b
is dispatched via helpers, it takes much longer than when it's
dispatched via a fast tail call (we have to jit a couple of IL stubs).
Because of that, it's possible that all other actions complete on the
other thread before cancellation is completed.