Skip to content

Commit 089f938

Browse files
committed
[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.
1 parent e866c44 commit 089f938

File tree

2 files changed

+66
-35
lines changed

2 files changed

+66
-35
lines changed

llvm/lib/Support/Parallel.cpp

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -193,16 +193,7 @@ class ThreadPoolExecutor : public Executor {
193193
JobserverClient *TheJobserver = nullptr;
194194
};
195195

196-
// A global raw pointer to the executor. Lifetime is managed by the
197-
// objects created within createExecutor().
198-
static Executor *TheExec = nullptr;
199-
static std::once_flag Flag;
200-
201-
// This function will be called exactly once to create the executor.
202-
// It contains the necessary platform-specific logic. Since functions
203-
// called by std::call_once cannot return value, we have to set the
204-
// executor as a global variable.
205-
void createExecutor() {
196+
Executor *Executor::getDefaultExecutor() {
206197
#ifdef _WIN32
207198
// The ManagedStatic enables the ThreadPoolExecutor to be stopped via
208199
// llvm_shutdown() which allows a "clean" fast exit, e.g. via _exit(). This
@@ -221,27 +212,19 @@ void createExecutor() {
221212
// are more frequent with the debug static runtime.
222213
//
223214
// This also prevents intermittent deadlocks on exit with the MinGW runtime.
224-
225215
static ManagedStatic<ThreadPoolExecutor, ThreadPoolExecutor::Creator,
226216
ThreadPoolExecutor::Deleter>
227217
ManagedExec;
228-
static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
229-
TheExec = Exec.get();
218+
return &*ManagedExec;
230219
#else
231220
// ManagedStatic is not desired on other platforms. When `Exec` is destroyed
232221
// by llvm_shutdown(), worker threads will clean up and invoke TLS
233222
// destructors. This can lead to race conditions if other threads attempt to
234223
// access TLS objects that have already been destroyed.
235224
static ThreadPoolExecutor Exec(strategy);
236-
TheExec = &Exec;
225+
return &Exec;
237226
#endif
238227
}
239-
240-
Executor *Executor::getDefaultExecutor() {
241-
// Use std::call_once to lazily and safely initialize the executor.
242-
std::call_once(Flag, createExecutor);
243-
return TheExec;
244-
}
245228
} // namespace
246229
} // namespace detail
247230

llvm/unittests/Support/JobserverTest.cpp

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "llvm/Config/llvm-config.h"
1616
#include "llvm/Support/Debug.h"
1717
#include "llvm/Support/Parallel.h"
18+
#include "llvm/Support/Program.h"
1819
#include "llvm/Support/ThreadPool.h"
1920
#include "llvm/Support/raw_ostream.h"
2021
#include "gtest/gtest.h"
@@ -40,8 +41,14 @@
4041

4142
using namespace llvm;
4243

44+
// Provided by the unit test main to locate the current test binary.
45+
extern const char *TestMainArgv0;
46+
4347
namespace {
4448

49+
// Unique anchor whose address helps locate the current test binary.
50+
static int JobserverTestAnchor = 0;
51+
4552
// RAII helper to set an environment variable for the duration of a test.
4653
class ScopedEnvironment {
4754
std::string Name;
@@ -382,51 +389,92 @@ TEST_F(JobserverStrategyTest, ThreadPoolConcurrencyIsLimited) {
382389
EXPECT_EQ(CompletedTasks, NumTasks);
383390
}
384391

385-
TEST_F(JobserverStrategyTest, ParallelForIsLimited) {
392+
// Parent-side driver that spawns a fresh process to run the child test which
393+
// validates that parallelFor respects the jobserver limit when it is the first
394+
// user of the default executor in that process.
395+
TEST_F(JobserverStrategyTest, ParallelForIsLimited_Subprocess) {
396+
// Mark child execution.
397+
setenv("LLVM_JOBSERVER_TEST_CHILD", "1", 1);
398+
399+
// Find the current test binary and build args to run only the child test.
400+
std::string Executable =
401+
sys::fs::getMainExecutable(TestMainArgv0, &JobserverTestAnchor);
402+
SmallVector<StringRef, 4> Args{Executable,
403+
"--gtest_filter=JobserverStrategyTest."
404+
"ParallelForIsLimited_SubprocessChild"};
405+
406+
std::string Error;
407+
bool ExecFailed = false;
408+
int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error,
409+
&ExecFailed);
410+
unsetenv("LLVM_JOBSERVER_TEST_CHILD");
411+
ASSERT_FALSE(ExecFailed) << Error;
412+
ASSERT_EQ(RC, 0) << Error;
413+
}
414+
415+
// Child-side test: create FIFO and make-proxy in this process, set the
416+
// jobserver strategy, and then run parallelFor.
417+
TEST_F(JobserverStrategyTest, ParallelForIsLimited_SubprocessChild) {
418+
if (!getenv("LLVM_JOBSERVER_TEST_CHILD"))
419+
GTEST_SKIP() << "Not running in child mode";
420+
386421
// This test verifies that llvm::parallelFor respects the jobserver limit.
387422
const int NumExplicitJobs = 3;
388423
const int ConcurrencyLimit = NumExplicitJobs + 1; // +1 implicit
389424
const int NumTasks = 20;
390425

391-
LLVM_DEBUG(dbgs() << "Calling startMakeProxy with " << NumExplicitJobs
392-
<< " jobs.\n");
393426
startMakeProxy(NumExplicitJobs);
394-
LLVM_DEBUG(dbgs() << "MakeProxy is running.\n");
395427

396-
// Set the global strategy. parallelFor will use this.
428+
// Set the global strategy before any default executor is created.
397429
parallel::strategy = jobserver_concurrency();
398430

399431
std::atomic<int> ActiveTasks{0};
400432
std::atomic<int> MaxActiveTasks{0};
401433

402434
parallelFor(0, NumTasks, [&](int i) {
403435
int CurrentActive = ++ActiveTasks;
404-
LLVM_DEBUG(dbgs() << "Task " << i << ": Active tasks: " << CurrentActive
405-
<< "\n");
436+
(void)i;
406437
int OldMax = MaxActiveTasks.load();
407438
while (CurrentActive > OldMax)
408439
MaxActiveTasks.compare_exchange_weak(OldMax, CurrentActive);
409-
410440
std::this_thread::sleep_for(std::chrono::milliseconds(20));
411441
--ActiveTasks;
412442
});
413443

414-
LLVM_DEBUG(dbgs() << "ParallelFor finished. Max active tasks was "
415-
<< MaxActiveTasks << ".\n");
416444
EXPECT_LE(MaxActiveTasks, ConcurrencyLimit);
417445
}
418446

419-
TEST_F(JobserverStrategyTest, ParallelSortIsLimited) {
420-
// This test serves as an integration test to ensure parallelSort completes
421-
// correctly when running under the jobserver strategy. It doesn't directly
422-
// measure concurrency but verifies correctness.
447+
// Parent-side driver for parallelSort child test.
448+
TEST_F(JobserverStrategyTest, ParallelSortIsLimited_Subprocess) {
449+
setenv("LLVM_JOBSERVER_TEST_CHILD", "1", 1);
450+
451+
std::string Executable =
452+
sys::fs::getMainExecutable(TestMainArgv0, &JobserverTestAnchor);
453+
SmallVector<StringRef, 4> Args{Executable,
454+
"--gtest_filter=JobserverStrategyTest."
455+
"ParallelSortIsLimited_SubprocessChild"};
456+
457+
std::string Error;
458+
bool ExecFailed = false;
459+
int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error,
460+
&ExecFailed);
461+
unsetenv("LLVM_JOBSERVER_TEST_CHILD");
462+
ASSERT_FALSE(ExecFailed) << Error;
463+
ASSERT_EQ(RC, 0) << Error;
464+
}
465+
466+
// Child-side test: ensure parallelSort runs and completes correctly under the
467+
// jobserver strategy when it owns default executor initialization.
468+
TEST_F(JobserverStrategyTest, ParallelSortIsLimited_SubprocessChild) {
469+
if (!getenv("LLVM_JOBSERVER_TEST_CHILD"))
470+
GTEST_SKIP() << "Not running in child mode";
471+
423472
const int NumExplicitJobs = 3;
424473
startMakeProxy(NumExplicitJobs);
425474

426475
parallel::strategy = jobserver_concurrency();
427476

428477
std::vector<int> V(1024);
429-
// Fill with random data
430478
std::mt19937 randEngine;
431479
std::uniform_int_distribution<int> dist;
432480
for (int &i : V)

0 commit comments

Comments
 (0)