Skip to content

Commit

Permalink
Improve tailcallstress testing (#41059)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
erozenfeld authored Aug 24, 2020
1 parent c6b4af3 commit ea1d20c
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 4 deletions.
16 changes: 15 additions & 1 deletion src/coreclr/src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6774,6 +6774,12 @@ bool Compiler::fgCanFastTailCall(GenTreeCall* callee, const char** failReason)
return false;
}

if (callee->IsStressTailCall())
{
reportFastTailCallDecision("Fast tail calls are not performed under tail call stress");
return false;
}

// Note on vararg methods:
// If the caller is vararg method, we don't know the number of arguments passed by caller's caller.
// But we can be sure that in-coming arg area of vararg caller would be sufficient to hold its
Expand Down Expand Up @@ -7226,6 +7232,14 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
// is set. This avoids the need for iterating through all lcl vars of the current
// method. Right now throughout the code base we are not consistently using 'set'
// method to set lvHasLdAddrOp and lvAddrExposed flags.

bool isImplicitOrStressTailCall = call->IsImplicitTailCall() || call->IsStressTailCall();
if (isImplicitOrStressTailCall && compLocallocUsed)
{
failTailCall("Localloc used");
return nullptr;
}

bool hasStructParam = false;
for (unsigned varNum = 0; varNum < lvaCount; varNum++)
{
Expand All @@ -7235,7 +7249,7 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call)
// We still must check for any struct parameters and set 'hasStructParam'
// so that we won't transform the recursive tail call into a loop.
//
if (call->IsImplicitTailCall() || call->IsStressTailCall())
if (isImplicitOrStressTailCall)
{
if (varDsc->lvHasLdAddrOp && !lvaIsImplicitByRefLocal(varNum))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,7 @@ public static void TestInvokeDOPAndCancel()
{
int newVal = Interlocked.Increment(ref counter);
if (newVal == 1) throw new Exception("some non-cancellation-related exception");
if (newVal == 2) cts.Cancel();
if (newVal >= 2) cts.Cancel();
};
for (int i = 0; i < numActions; i++) actions[i] = a3;

Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Threading.Thread/tests/ThreadTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ public static class ThreadTests
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
public static void ConstructorTest()
{
const int SmallStackSizeBytes = 64 << 10; // 64 KB, currently accepted in all supported platforms, and is the PAL minimum
const int LargeStackSizeBytes = 16 << 20; // 16 MB
const int SmallStackSizeBytes = 128 << 10; // 128 KB
const int LargeStackSizeBytes = 16 << 20; // 16 MB

int pageSizeBytes = Environment.SystemPageSize;

Expand Down

0 comments on commit ea1d20c

Please sign in to comment.