Skip to content

Commit

Permalink
Improve tailcallstress testing
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 committed Aug 21, 2020
1 parent 26a71f9 commit 25f0a72
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 25f0a72

Please sign in to comment.