From cb40a26cc34388fd22a4efef89a902c2b2bbdfc8 Mon Sep 17 00:00:00 2001 From: "Yaxun (Sam) Liu" Date: Mon, 27 Oct 2025 10:41:50 -0400 Subject: [PATCH] [Support][Jobserver][Tests] Simplify default executor init and make jobserver tests deterministic - Replace call-once wrapper in Parallel.cpp with a function-local static default executor. Keep original Windows vs. non-Windows ManagedStatic commentary and behavior: use ManagedStatic on Windows (so llvm_shutdown() stops the pool), use a plain static elsewhere to avoid TLS teardown races during shutdown. - Rework Jobserver tests for parallelFor/parallelSort to run in a fresh subprocess. The parent test spawns the current test binary with a gtest filter selecting a child test, ensuring the child process initializes the default executor after setting parallel::strategy = jobserver_concurrency() and after setting up a FIFO-backed jobserver proxy. This makes the tests reliable and independent from prior executor initialization in the combined SupportTests binary. This addresses review feedback: - Remove unnecessary call-once wrapper around a function-local static. - Fix flakiness: avoid changing strategy after the global executor exists by running sensitive tests in a fresh process that controls initialization. --- llvm/lib/Support/Parallel.cpp | 23 +------ llvm/unittests/Support/JobserverTest.cpp | 81 +++++++++++++++++++----- 2 files changed, 68 insertions(+), 36 deletions(-) diff --git a/llvm/lib/Support/Parallel.cpp b/llvm/lib/Support/Parallel.cpp index 8e0c724accb36..f382a174a492a 100644 --- a/llvm/lib/Support/Parallel.cpp +++ b/llvm/lib/Support/Parallel.cpp @@ -193,16 +193,7 @@ class ThreadPoolExecutor : public Executor { JobserverClient *TheJobserver = nullptr; }; -// A global raw pointer to the executor. Lifetime is managed by the -// objects created within createExecutor(). -static Executor *TheExec = nullptr; -static std::once_flag Flag; - -// This function will be called exactly once to create the executor. -// It contains the necessary platform-specific logic. Since functions -// called by std::call_once cannot return value, we have to set the -// executor as a global variable. -void createExecutor() { +Executor *Executor::getDefaultExecutor() { #ifdef _WIN32 // The ManagedStatic enables the ThreadPoolExecutor to be stopped via // llvm_shutdown() which allows a "clean" fast exit, e.g. via _exit(). This @@ -221,27 +212,19 @@ void createExecutor() { // are more frequent with the debug static runtime. // // This also prevents intermittent deadlocks on exit with the MinGW runtime. - static ManagedStatic ManagedExec; - static std::unique_ptr Exec(&(*ManagedExec)); - TheExec = Exec.get(); + return &*ManagedExec; #else // ManagedStatic is not desired on other platforms. When `Exec` is destroyed // by llvm_shutdown(), worker threads will clean up and invoke TLS // destructors. This can lead to race conditions if other threads attempt to // access TLS objects that have already been destroyed. static ThreadPoolExecutor Exec(strategy); - TheExec = &Exec; + return &Exec; #endif } - -Executor *Executor::getDefaultExecutor() { - // Use std::call_once to lazily and safely initialize the executor. - std::call_once(Flag, createExecutor); - return TheExec; -} } // namespace } // namespace detail diff --git a/llvm/unittests/Support/JobserverTest.cpp b/llvm/unittests/Support/JobserverTest.cpp index d27445897db0a..1917145704608 100644 --- a/llvm/unittests/Support/JobserverTest.cpp +++ b/llvm/unittests/Support/JobserverTest.cpp @@ -15,6 +15,7 @@ #include "llvm/Config/llvm-config.h" #include "llvm/Support/Debug.h" #include "llvm/Support/Parallel.h" +#include "llvm/Support/Program.h" #include "llvm/Support/ThreadPool.h" #include "llvm/Support/raw_ostream.h" #include "gtest/gtest.h" @@ -40,8 +41,14 @@ using namespace llvm; +// Provided by the unit test main to locate the current test binary. +extern const char *TestMainArgv0; + namespace { +// Unique anchor whose address helps locate the current test binary. +static int JobserverTestAnchor = 0; + // RAII helper to set an environment variable for the duration of a test. class ScopedEnvironment { std::string Name; @@ -382,51 +389,93 @@ TEST_F(JobserverStrategyTest, ThreadPoolConcurrencyIsLimited) { EXPECT_EQ(CompletedTasks, NumTasks); } -TEST_F(JobserverStrategyTest, ParallelForIsLimited) { +// Parent-side driver that spawns a fresh process to run the child test which +// validates that parallelFor respects the jobserver limit when it is the first +// user of the default executor in that process. +TEST_F(JobserverStrategyTest, ParallelForIsLimited_Subprocess) { + // Mark child execution. + setenv("LLVM_JOBSERVER_TEST_CHILD", "1", 1); + + // Find the current test binary and build args to run only the child test. + std::string Executable = + sys::fs::getMainExecutable(TestMainArgv0, &JobserverTestAnchor); + ASSERT_FALSE(Executable.empty()) << "Failed to get main executable path"; + SmallVector Args{Executable, + "--gtest_filter=JobserverStrategyTest." + "ParallelForIsLimited_SubprocessChild"}; + + std::string Error; + bool ExecFailed = false; + int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error, + &ExecFailed); + unsetenv("LLVM_JOBSERVER_TEST_CHILD"); + ASSERT_FALSE(ExecFailed) << Error; + ASSERT_EQ(RC, 0) << "Executable failed with exit code " << RC; +} + +// Child-side test: create FIFO and make-proxy in this process, set the +// jobserver strategy, and then run parallelFor. +TEST_F(JobserverStrategyTest, ParallelForIsLimited_SubprocessChild) { + if (!getenv("LLVM_JOBSERVER_TEST_CHILD")) + GTEST_SKIP() << "Not running in child mode"; + // This test verifies that llvm::parallelFor respects the jobserver limit. const int NumExplicitJobs = 3; const int ConcurrencyLimit = NumExplicitJobs + 1; // +1 implicit const int NumTasks = 20; - LLVM_DEBUG(dbgs() << "Calling startMakeProxy with " << NumExplicitJobs - << " jobs.\n"); startMakeProxy(NumExplicitJobs); - LLVM_DEBUG(dbgs() << "MakeProxy is running.\n"); - // Set the global strategy. parallelFor will use this. + // Set the global strategy before any default executor is created. parallel::strategy = jobserver_concurrency(); std::atomic ActiveTasks{0}; std::atomic MaxActiveTasks{0}; - parallelFor(0, NumTasks, [&](int i) { + parallelFor(0, NumTasks, [&]([[maybe_unused]] int i) { int CurrentActive = ++ActiveTasks; - LLVM_DEBUG(dbgs() << "Task " << i << ": Active tasks: " << CurrentActive - << "\n"); int OldMax = MaxActiveTasks.load(); while (CurrentActive > OldMax) MaxActiveTasks.compare_exchange_weak(OldMax, CurrentActive); - std::this_thread::sleep_for(std::chrono::milliseconds(20)); --ActiveTasks; }); - LLVM_DEBUG(dbgs() << "ParallelFor finished. Max active tasks was " - << MaxActiveTasks << ".\n"); EXPECT_LE(MaxActiveTasks, ConcurrencyLimit); } -TEST_F(JobserverStrategyTest, ParallelSortIsLimited) { - // This test serves as an integration test to ensure parallelSort completes - // correctly when running under the jobserver strategy. It doesn't directly - // measure concurrency but verifies correctness. +// Parent-side driver for parallelSort child test. +TEST_F(JobserverStrategyTest, ParallelSortIsLimited_Subprocess) { + setenv("LLVM_JOBSERVER_TEST_CHILD", "1", 1); + + std::string Executable = + sys::fs::getMainExecutable(TestMainArgv0, &JobserverTestAnchor); + ASSERT_FALSE(Executable.empty()) << "Failed to get main executable path"; + SmallVector Args{Executable, + "--gtest_filter=JobserverStrategyTest." + "ParallelSortIsLimited_SubprocessChild"}; + + std::string Error; + bool ExecFailed = false; + int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error, + &ExecFailed); + unsetenv("LLVM_JOBSERVER_TEST_CHILD"); + ASSERT_FALSE(ExecFailed) << Error; + ASSERT_EQ(RC, 0) << "Executable failed with exit code " << RC; +} + +// Child-side test: ensure parallelSort runs and completes correctly under the +// jobserver strategy when it owns default executor initialization. +TEST_F(JobserverStrategyTest, ParallelSortIsLimited_SubprocessChild) { + if (!getenv("LLVM_JOBSERVER_TEST_CHILD")) + GTEST_SKIP() << "Not running in child mode"; + const int NumExplicitJobs = 3; startMakeProxy(NumExplicitJobs); parallel::strategy = jobserver_concurrency(); std::vector V(1024); - // Fill with random data std::mt19937 randEngine; std::uniform_int_distribution dist; for (int &i : V)