Skip to content

Commit c7d2ed4

Browse files
authored
Reland [Support][Jobserver][Tests] Simplify default executor init (#168165)
and make (#165264) Truely recover Executor::getDefaultExecutor. The previous change missed std::unique_ptr, which is needed in a normal program exit, since only with that ThreadPoolExecutor destructor will be called in a normal program exit, where it ensures the executor has been stopped and waits for worker threads to finish. The wait is important as it prevents intermittent crashes on Windows when the process is doing a full exit.
1 parent 2675dcd commit c7d2ed4

File tree

2 files changed

+68
-34
lines changed

2 files changed

+68
-34
lines changed

llvm/lib/Support/Parallel.cpp

Lines changed: 3 additions & 18 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
@@ -226,22 +217,16 @@ void createExecutor() {
226217
ThreadPoolExecutor::Deleter>
227218
ManagedExec;
228219
static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
229-
TheExec = Exec.get();
220+
return Exec.get();
230221
#else
231222
// ManagedStatic is not desired on other platforms. When `Exec` is destroyed
232223
// by llvm_shutdown(), worker threads will clean up and invoke TLS
233224
// destructors. This can lead to race conditions if other threads attempt to
234225
// access TLS objects that have already been destroyed.
235226
static ThreadPoolExecutor Exec(strategy);
236-
TheExec = &Exec;
227+
return &Exec;
237228
#endif
238229
}
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-
}
245230
} // namespace
246231
} // namespace detail
247232

llvm/unittests/Support/JobserverTest.cpp

Lines changed: 65 additions & 16 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,93 @@ 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+
ASSERT_FALSE(Executable.empty()) << "Failed to get main executable path";
403+
SmallVector<StringRef, 4> Args{Executable,
404+
"--gtest_filter=JobserverStrategyTest."
405+
"ParallelForIsLimited_SubprocessChild"};
406+
407+
std::string Error;
408+
bool ExecFailed = false;
409+
int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error,
410+
&ExecFailed);
411+
unsetenv("LLVM_JOBSERVER_TEST_CHILD");
412+
ASSERT_FALSE(ExecFailed) << Error;
413+
ASSERT_EQ(RC, 0) << "Executable failed with exit code " << RC;
414+
}
415+
416+
// Child-side test: create FIFO and make-proxy in this process, set the
417+
// jobserver strategy, and then run parallelFor.
418+
TEST_F(JobserverStrategyTest, ParallelForIsLimited_SubprocessChild) {
419+
if (!getenv("LLVM_JOBSERVER_TEST_CHILD"))
420+
GTEST_SKIP() << "Not running in child mode";
421+
386422
// This test verifies that llvm::parallelFor respects the jobserver limit.
387423
const int NumExplicitJobs = 3;
388424
const int ConcurrencyLimit = NumExplicitJobs + 1; // +1 implicit
389425
const int NumTasks = 20;
390426

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

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

399432
std::atomic<int> ActiveTasks{0};
400433
std::atomic<int> MaxActiveTasks{0};
401434

402-
parallelFor(0, NumTasks, [&](int i) {
435+
parallelFor(0, NumTasks, [&]([[maybe_unused]] int i) {
403436
int CurrentActive = ++ActiveTasks;
404-
LLVM_DEBUG(dbgs() << "Task " << i << ": Active tasks: " << CurrentActive
405-
<< "\n");
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+
ASSERT_FALSE(Executable.empty()) << "Failed to get main executable path";
454+
SmallVector<StringRef, 4> Args{Executable,
455+
"--gtest_filter=JobserverStrategyTest."
456+
"ParallelSortIsLimited_SubprocessChild"};
457+
458+
std::string Error;
459+
bool ExecFailed = false;
460+
int RC = sys::ExecuteAndWait(Executable, Args, std::nullopt, {}, 0, 0, &Error,
461+
&ExecFailed);
462+
unsetenv("LLVM_JOBSERVER_TEST_CHILD");
463+
ASSERT_FALSE(ExecFailed) << Error;
464+
ASSERT_EQ(RC, 0) << "Executable failed with exit code " << RC;
465+
}
466+
467+
// Child-side test: ensure parallelSort runs and completes correctly under the
468+
// jobserver strategy when it owns default executor initialization.
469+
TEST_F(JobserverStrategyTest, ParallelSortIsLimited_SubprocessChild) {
470+
if (!getenv("LLVM_JOBSERVER_TEST_CHILD"))
471+
GTEST_SKIP() << "Not running in child mode";
472+
423473
const int NumExplicitJobs = 3;
424474
startMakeProxy(NumExplicitJobs);
425475

426476
parallel::strategy = jobserver_concurrency();
427477

428478
std::vector<int> V(1024);
429-
// Fill with random data
430479
std::mt19937 randEngine;
431480
std::uniform_int_distribution<int> dist;
432481
for (int &i : V)

0 commit comments

Comments
 (0)