Merge topic 'ctest-cleanup'

0432b921ae cmCTestMultiProcessHandler: Inline removal of pending test as it starts
b17c732e88 cmCTestMultiProcessHandler: Clarify role of StartTestProcess
0950acb337 cmCTestMultiProcessHandler: Manage concurrency slots with other resources
697900da29 cmCTestMultiProcessHandler: Manage affinity assignments with other resources

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

Source/CTest/cmCTestMultiProcessHandler.cxx

@@ -164,24 +164,8 @@ void cmCTestMultiProcessHandler::RunTests()
 
 void cmCTestMultiProcessHandler::StartTestProcess(int test)
 {
-  if (this->HaveAffinity && this->Properties[test]->WantAffinity) {
-    size_t needProcessors = this->GetProcessorsUsed(test);
-    assert(needProcessors <= this->ProcessorsAvailable.size());
-    std::vector<size_t> affinity;
-    affinity.reserve(needProcessors);
-    for (size_t i = 0; i < needProcessors; ++i) {
-      auto p = this->ProcessorsAvailable.begin();
-      affinity.push_back(*p);
-      this->ProcessorsAvailable.erase(p);
-    }
-    this->Properties[test]->Affinity = std::move(affinity);
-  }
-
   cmCTestOptionalLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
                      "test " << test << "\n", this->Quiet);
-  // now remove the test itself
-  this->ErasePendingTest(test);
-  this->RunningCount += this->GetProcessorsUsed(test);
 
   auto testRun = cm::make_unique<cmCTestRunTest>(*this, test);
 
@@ -202,10 +186,6 @@ void cmCTestMultiProcessHandler::StartTestProcess(int test)
     }
   }
 
-  // Always lock the resources we'll be using, even if we fail to set the
-  // working directory because FinishTestProcess() will try to unlock them
-  this->LockResources(test);
-
   if (!this->ResourceAvailabilityErrors[test].empty()) {
     std::ostringstream e;
     e << "Insufficient resources for test " << this->Properties[test]->Name
@@ -413,30 +393,49 @@ void cmCTestMultiProcessHandler::SetStopTimePassed()
 
 void cmCTestMultiProcessHandler::LockResources(int index)
 {
+  this->RunningCount += this->GetProcessorsUsed(index);
+
   auto* properties = this->Properties[index];
+
   this->ProjectResourcesLocked.insert(properties->ProjectResources.begin(),
                                       properties->ProjectResources.end());
+
   if (properties->RunSerial) {
     this->SerialTestRunning = true;
   }
+
+  if (this->HaveAffinity && properties->WantAffinity) {
+    size_t needProcessors = this->GetProcessorsUsed(index);
+    assert(needProcessors <= this->ProcessorsAvailable.size());
+    std::vector<size_t> affinity;
+    affinity.reserve(needProcessors);
+    for (size_t i = 0; i < needProcessors; ++i) {
+      auto p = this->ProcessorsAvailable.begin();
+      affinity.push_back(*p);
+      this->ProcessorsAvailable.erase(p);
+    }
+    properties->Affinity = std::move(affinity);
+  }
 }
 
 void cmCTestMultiProcessHandler::UnlockResources(int index)
 {
   auto* properties = this->Properties[index];
+
+  for (auto p : properties->Affinity) {
+    this->ProcessorsAvailable.insert(p);
+  }
+  properties->Affinity.clear();
+
   for (std::string const& i : properties->ProjectResources) {
     this->ProjectResourcesLocked.erase(i);
   }
+
   if (properties->RunSerial) {
     this->SerialTestRunning = false;
   }
-}
 
-void cmCTestMultiProcessHandler::ErasePendingTest(int test)
-{
-  this->PendingTests.erase(test);
-  this->OrderedTests.erase(
-    std::find(this->OrderedTests.begin(), this->OrderedTests.end(), test));
+  this->RunningCount -= this->GetProcessorsUsed(index);
 }
 
 inline size_t cmCTestMultiProcessHandler::GetProcessorsUsed(int test)
@@ -531,7 +530,8 @@ void cmCTestMultiProcessHandler::StartNextTests()
          ti != this->OrderedTests.end()) {
     // Increment the test iterator now because the current list
     // entry may be deleted below.
-    int test = *ti++;
+    auto cti = ti++;
+    int test = *cti;
 
     // We can only start a RUN_SERIAL test if no other tests are also
     // running.
@@ -581,8 +581,13 @@ void cmCTestMultiProcessHandler::StartNextTests()
       continue;
     }
 
+    // Lock resources needed by this test.
+    this->LockResources(test);
+
     // The test is ready to run.
     numToStart -= processors;
+    this->OrderedTests.erase(cti);
+    this->PendingTests.erase(test);
     this->StartTest(test);
   }
 
@@ -684,12 +689,6 @@ void cmCTestMultiProcessHandler::FinishTestProcess(
   this->WriteCheckpoint(test);
   this->DeallocateResources(test);
   this->UnlockResources(test);
-  this->RunningCount -= this->GetProcessorsUsed(test);
-
-  for (auto p : properties->Affinity) {
-    this->ProcessorsAvailable.insert(p);
-  }
-  properties->Affinity.clear();
 
   runner.reset();
 
@@ -1407,7 +1406,9 @@ void cmCTestMultiProcessHandler::CheckResume()
 
 void cmCTestMultiProcessHandler::RemoveTest(int index)
 {
-  this->ErasePendingTest(index);
+  this->OrderedTests.erase(
+    std::find(this->OrderedTests.begin(), this->OrderedTests.end(), index));
+  this->PendingTests.erase(index);
   this->Properties.erase(index);
   this->Completed++;
 }

Source/CTest/cmCTestMultiProcessHandler.h

@@ -127,7 +127,6 @@ protected:
 
   // Removes the checkpoint file
   void MarkFinished();
-  void ErasePendingTest(int index);
   void FinishTestProcess(std::unique_ptr<cmCTestRunTest> runner, bool started);
 
   void StartNextTestsOnIdle();