Merge topic 'ctest-loop-idle'

086a41c0f3 cmCTestMultiProcessHandler: Simplify test startup batching
e528cd795f cmCTestMultiProcessHandler: Start new tests asynchronously
9d8415c17b cmCTestMultiProcessHandler: Clarify test-load retry timer infrastructure
61e98ca33b cmCTestMultiProcessHandler: Factor out loop startup and teardown
5ff0b4ed57 cmCTestMultiProcessHandler: Consolidate test readiness checks
ad3df3ce4d cmCTestMultiProcessHandler: Exclude dependent tests earlier
3c4767f467 cmCTestTestHandler: Clarify name of member storing RESOURCE_LOCK property

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: buildbot <buildbot@kitware.com>
Merge-request: !8995

6 files changed, 94 insertions, 81 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index bfdec9f..a9930df 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -125,6 +125,20 @@ bool cmCTestMultiProcessHandler::Complete()
   return this->Completed == this->Total;
 }
 
+void cmCTestMultiProcessHandler::InitializeLoop()
+{
+  this->Loop.init();
+  this->StartNextTestsOnIdle_.init(*this->Loop, this);
+  this->StartNextTestsOnTimer_.init(*this->Loop, this);
+}
+
+void cmCTestMultiProcessHandler::FinalizeLoop()
+{
+  this->StartNextTestsOnTimer_.reset();
+  this->StartNextTestsOnIdle_.reset();
+  this->Loop.reset();
+}
+
 void cmCTestMultiProcessHandler::RunTests()
 {
   this->CheckResume();
@@ -133,10 +147,10 @@ void cmCTestMultiProcessHandler::RunTests()
   }
   this->TestHandler->SetMaxIndex(this->FindMaxIndex());
 
-  this->Loop.init();
-  this->StartNextTests();
+  this->InitializeLoop();
+  this->StartNextTestsOnIdle();
   uv_run(this->Loop, UV_RUN_DEFAULT);
-  this->Loop.reset();
+  this->FinalizeLoop();
 
   if (!this->StopTimePassed && !this->CheckStopOnFailure()) {
     assert(this->Complete());
@@ -148,7 +162,7 @@ void cmCTestMultiProcessHandler::RunTests()
   this->UpdateCostData();
 }
 
-bool cmCTestMultiProcessHandler::StartTestProcess(int test)
+void cmCTestMultiProcessHandler::StartTestProcess(int test)
 {
   if (this->HaveAffinity && this->Properties[test]->WantAffinity) {
     size_t needProcessors = this->GetProcessorsUsed(test);
@@ -227,7 +241,7 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
     e << "Resource spec file:\n\n  " << this->ResourceSpecFile;
     cmCTestRunTest::StartFailure(std::move(testRun), this->Total, e.str(),
                                  "Insufficient resources");
-    return false;
+    return;
   }
 
   cmWorkingDirectory workdir(this->Properties[test]->Directory);
@@ -237,13 +251,12 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test)
                                    this->Properties[test]->Directory + " : " +
                                    std::strerror(workdir.GetLastResult()),
                                  "Failed to change working directory");
-    return false;
+    return;
   }
 
   // Ownership of 'testRun' has moved to another structure.
   // When the test finishes, FinishTestProcess will be called.
-  return cmCTestRunTest::StartTest(std::move(testRun), this->Completed,
-                                   this->Total);
+  cmCTestRunTest::StartTest(std::move(testRun), this->Completed, this->Total);
 }
 
 bool cmCTestMultiProcessHandler::AllocateResources(int index)
@@ -401,8 +414,8 @@ void cmCTestMultiProcessHandler::SetStopTimePassed()
 void cmCTestMultiProcessHandler::LockResources(int index)
 {
   auto* properties = this->Properties[index];
-  this->LockedResources.insert(properties->LockedResources.begin(),
-                               properties->LockedResources.end());
+  this->ProjectResourcesLocked.insert(properties->ProjectResources.begin(),
+                                      properties->ProjectResources.end());
   if (properties->RunSerial) {
     this->SerialTestRunning = true;
   }
@@ -411,8 +424,8 @@ void cmCTestMultiProcessHandler::LockResources(int index)
 void cmCTestMultiProcessHandler::UnlockResources(int index)
 {
   auto* properties = this->Properties[index];
-  for (std::string const& i : properties->LockedResources) {
-    this->LockedResources.erase(i);
+  for (std::string const& i : properties->ProjectResources) {
+    this->ProjectResourcesLocked.erase(i);
   }
   if (properties->RunSerial) {
     this->SerialTestRunning = false;
@@ -447,47 +460,20 @@ std::string cmCTestMultiProcessHandler::GetName(int test)
   return this->Properties[test]->Name;
 }
 
-bool cmCTestMultiProcessHandler::StartTest(int test)
+void cmCTestMultiProcessHandler::StartTest(int test)
 {
-  // Check for locked resources
-  for (std::string const& i : this->Properties[test]->LockedResources) {
-    if (cm::contains(this->LockedResources, i)) {
-      return false;
-    }
-  }
-
-  if (!this->AllocateResources(test)) {
-    return false;
-  }
-
-  // if there are no depends left then run this test
-  if (this->PendingTests[test].Depends.empty()) {
-    return this->StartTestProcess(test);
-  }
-  // This test was not able to start because it is waiting
-  // on depends to run
-  this->DeallocateResources(test);
-  return false;
+  this->StartTestProcess(test);
 }
 
 void cmCTestMultiProcessHandler::StartNextTests()
 {
-  if (this->TestLoadRetryTimer.get() != nullptr) {
-    // This timer may be waiting to call StartNextTests again.
-    // Since we have been called it is no longer needed.
-    uv_timer_stop(this->TestLoadRetryTimer);
-  }
+  // One or more events may be scheduled to call this method again.
+  // Since this method has been called they are no longer needed.
+  this->StartNextTestsOnIdle_.stop();
+  this->StartNextTestsOnTimer_.stop();
 
-  if (this->PendingTests.empty()) {
-    this->TestLoadRetryTimer.reset();
-    return;
-  }
-
-  if (this->CheckStopTimePassed()) {
-    return;
-  }
-
-  if (this->CheckStopOnFailure() && !this->Failed->empty()) {
+  if (this->PendingTests.empty() || this->CheckStopTimePassed() ||
+      (this->CheckStopOnFailure() && !this->Failed->empty())) {
     return;
   }
 
@@ -553,6 +539,11 @@ void cmCTestMultiProcessHandler::StartNextTests()
       continue;
     }
 
+    // Exclude tests that depend on unfinished tests.
+    if (!this->PendingTests[test].Depends.empty()) {
+      continue;
+    }
+
     size_t processors = this->GetProcessorsUsed(test);
     if (this->TestLoad > 0) {
       // Exclude tests that are too big to fit in the spare load.
@@ -578,9 +569,21 @@ void cmCTestMultiProcessHandler::StartNextTests()
       continue;
     }
 
-    if (this->StartTest(test)) {
-      numToStart -= processors;
+    // Exclude tests that depend on currently-locked project resources.
+    for (std::string const& i : this->Properties[test]->ProjectResources) {
+      if (cm::contains(this->ProjectResourcesLocked, i)) {
+        continue;
+      }
     }
+
+    // Allocate system resources needed by this test.
+    if (!this->AllocateResources(test)) {
+      continue;
+    }
+
+    // The test is ready to run.
+    numToStart -= processors;
+    this->StartTest(test);
   }
 
   if (allTestsFailedTestLoadCheck) {
@@ -617,23 +620,34 @@ void cmCTestMultiProcessHandler::StartNextTests()
     }
     cmCTestLog(this->CTest, HANDLER_VERBOSE_OUTPUT, "*****" << std::endl);
 
-    // Wait between 1 and 5 seconds before trying again.
-    unsigned int milliseconds = (cmSystemTools::RandomSeed() % 5 + 1) * 1000;
-    if (this->FakeLoadForTesting) {
-      milliseconds = 10;
-    }
-    if (this->TestLoadRetryTimer.get() == nullptr) {
-      this->TestLoadRetryTimer.init(*this->Loop, this);
-    }
-    this->TestLoadRetryTimer.start(
-      &cmCTestMultiProcessHandler::OnTestLoadRetryCB, milliseconds, 0);
+    // Try again later when the load might be lower.
+    this->StartNextTestsOnTimer();
   }
 }
 
-void cmCTestMultiProcessHandler::OnTestLoadRetryCB(uv_timer_t* timer)
+void cmCTestMultiProcessHandler::StartNextTestsOnIdle()
 {
-  auto* self = static_cast<cmCTestMultiProcessHandler*>(timer->data);
-  self->StartNextTests();
+  // Start more tests on the next loop iteration.
+  this->StartNextTestsOnIdle_.start([](uv_idle_t* idle) {
+    uv_idle_stop(idle);
+    auto* self = static_cast<cmCTestMultiProcessHandler*>(idle->data);
+    self->StartNextTests();
+  });
+}
+
+void cmCTestMultiProcessHandler::StartNextTestsOnTimer()
+{
+  // Wait between 1 and 5 seconds before trying again.
+  unsigned int const milliseconds = this->FakeLoadForTesting
+    ? 10
+    : (cmSystemTools::RandomSeed() % 5 + 1) * 1000;
+  this->StartNextTestsOnTimer_.start(
+    [](uv_timer_t* timer) {
+      uv_timer_stop(timer);
+      auto* self = static_cast<cmCTestMultiProcessHandler*>(timer->data);
+      self->StartNextTests();
+    },
+    milliseconds, 0);
 }
 
 void cmCTestMultiProcessHandler::FinishTestProcess(
@@ -678,9 +692,8 @@ void cmCTestMultiProcessHandler::FinishTestProcess(
   properties->Affinity.clear();
 
   runner.reset();
-  if (started) {
-    this->StartNextTests();
-  }
+
+  this->StartNextTestsOnIdle();
 }
 
 void cmCTestMultiProcessHandler::UpdateCostData()
@@ -1093,9 +1106,9 @@ static Json::Value DumpCTestProperties(
     properties.append(DumpCTestProperty(
       "REQUIRED_FILES", DumpToJsonArray(testProperties.RequiredFiles)));
   }
-  if (!testProperties.LockedResources.empty()) {
+  if (!testProperties.ProjectResources.empty()) {
     properties.append(DumpCTestProperty(
-      "RESOURCE_LOCK", DumpToJsonArray(testProperties.LockedResources)));
+      "RESOURCE_LOCK", DumpToJsonArray(testProperties.ProjectResources)));
   }
   if (testProperties.RunSerial) {
     properties.append(
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index c01089c..ffbe16f 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -14,8 +14,6 @@
 
 #include <cm/optional>
 
-#include <cm3p/uv.h>
-
 #include "cmCTest.h"
 #include "cmCTestResourceAllocator.h"
 #include "cmCTestResourceSpec.h"
@@ -110,8 +108,8 @@ protected:
   // Start the next test or tests as many as are allowed by
   // ParallelLevel
   void StartNextTests();
-  bool StartTestProcess(int test);
-  bool StartTest(int test);
+  void StartTestProcess(int test);
+  void StartTest(int test);
   // Mark the checkpoint for the given test
   void WriteCheckpoint(int index);
 
@@ -132,7 +130,8 @@ protected:
   void ErasePendingTest(int index);
   void FinishTestProcess(std::unique_ptr<cmCTestRunTest> runner, bool started);
 
-  static void OnTestLoadRetryCB(uv_timer_t* timer);
+  void StartNextTestsOnIdle();
+  void StartNextTestsOnTimer();
 
   void RemoveTest(int index);
   // Check if we need to resume an interrupted test set
@@ -148,6 +147,9 @@ protected:
   bool CheckStopTimePassed();
   void SetStopTimePassed();
 
+  void InitializeLoop();
+  void FinalizeLoop();
+
   void LockResources(int index);
   void UnlockResources(int index);
 
@@ -194,7 +196,7 @@ protected:
   std::vector<std::string>* Passed;
   std::vector<std::string>* Failed;
   std::vector<std::string> LastTestsFailed;
-  std::set<std::string> LockedResources;
+  std::set<std::string> ProjectResourcesLocked;
   std::map<int,
            std::vector<std::map<std::string, std::vector<ResourceAllocation>>>>
     AllocatedResources;
@@ -206,7 +208,8 @@ protected:
   unsigned long TestLoad;
   unsigned long FakeLoadForTesting;
   cm::uv_loop_ptr Loop;
-  cm::uv_timer_ptr TestLoadRetryTimer;
+  cm::uv_idle_ptr StartNextTestsOnIdle_;
+  cm::uv_timer_ptr StartNextTestsOnTimer_;
   cmCTestTestHandler* TestHandler;
   cmCTest* CTest;
   bool HasCycles;
diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index e96dc1f..483b3b4 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -530,7 +530,7 @@ std::string cmCTestRunTest::GetTestPrefix(size_t completed, size_t total) const
   return outputStream.str();
 }
 
-bool cmCTestRunTest::StartTest(std::unique_ptr<cmCTestRunTest> runner,
+void cmCTestRunTest::StartTest(std::unique_ptr<cmCTestRunTest> runner,
                                size_t completed, size_t total)
 {
   auto* testRun = runner.get();
@@ -539,10 +539,7 @@ bool cmCTestRunTest::StartTest(std::unique_ptr<cmCTestRunTest> runner,
 
   if (!testRun->StartTest(completed, total)) {
     testRun->FinalizeTest(false);
-    return false;
   }
-
-  return true;
 }
 
 // Starts the execution of a test.  Returns once it has started
diff --git a/Source/CTest/cmCTestRunTest.h b/Source/CTest/cmCTestRunTest.h
index 154abca..71d0865 100644
--- a/Source/CTest/cmCTestRunTest.h
+++ b/Source/CTest/cmCTestRunTest.h
@@ -56,7 +56,7 @@ public:
   // Read and store output.  Returns true if it must be called again.
   void CheckOutput(std::string const& line);
 
-  static bool StartTest(std::unique_ptr<cmCTestRunTest> runner,
+  static void StartTest(std::unique_ptr<cmCTestRunTest> runner,
                         size_t completed, size_t total);
   static bool StartAgain(std::unique_ptr<cmCTestRunTest> runner,
                          size_t completed);
diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx
index ba15366..1918b2c 100644
--- a/Source/CTest/cmCTestTestHandler.cxx
+++ b/Source/CTest/cmCTestTestHandler.cxx
@@ -2233,7 +2233,7 @@ bool cmCTestTestHandler::SetTestsProperties(
           } else if (key == "RESOURCE_LOCK"_s) {
             cmList lval{ val };
 
-            rt.LockedResources.insert(lval.begin(), lval.end());
+            rt.ProjectResources.insert(lval.begin(), lval.end());
           } else if (key == "FIXTURES_SETUP"_s) {
             cmList lval{ val };
 
diff --git a/Source/CTest/cmCTestTestHandler.h b/Source/CTest/cmCTestTestHandler.h
index 23f0a76..b81fcd5 100644
--- a/Source/CTest/cmCTestTestHandler.h
+++ b/Source/CTest/cmCTestTestHandler.h
@@ -165,7 +165,7 @@ public:
     std::vector<std::string> Environment;
     std::vector<std::string> EnvironmentModification;
     std::vector<std::string> Labels;
-    std::set<std::string> LockedResources;
+    std::set<std::string> ProjectResources; // RESOURCE_LOCK
     std::set<std::string> FixturesSetup;
     std::set<std::string> FixturesCleanup;
     std::set<std::string> FixturesRequired;