From 17d13fd7881fd3ce9f9b9d44ce435d6caf4b8f28 Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Tue, 22 Mar 2016 13:48:07 +0200 Subject: [PATCH 01/11] Add GNU make jobserver client support - add new TokenPool interface - GNU make implementation for TokenPool parses and verifies the magic information from the MAKEFLAGS environment variable - RealCommandRunner tries to acquire TokenPool * if no token pool is available then there is no change in behaviour - When a token pool is available then RealCommandRunner behaviour changes as follows * CanRunMore() only returns true if TokenPool::Acquire() returns true * StartCommand() calls TokenPool::Reserve() * WaitForCommand() calls TokenPool::Release() Documentation for GNU make jobserver http://make.mad-scientist.net/papers/jobserver-implementation/ Fixes https://github.com/ninja-build/ninja/issues/1139 --- configure.py | 2 + src/build.cc | 63 ++++++++---- src/build.h | 3 + src/tokenpool-gnu-make.cc | 211 ++++++++++++++++++++++++++++++++++++++ src/tokenpool-none.cc | 27 +++++ src/tokenpool.h | 26 +++++ 6 files changed, 310 insertions(+), 22 deletions(-) create mode 100644 src/tokenpool-gnu-make.cc create mode 100644 src/tokenpool-none.cc create mode 100644 src/tokenpool.h diff --git a/configure.py b/configure.py index 43904349a8..db3492c93c 100755 --- a/configure.py +++ b/configure.py @@ -522,6 +522,7 @@ def has_re2c(): objs += cxx(name, variables=cxxvariables) if platform.is_windows(): for name in ['subprocess-win32', + 'tokenpool-none', 'includes_normalize-win32', 'msvc_helper-win32', 'msvc_helper_main-win32']: @@ -531,6 +532,7 @@ def has_re2c(): objs += cc('getopt') else: objs += cxx('subprocess-posix') + objs += cxx('tokenpool-gnu-make') if platform.is_aix(): objs += cc('getopt') if platform.is_msvc(): diff --git a/src/build.cc b/src/build.cc index 6f11ed7a3c..fa096eac33 100644 --- a/src/build.cc +++ b/src/build.cc @@ -35,6 +35,7 @@ #include "state.h" #include "status.h" #include "subprocess.h" +#include "tokenpool.h" #include "util.h" using namespace std; @@ -149,7 +150,7 @@ void Plan::EdgeWanted(const Edge* edge) { } Edge* Plan::FindWork() { - if (ready_.empty()) + if (!more_ready()) return NULL; EdgeSet::iterator e = ready_.begin(); Edge* edge = *e; @@ -448,8 +449,8 @@ void Plan::Dump() const { } struct RealCommandRunner : public CommandRunner { - explicit RealCommandRunner(const BuildConfig& config) : config_(config) {} - virtual ~RealCommandRunner() {} + explicit RealCommandRunner(const BuildConfig& config); + virtual ~RealCommandRunner(); virtual bool CanRunMore() const; virtual bool StartCommand(Edge* edge); virtual bool WaitForCommand(Result* result); @@ -458,9 +459,18 @@ struct RealCommandRunner : public CommandRunner { const BuildConfig& config_; SubprocessSet subprocs_; + TokenPool *tokens_; map subproc_to_edge_; }; +RealCommandRunner::RealCommandRunner(const BuildConfig& config) : config_(config) { + tokens_ = TokenPool::Get(); +} + +RealCommandRunner::~RealCommandRunner() { + delete tokens_; +} + vector RealCommandRunner::GetActiveEdges() { vector edges; for (map::iterator e = subproc_to_edge_.begin(); @@ -471,14 +481,18 @@ vector RealCommandRunner::GetActiveEdges() { void RealCommandRunner::Abort() { subprocs_.Clear(); + if (tokens_) + tokens_->Clear(); } bool RealCommandRunner::CanRunMore() const { size_t subproc_number = subprocs_.running_.size() + subprocs_.finished_.size(); return (int)subproc_number < config_.parallelism - && ((subprocs_.running_.empty() || config_.max_load_average <= 0.0f) - || GetLoadAverage() < config_.max_load_average); + && (subprocs_.running_.empty() || + ((config_.max_load_average <= 0.0f || + GetLoadAverage() < config_.max_load_average) + && (!tokens_ || tokens_->Acquire()))); } bool RealCommandRunner::StartCommand(Edge* edge) { @@ -486,6 +500,8 @@ bool RealCommandRunner::StartCommand(Edge* edge) { Subprocess* subproc = subprocs_.Add(command, edge->use_console()); if (!subproc) return false; + if (tokens_) + tokens_->Reserve(); subproc_to_edge_.insert(make_pair(subproc, edge)); return true; @@ -499,6 +515,9 @@ bool RealCommandRunner::WaitForCommand(Result* result) { return false; } + if (tokens_) + tokens_->Release(); + result->status = subproc->Finish(); result->output = subproc->GetOutput(); @@ -621,31 +640,31 @@ bool Builder::Build(string* err) { // Second, we attempt to wait for / reap the next finished command. while (plan_.more_to_do()) { // See if we can start any more commands. - if (failures_allowed && command_runner_->CanRunMore()) { - if (Edge* edge = plan_.FindWork()) { - if (edge->GetBindingBool("generator")) { + if (failures_allowed && plan_.more_ready() && + command_runner_->CanRunMore()) { + Edge* edge = plan_.FindWork(); + if (edge->GetBindingBool("generator")) { scan_.build_log()->Close(); } - if (!StartEdge(edge, err)) { + if (!StartEdge(edge, err)) { + Cleanup(); + status_->BuildFinished(); + return false; + } + + if (edge->is_phony()) { + if (!plan_.EdgeFinished(edge, Plan::kEdgeSucceeded, err)) { Cleanup(); status_->BuildFinished(); return false; } - - if (edge->is_phony()) { - if (!plan_.EdgeFinished(edge, Plan::kEdgeSucceeded, err)) { - Cleanup(); - status_->BuildFinished(); - return false; - } - } else { - ++pending_commands; - } - - // We made some progress; go back to the main loop. - continue; + } else { + ++pending_commands; } + + // We made some progress; go back to the main loop. + continue; } // See if we can reap any finished commands. diff --git a/src/build.h b/src/build.h index d697dfb89e..7dcd111e61 100644 --- a/src/build.h +++ b/src/build.h @@ -52,6 +52,9 @@ struct Plan { /// Returns true if there's more work to be done. bool more_to_do() const { return wanted_edges_ > 0 && command_edges_ > 0; } + /// Returns true if there's more edges ready to start + bool more_ready() const { return !ready_.empty(); } + /// Dumps the current state of the plan. void Dump() const; diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc new file mode 100644 index 0000000000..a8f9b7139d --- /dev/null +++ b/src/tokenpool-gnu-make.cc @@ -0,0 +1,211 @@ +// Copyright 2016 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool.h" + +#include +#include +#include +#include +#include +#include +#include +#include + +// TokenPool implementation for GNU make jobserver +// (http://make.mad-scientist.net/papers/jobserver-implementation/) +struct GNUmakeTokenPool : public TokenPool { + GNUmakeTokenPool(); + virtual ~GNUmakeTokenPool(); + + virtual bool Acquire(); + virtual void Reserve(); + virtual void Release(); + virtual void Clear(); + + bool Setup(); + + private: + int available_; + int used_; + +#ifdef _WIN32 + // @TODO +#else + int rfd_; + int wfd_; + + struct sigaction old_act_; + bool restore_; + + static int dup_rfd_; + static void CloseDupRfd(int signum); + + bool CheckFd(int fd); + bool SetAlarmHandler(); +#endif + + void Return(); +}; + +// every instance owns an implicit token -> available_ == 1 +GNUmakeTokenPool::GNUmakeTokenPool() : available_(1), used_(0), + rfd_(-1), wfd_(-1), restore_(false) { +} + +GNUmakeTokenPool::~GNUmakeTokenPool() { + Clear(); + if (restore_) + sigaction(SIGALRM, &old_act_, NULL); +} + +bool GNUmakeTokenPool::CheckFd(int fd) { + if (fd < 0) + return false; + int ret = fcntl(fd, F_GETFD); + if (ret < 0) + return false; + return true; +} + +int GNUmakeTokenPool::dup_rfd_ = -1; + +void GNUmakeTokenPool::CloseDupRfd(int signum) { + close(dup_rfd_); + dup_rfd_ = -1; +} + +bool GNUmakeTokenPool::SetAlarmHandler() { + struct sigaction act; + memset(&act, 0, sizeof(act)); + act.sa_handler = CloseDupRfd; + if (sigaction(SIGALRM, &act, &old_act_) < 0) { + perror("sigaction:"); + return(false); + } else { + restore_ = true; + return(true); + } +} + +bool GNUmakeTokenPool::Setup() { + const char *value = getenv("MAKEFLAGS"); + if (value) { + // GNU make <= 4.1 + const char *jobserver = strstr(value, "--jobserver-fds="); + // GNU make => 4.2 + if (!jobserver) + jobserver = strstr(value, "--jobserver-auth="); + if (jobserver) { + int rfd = -1; + int wfd = -1; + if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && + CheckFd(rfd) && + CheckFd(wfd) && + SetAlarmHandler()) { + printf("ninja: using GNU make jobserver.\n"); + rfd_ = rfd; + wfd_ = wfd; + return true; + } + } + } + + return false; +} + +bool GNUmakeTokenPool::Acquire() { + if (available_ > 0) + return true; + +#ifdef USE_PPOLL + pollfd pollfds[] = {{rfd_, POLLIN, 0}}; + int ret = poll(pollfds, 1, 0); +#else + fd_set set; + struct timeval timeout = { 0, 0 }; + FD_ZERO(&set); + FD_SET(rfd_, &set); + int ret = select(rfd_ + 1, &set, NULL, NULL, &timeout); +#endif + if (ret > 0) { + dup_rfd_ = dup(rfd_); + + if (dup_rfd_ != -1) { + struct sigaction act, old_act; + int ret = 0; + + memset(&act, 0, sizeof(act)); + act.sa_handler = CloseDupRfd; + if (sigaction(SIGCHLD, &act, &old_act) == 0) { + char buf; + + // block until token read, child exits or timeout + alarm(1); + ret = read(dup_rfd_, &buf, 1); + alarm(0); + + sigaction(SIGCHLD, &old_act, NULL); + } + + CloseDupRfd(0); + + if (ret > 0) { + available_++; + return true; + } + } + } + return false; +} + +void GNUmakeTokenPool::Reserve() { + available_--; + used_++; +} + +void GNUmakeTokenPool::Return() { + const char buf = '+'; + while (1) { + int ret = write(wfd_, &buf, 1); + if (ret > 0) + available_--; + if ((ret != -1) || (errno != EINTR)) + return; + // write got interrupted - retry + } +} + +void GNUmakeTokenPool::Release() { + available_++; + used_--; + if (available_ > 1) + Return(); +} + +void GNUmakeTokenPool::Clear() { + while (used_ > 0) + Release(); + while (available_ > 1) + Return(); +} + +struct TokenPool *TokenPool::Get(void) { + GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; + if (tokenpool->Setup()) + return tokenpool; + else + delete tokenpool; + return NULL; +} diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc new file mode 100644 index 0000000000..602b3316f5 --- /dev/null +++ b/src/tokenpool-none.cc @@ -0,0 +1,27 @@ +// Copyright 2016 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool.h" + +#include +#include +#include +#include +#include +#include + +// No-op TokenPool implementation +struct TokenPool *TokenPool::Get(void) { + return NULL; +} diff --git a/src/tokenpool.h b/src/tokenpool.h new file mode 100644 index 0000000000..f560b1083b --- /dev/null +++ b/src/tokenpool.h @@ -0,0 +1,26 @@ +// Copyright 2016 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// interface to token pool +struct TokenPool { + virtual ~TokenPool() {} + + virtual bool Acquire() = 0; + virtual void Reserve() = 0; + virtual void Release() = 0; + virtual void Clear() = 0; + + // returns NULL if token pool is not available + static struct TokenPool *Get(void); +}; From ccaccc610cd456f6068758f82e72006364c7380b Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Fri, 27 May 2016 16:47:10 +0300 Subject: [PATCH 02/11] Add TokenPool monitoring to SubprocessSet::DoWork() Improve on the original jobserver client implementation. This makes ninja a more aggressive GNU make jobserver client. - add monitor interface to TokenPool - TokenPool is passed down when main loop indicates that more work is ready and would be allowed to start if a token becomes available - posix: update DoWork() to monitor TokenPool read file descriptor - WaitForCommand() exits when DoWork() sets token flag - Main loop starts over when WaitForCommand() sets token exit status --- src/build.cc | 53 +++++++++++++++++++++++++++++---------- src/build.h | 3 ++- src/build_test.cc | 9 +++++-- src/exit_status.h | 3 ++- src/subprocess-posix.cc | 33 ++++++++++++++++++++++-- src/subprocess-win32.cc | 2 +- src/subprocess.h | 8 +++++- src/subprocess_test.cc | 47 +++++++++++++++++++++++----------- src/tokenpool-gnu-make.cc | 5 ++++ src/tokenpool.h | 6 +++++ 10 files changed, 134 insertions(+), 35 deletions(-) diff --git a/src/build.cc b/src/build.cc index fa096eac33..a25c349050 100644 --- a/src/build.cc +++ b/src/build.cc @@ -48,8 +48,9 @@ struct DryRunCommandRunner : public CommandRunner { // Overridden from CommandRunner: virtual bool CanRunMore() const; + virtual bool AcquireToken(); virtual bool StartCommand(Edge* edge); - virtual bool WaitForCommand(Result* result); + virtual bool WaitForCommand(Result* result, bool more_ready); private: queue finished_; @@ -59,12 +60,16 @@ bool DryRunCommandRunner::CanRunMore() const { return true; } +bool DryRunCommandRunner::AcquireToken() { + return true; +} + bool DryRunCommandRunner::StartCommand(Edge* edge) { finished_.push(edge); return true; } -bool DryRunCommandRunner::WaitForCommand(Result* result) { +bool DryRunCommandRunner::WaitForCommand(Result* result, bool more_ready) { if (finished_.empty()) return false; @@ -452,8 +457,9 @@ struct RealCommandRunner : public CommandRunner { explicit RealCommandRunner(const BuildConfig& config); virtual ~RealCommandRunner(); virtual bool CanRunMore() const; + virtual bool AcquireToken(); virtual bool StartCommand(Edge* edge); - virtual bool WaitForCommand(Result* result); + virtual bool WaitForCommand(Result* result, bool more_ready); virtual vector GetActiveEdges(); virtual void Abort(); @@ -490,9 +496,12 @@ bool RealCommandRunner::CanRunMore() const { subprocs_.running_.size() + subprocs_.finished_.size(); return (int)subproc_number < config_.parallelism && (subprocs_.running_.empty() || - ((config_.max_load_average <= 0.0f || - GetLoadAverage() < config_.max_load_average) - && (!tokens_ || tokens_->Acquire()))); + (config_.max_load_average <= 0.0f || + GetLoadAverage() < config_.max_load_average)); +} + +bool RealCommandRunner::AcquireToken() { + return (!tokens_ || tokens_->Acquire()); } bool RealCommandRunner::StartCommand(Edge* edge) { @@ -507,14 +516,23 @@ bool RealCommandRunner::StartCommand(Edge* edge) { return true; } -bool RealCommandRunner::WaitForCommand(Result* result) { +bool RealCommandRunner::WaitForCommand(Result* result, bool more_ready) { Subprocess* subproc; - while ((subproc = subprocs_.NextFinished()) == NULL) { - bool interrupted = subprocs_.DoWork(); + subprocs_.ResetTokenAvailable(); + while (((subproc = subprocs_.NextFinished()) == NULL) && + !subprocs_.IsTokenAvailable()) { + bool interrupted = subprocs_.DoWork(more_ready ? tokens_ : NULL); if (interrupted) return false; } + // token became available + if (subproc == NULL) { + result->status = ExitTokenAvailable; + return true; + } + + // command completed if (tokens_) tokens_->Release(); @@ -639,9 +657,14 @@ bool Builder::Build(string* err) { // command runner. // Second, we attempt to wait for / reap the next finished command. while (plan_.more_to_do()) { - // See if we can start any more commands. - if (failures_allowed && plan_.more_ready() && - command_runner_->CanRunMore()) { + // See if we can start any more commands... + bool can_run_more = + failures_allowed && + plan_.more_ready() && + command_runner_->CanRunMore(); + + // ... but we also need a token to do that. + if (can_run_more && command_runner_->AcquireToken()) { Edge* edge = plan_.FindWork(); if (edge->GetBindingBool("generator")) { scan_.build_log()->Close(); @@ -670,7 +693,7 @@ bool Builder::Build(string* err) { // See if we can reap any finished commands. if (pending_commands) { CommandRunner::Result result; - if (!command_runner_->WaitForCommand(&result) || + if (!command_runner_->WaitForCommand(&result, can_run_more) || result.status == ExitInterrupted) { Cleanup(); status_->BuildFinished(); @@ -678,6 +701,10 @@ bool Builder::Build(string* err) { return false; } + // We might be able to start another command; start the main loop over. + if (result.status == ExitTokenAvailable) + continue; + --pending_commands; if (!FinishCommand(&result, err)) { Cleanup(); diff --git a/src/build.h b/src/build.h index 7dcd111e61..35c7b97d12 100644 --- a/src/build.h +++ b/src/build.h @@ -139,6 +139,7 @@ struct Plan { struct CommandRunner { virtual ~CommandRunner() {} virtual bool CanRunMore() const = 0; + virtual bool AcquireToken() = 0; virtual bool StartCommand(Edge* edge) = 0; /// The result of waiting for a command. @@ -150,7 +151,7 @@ struct CommandRunner { bool success() const { return status == ExitSuccess; } }; /// Wait for a command to complete, or return false if interrupted. - virtual bool WaitForCommand(Result* result) = 0; + virtual bool WaitForCommand(Result* result, bool more_ready) = 0; virtual std::vector GetActiveEdges() { return std::vector(); } virtual void Abort() {} diff --git a/src/build_test.cc b/src/build_test.cc index 4ef62b2113..7a5ff4015a 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -474,8 +474,9 @@ struct FakeCommandRunner : public CommandRunner { // CommandRunner impl virtual bool CanRunMore() const; + virtual bool AcquireToken(); virtual bool StartCommand(Edge* edge); - virtual bool WaitForCommand(Result* result); + virtual bool WaitForCommand(Result* result, bool more_ready); virtual vector GetActiveEdges(); virtual void Abort(); @@ -578,6 +579,10 @@ bool FakeCommandRunner::CanRunMore() const { return active_edges_.size() < max_active_edges_; } +bool FakeCommandRunner::AcquireToken() { + return true; +} + bool FakeCommandRunner::StartCommand(Edge* edge) { assert(active_edges_.size() < max_active_edges_); assert(find(active_edges_.begin(), active_edges_.end(), edge) @@ -649,7 +654,7 @@ bool FakeCommandRunner::StartCommand(Edge* edge) { return true; } -bool FakeCommandRunner::WaitForCommand(Result* result) { +bool FakeCommandRunner::WaitForCommand(Result* result, bool more_ready) { if (active_edges_.empty()) return false; diff --git a/src/exit_status.h b/src/exit_status.h index a714ece791..75ebf6a7a0 100644 --- a/src/exit_status.h +++ b/src/exit_status.h @@ -18,7 +18,8 @@ enum ExitStatus { ExitSuccess, ExitFailure, - ExitInterrupted + ExitTokenAvailable, + ExitInterrupted, }; #endif // NINJA_EXIT_STATUS_H_ diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index 8e785406c9..74451b0be2 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "subprocess.h" +#include "tokenpool.h" #include #include @@ -249,7 +250,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { } #ifdef USE_PPOLL -bool SubprocessSet::DoWork() { +bool SubprocessSet::DoWork(struct TokenPool* tokens) { vector fds; nfds_t nfds = 0; @@ -263,6 +264,12 @@ bool SubprocessSet::DoWork() { ++nfds; } + if (tokens) { + pollfd pfd = { tokens->GetMonitorFd(), POLLIN | POLLPRI, 0 }; + fds.push_back(pfd); + ++nfds; + } + interrupted_ = 0; int ret = ppoll(&fds.front(), nfds, NULL, &old_mask_); if (ret == -1) { @@ -295,11 +302,20 @@ bool SubprocessSet::DoWork() { ++i; } + if (tokens) { + pollfd *pfd = &fds[nfds - 1]; + if (pfd->fd >= 0) { + assert(pfd->fd == tokens->GetMonitorFd()); + if (pfd->revents != 0) + token_available_ = true; + } + } + return IsInterrupted(); } #else // !defined(USE_PPOLL) -bool SubprocessSet::DoWork() { +bool SubprocessSet::DoWork(struct TokenPool* tokens) { fd_set set; int nfds = 0; FD_ZERO(&set); @@ -314,6 +330,13 @@ bool SubprocessSet::DoWork() { } } + if (tokens) { + int fd = tokens->GetMonitorFd(); + FD_SET(fd, &set); + if (nfds < fd+1) + nfds = fd+1; + } + interrupted_ = 0; int ret = pselect(nfds, &set, 0, 0, 0, &old_mask_); if (ret == -1) { @@ -342,6 +365,12 @@ bool SubprocessSet::DoWork() { ++i; } + if (tokens) { + int fd = tokens->GetMonitorFd(); + if ((fd >= 0) && FD_ISSET(fd, &set)) + token_available_ = true; + } + return IsInterrupted(); } #endif // !defined(USE_PPOLL) diff --git a/src/subprocess-win32.cc b/src/subprocess-win32.cc index ff3baaca7f..66d2c2c430 100644 --- a/src/subprocess-win32.cc +++ b/src/subprocess-win32.cc @@ -251,7 +251,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { return subprocess; } -bool SubprocessSet::DoWork() { +bool SubprocessSet::DoWork(struct TokenPool* tokens) { DWORD bytes_read; Subprocess* subproc; OVERLAPPED* overlapped; diff --git a/src/subprocess.h b/src/subprocess.h index 9e3d2ee98f..9ea67ea477 100644 --- a/src/subprocess.h +++ b/src/subprocess.h @@ -76,6 +76,8 @@ struct Subprocess { friend struct SubprocessSet; }; +struct TokenPool; + /// SubprocessSet runs a ppoll/pselect() loop around a set of Subprocesses. /// DoWork() waits for any state change in subprocesses; finished_ /// is a queue of subprocesses as they finish. @@ -84,13 +86,17 @@ struct SubprocessSet { ~SubprocessSet(); Subprocess* Add(const std::string& command, bool use_console = false); - bool DoWork(); + bool DoWork(struct TokenPool* tokens); Subprocess* NextFinished(); void Clear(); std::vector running_; std::queue finished_; + bool token_available_; + bool IsTokenAvailable() { return token_available_; } + void ResetTokenAvailable() { token_available_ = false; } + #ifdef _WIN32 static BOOL WINAPI NotifyInterrupted(DWORD dwCtrlType); static HANDLE ioport_; diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 073fe86931..4bc8083e26 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -45,10 +45,12 @@ TEST_F(SubprocessTest, BadCommandStderr) { Subprocess* subproc = subprocs_.Add("cmd /c ninja_no_such_command"); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { // Pretend we discovered that stderr was ready for writing. - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -59,10 +61,12 @@ TEST_F(SubprocessTest, NoSuchCommand) { Subprocess* subproc = subprocs_.Add("ninja_no_such_command"); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { // Pretend we discovered that stderr was ready for writing. - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -78,9 +82,11 @@ TEST_F(SubprocessTest, InterruptChild) { Subprocess* subproc = subprocs_.Add("kill -INT $$"); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -90,7 +96,7 @@ TEST_F(SubprocessTest, InterruptParent) { ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { - bool interrupted = subprocs_.DoWork(); + bool interrupted = subprocs_.DoWork(NULL); if (interrupted) return; } @@ -102,9 +108,11 @@ TEST_F(SubprocessTest, InterruptChildWithSigTerm) { Subprocess* subproc = subprocs_.Add("kill -TERM $$"); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -114,7 +122,7 @@ TEST_F(SubprocessTest, InterruptParentWithSigTerm) { ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { - bool interrupted = subprocs_.DoWork(); + bool interrupted = subprocs_.DoWork(NULL); if (interrupted) return; } @@ -126,9 +134,11 @@ TEST_F(SubprocessTest, InterruptChildWithSigHup) { Subprocess* subproc = subprocs_.Add("kill -HUP $$"); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -138,7 +148,7 @@ TEST_F(SubprocessTest, InterruptParentWithSigHup) { ASSERT_NE((Subprocess *) 0, subproc); while (!subproc->Done()) { - bool interrupted = subprocs_.DoWork(); + bool interrupted = subprocs_.DoWork(NULL); if (interrupted) return; } @@ -153,9 +163,11 @@ TEST_F(SubprocessTest, Console) { subprocs_.Add("test -t 0 -a -t 1 -a -t 2", /*use_console=*/true); ASSERT_NE((Subprocess*)0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitSuccess, subproc->Finish()); } @@ -167,9 +179,11 @@ TEST_F(SubprocessTest, SetWithSingle) { Subprocess* subproc = subprocs_.Add(kSimpleCommand); ASSERT_NE((Subprocess *) 0, subproc); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_NE("", subproc->GetOutput()); @@ -200,12 +214,13 @@ TEST_F(SubprocessTest, SetWithMulti) { ASSERT_EQ("", processes[i]->GetOutput()); } + subprocs_.ResetTokenAvailable(); while (!processes[0]->Done() || !processes[1]->Done() || !processes[2]->Done()) { ASSERT_GT(subprocs_.running_.size(), 0u); - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } - + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); ASSERT_EQ(0u, subprocs_.running_.size()); ASSERT_EQ(3u, subprocs_.finished_.size()); @@ -237,8 +252,10 @@ TEST_F(SubprocessTest, SetWithLots) { ASSERT_NE((Subprocess *) 0, subproc); procs.push_back(subproc); } + subprocs_.ResetTokenAvailable(); while (!subprocs_.running_.empty()) - subprocs_.DoWork(); + subprocs_.DoWork(NULL); + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); for (size_t i = 0; i < procs.size(); ++i) { ASSERT_EQ(ExitSuccess, procs[i]->Finish()); ASSERT_NE("", procs[i]->GetOutput()); @@ -254,9 +271,11 @@ TEST_F(SubprocessTest, SetWithLots) { // that stdin is closed. TEST_F(SubprocessTest, ReadStdin) { Subprocess* subproc = subprocs_.Add("cat -"); + subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { - subprocs_.DoWork(); + subprocs_.DoWork(NULL); } + ASSERT_EQ(false, subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_EQ(1u, subprocs_.finished_.size()); } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index a8f9b7139d..396bb7d874 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -33,6 +33,7 @@ struct GNUmakeTokenPool : public TokenPool { virtual void Reserve(); virtual void Release(); virtual void Clear(); + virtual int GetMonitorFd(); bool Setup(); @@ -201,6 +202,10 @@ void GNUmakeTokenPool::Clear() { Return(); } +int GNUmakeTokenPool::GetMonitorFd() { + return(rfd_); +} + struct TokenPool *TokenPool::Get(void) { GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; if (tokenpool->Setup()) diff --git a/src/tokenpool.h b/src/tokenpool.h index f560b1083b..301e1998ee 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -21,6 +21,12 @@ struct TokenPool { virtual void Release() = 0; virtual void Clear() = 0; +#ifdef _WIN32 + // @TODO +#else + virtual int GetMonitorFd() = 0; +#endif + // returns NULL if token pool is not available static struct TokenPool *Get(void); }; From d09f3d77821b3b1fdf09fc0ef8e814907675eafb Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Sun, 12 Nov 2017 16:58:55 +0200 Subject: [PATCH 03/11] Ignore jobserver when -jN is forced on command line This emulates the behaviour of GNU make. - add parallelism_from_cmdline flag to build configuration - set the flag when -jN is given on command line - pass the flag to TokenPool::Get() - GNUmakeTokenPool::Setup() * prints a warning when the flag is true and jobserver was detected * returns false, i.e. jobserver will be ignored - ignore config.parallelism in CanRunMore() when we have a valid TokenPool, because it gets always initialized to a default when not given on the command line --- src/build.cc | 10 ++++++---- src/build.h | 4 +++- src/ninja.cc | 1 + src/tokenpool-gnu-make.cc | 34 +++++++++++++++++++--------------- src/tokenpool-none.cc | 4 ++-- src/tokenpool.h | 4 ++-- 6 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/build.cc b/src/build.cc index a25c349050..406a84ec39 100644 --- a/src/build.cc +++ b/src/build.cc @@ -470,7 +470,7 @@ struct RealCommandRunner : public CommandRunner { }; RealCommandRunner::RealCommandRunner(const BuildConfig& config) : config_(config) { - tokens_ = TokenPool::Get(); + tokens_ = TokenPool::Get(config_.parallelism_from_cmdline); } RealCommandRunner::~RealCommandRunner() { @@ -492,9 +492,11 @@ void RealCommandRunner::Abort() { } bool RealCommandRunner::CanRunMore() const { - size_t subproc_number = - subprocs_.running_.size() + subprocs_.finished_.size(); - return (int)subproc_number < config_.parallelism + bool parallelism_limit_not_reached = + tokens_ || // ignore config_.parallelism + ((int) (subprocs_.running_.size() + + subprocs_.finished_.size()) < config_.parallelism); + return parallelism_limit_not_reached && (subprocs_.running_.empty() || (config_.max_load_average <= 0.0f || GetLoadAverage() < config_.max_load_average)); diff --git a/src/build.h b/src/build.h index 35c7b97d12..dfde576573 100644 --- a/src/build.h +++ b/src/build.h @@ -159,7 +159,8 @@ struct CommandRunner { /// Options (e.g. verbosity, parallelism) passed to a build. struct BuildConfig { - BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1), + BuildConfig() : verbosity(NORMAL), dry_run(false), + parallelism(1), parallelism_from_cmdline(false), failures_allowed(1), max_load_average(-0.0f) {} enum Verbosity { @@ -171,6 +172,7 @@ struct BuildConfig { Verbosity verbosity; bool dry_run; int parallelism; + bool parallelism_from_cmdline; int failures_allowed; /// The maximum load average we must not exceed. A negative value /// means that we do not have any limit. diff --git a/src/ninja.cc b/src/ninja.cc index df39ba92d1..d904c56c4e 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -1447,6 +1447,7 @@ int ReadFlags(int* argc, char*** argv, // We want to run N jobs in parallel. For N = 0, INT_MAX // is close enough to infinite for most sane builds. config->parallelism = value > 0 ? value : INT_MAX; + config->parallelism_from_cmdline = true; deferGuessParallelism.needGuess = false; break; } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index 396bb7d874..af4be05a31 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -1,4 +1,4 @@ -// Copyright 2016 Google Inc. All Rights Reserved. +// Copyright 2016-2017 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -35,7 +35,7 @@ struct GNUmakeTokenPool : public TokenPool { virtual void Clear(); virtual int GetMonitorFd(); - bool Setup(); + bool Setup(bool ignore); private: int available_; @@ -100,7 +100,7 @@ bool GNUmakeTokenPool::SetAlarmHandler() { } } -bool GNUmakeTokenPool::Setup() { +bool GNUmakeTokenPool::Setup(bool ignore) { const char *value = getenv("MAKEFLAGS"); if (value) { // GNU make <= 4.1 @@ -109,16 +109,20 @@ bool GNUmakeTokenPool::Setup() { if (!jobserver) jobserver = strstr(value, "--jobserver-auth="); if (jobserver) { - int rfd = -1; - int wfd = -1; - if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && - CheckFd(rfd) && - CheckFd(wfd) && - SetAlarmHandler()) { - printf("ninja: using GNU make jobserver.\n"); - rfd_ = rfd; - wfd_ = wfd; - return true; + if (ignore) { + printf("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); + } else { + int rfd = -1; + int wfd = -1; + if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && + CheckFd(rfd) && + CheckFd(wfd) && + SetAlarmHandler()) { + printf("ninja: using GNU make jobserver.\n"); + rfd_ = rfd; + wfd_ = wfd; + return true; + } } } } @@ -206,9 +210,9 @@ int GNUmakeTokenPool::GetMonitorFd() { return(rfd_); } -struct TokenPool *TokenPool::Get(void) { +struct TokenPool *TokenPool::Get(bool ignore) { GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; - if (tokenpool->Setup()) + if (tokenpool->Setup(ignore)) return tokenpool; else delete tokenpool; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc index 602b3316f5..199b22264b 100644 --- a/src/tokenpool-none.cc +++ b/src/tokenpool-none.cc @@ -1,4 +1,4 @@ -// Copyright 2016 Google Inc. All Rights Reserved. +// Copyright 2016-2017 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -22,6 +22,6 @@ #include // No-op TokenPool implementation -struct TokenPool *TokenPool::Get(void) { +struct TokenPool *TokenPool::Get(bool ignore) { return NULL; } diff --git a/src/tokenpool.h b/src/tokenpool.h index 301e1998ee..878a0933c2 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -1,4 +1,4 @@ -// Copyright 2016 Google Inc. All Rights Reserved. +// Copyright 2016-2017 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -28,5 +28,5 @@ struct TokenPool { #endif // returns NULL if token pool is not available - static struct TokenPool *Get(void); + static struct TokenPool *Get(bool ignore); }; From dfe4ca753caee65bf9041e2b4e883dfa172a5c6a Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Sun, 12 Nov 2017 18:04:12 +0200 Subject: [PATCH 04/11] Honor -lN from MAKEFLAGS This emulates the behaviour of GNU make. - build: make a copy of max_load_average and pass it to TokenPool. - GNUmakeTokenPool: if we detect a jobserver and a valid -lN argument in MAKEFLAGS then set max_load_average to N. --- src/build.cc | 10 +++++++--- src/tokenpool-gnu-make.cc | 19 +++++++++++++++---- src/tokenpool-none.cc | 2 +- src/tokenpool.h | 2 +- 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/build.cc b/src/build.cc index 406a84ec39..9e6272d035 100644 --- a/src/build.cc +++ b/src/build.cc @@ -464,13 +464,17 @@ struct RealCommandRunner : public CommandRunner { virtual void Abort(); const BuildConfig& config_; + // copy of config_.max_load_average; can be modified by TokenPool setup + double max_load_average_; SubprocessSet subprocs_; TokenPool *tokens_; map subproc_to_edge_; }; RealCommandRunner::RealCommandRunner(const BuildConfig& config) : config_(config) { - tokens_ = TokenPool::Get(config_.parallelism_from_cmdline); + max_load_average_ = config.max_load_average; + tokens_ = TokenPool::Get(config_.parallelism_from_cmdline, + max_load_average_); } RealCommandRunner::~RealCommandRunner() { @@ -498,8 +502,8 @@ bool RealCommandRunner::CanRunMore() const { subprocs_.finished_.size()) < config_.parallelism); return parallelism_limit_not_reached && (subprocs_.running_.empty() || - (config_.max_load_average <= 0.0f || - GetLoadAverage() < config_.max_load_average)); + (max_load_average_ <= 0.0f || + GetLoadAverage() < max_load_average_)); } bool RealCommandRunner::AcquireToken() { diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index af4be05a31..fb654c4d88 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -35,7 +35,7 @@ struct GNUmakeTokenPool : public TokenPool { virtual void Clear(); virtual int GetMonitorFd(); - bool Setup(bool ignore); + bool Setup(bool ignore, double& max_load_average); private: int available_; @@ -100,7 +100,7 @@ bool GNUmakeTokenPool::SetAlarmHandler() { } } -bool GNUmakeTokenPool::Setup(bool ignore) { +bool GNUmakeTokenPool::Setup(bool ignore, double& max_load_average) { const char *value = getenv("MAKEFLAGS"); if (value) { // GNU make <= 4.1 @@ -118,9 +118,20 @@ bool GNUmakeTokenPool::Setup(bool ignore) { CheckFd(rfd) && CheckFd(wfd) && SetAlarmHandler()) { + const char *l_arg = strstr(value, " -l"); + int load_limit = -1; + printf("ninja: using GNU make jobserver.\n"); rfd_ = rfd; wfd_ = wfd; + + // translate GNU make -lN to ninja -lN + if (l_arg && + (sscanf(l_arg + 3, "%d ", &load_limit) == 1) && + (load_limit > 0)) { + max_load_average = load_limit; + } + return true; } } @@ -210,9 +221,9 @@ int GNUmakeTokenPool::GetMonitorFd() { return(rfd_); } -struct TokenPool *TokenPool::Get(bool ignore) { +struct TokenPool *TokenPool::Get(bool ignore, double& max_load_average) { GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; - if (tokenpool->Setup(ignore)) + if (tokenpool->Setup(ignore, max_load_average)) return tokenpool; else delete tokenpool; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc index 199b22264b..e8e25426c3 100644 --- a/src/tokenpool-none.cc +++ b/src/tokenpool-none.cc @@ -22,6 +22,6 @@ #include // No-op TokenPool implementation -struct TokenPool *TokenPool::Get(bool ignore) { +struct TokenPool *TokenPool::Get(bool ignore, double& max_load_average) { return NULL; } diff --git a/src/tokenpool.h b/src/tokenpool.h index 878a0933c2..f9e8cc2ee0 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -28,5 +28,5 @@ struct TokenPool { #endif // returns NULL if token pool is not available - static struct TokenPool *Get(bool ignore); + static struct TokenPool *Get(bool ignore, double& max_load_average); }; From 1c10047fc6a3269ba42839da19361e09cbc06ff0 Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Wed, 6 Dec 2017 22:14:21 +0200 Subject: [PATCH 05/11] Use LinePrinter for TokenPool messages - replace printf() with calls to LinePrinter - print GNU make jobserver message only when verbose build is requested --- src/build.cc | 1 + src/tokenpool-gnu-make.cc | 22 ++++++++++++++++------ src/tokenpool-none.cc | 4 +++- src/tokenpool.h | 4 +++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/build.cc b/src/build.cc index 9e6272d035..662e4bd7be 100644 --- a/src/build.cc +++ b/src/build.cc @@ -474,6 +474,7 @@ struct RealCommandRunner : public CommandRunner { RealCommandRunner::RealCommandRunner(const BuildConfig& config) : config_(config) { max_load_average_ = config.max_load_average; tokens_ = TokenPool::Get(config_.parallelism_from_cmdline, + config_.verbosity == BuildConfig::VERBOSE, max_load_average_); } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index fb654c4d88..b0d3e6ebc4 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -23,6 +23,8 @@ #include #include +#include "line_printer.h" + // TokenPool implementation for GNU make jobserver // (http://make.mad-scientist.net/papers/jobserver-implementation/) struct GNUmakeTokenPool : public TokenPool { @@ -35,7 +37,7 @@ struct GNUmakeTokenPool : public TokenPool { virtual void Clear(); virtual int GetMonitorFd(); - bool Setup(bool ignore, double& max_load_average); + bool Setup(bool ignore, bool verbose, double& max_load_average); private: int available_; @@ -100,7 +102,9 @@ bool GNUmakeTokenPool::SetAlarmHandler() { } } -bool GNUmakeTokenPool::Setup(bool ignore, double& max_load_average) { +bool GNUmakeTokenPool::Setup(bool ignore, + bool verbose, + double& max_load_average) { const char *value = getenv("MAKEFLAGS"); if (value) { // GNU make <= 4.1 @@ -109,8 +113,10 @@ bool GNUmakeTokenPool::Setup(bool ignore, double& max_load_average) { if (!jobserver) jobserver = strstr(value, "--jobserver-auth="); if (jobserver) { + LinePrinter printer; + if (ignore) { - printf("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); + printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); } else { int rfd = -1; int wfd = -1; @@ -121,7 +127,9 @@ bool GNUmakeTokenPool::Setup(bool ignore, double& max_load_average) { const char *l_arg = strstr(value, " -l"); int load_limit = -1; - printf("ninja: using GNU make jobserver.\n"); + if (verbose) { + printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); + } rfd_ = rfd; wfd_ = wfd; @@ -221,9 +229,11 @@ int GNUmakeTokenPool::GetMonitorFd() { return(rfd_); } -struct TokenPool *TokenPool::Get(bool ignore, double& max_load_average) { +struct TokenPool *TokenPool::Get(bool ignore, + bool verbose, + double& max_load_average) { GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; - if (tokenpool->Setup(ignore, max_load_average)) + if (tokenpool->Setup(ignore, verbose, max_load_average)) return tokenpool; else delete tokenpool; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc index e8e25426c3..1c1c499c8d 100644 --- a/src/tokenpool-none.cc +++ b/src/tokenpool-none.cc @@ -22,6 +22,8 @@ #include // No-op TokenPool implementation -struct TokenPool *TokenPool::Get(bool ignore, double& max_load_average) { +struct TokenPool *TokenPool::Get(bool ignore, + bool verbose, + double& max_load_average) { return NULL; } diff --git a/src/tokenpool.h b/src/tokenpool.h index f9e8cc2ee0..4bf477f20c 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -28,5 +28,7 @@ struct TokenPool { #endif // returns NULL if token pool is not available - static struct TokenPool *Get(bool ignore, double& max_load_average); + static struct TokenPool *Get(bool ignore, + bool verbose, + double& max_load_average); }; From fdbf68416e3574add3bffd0b637d0694fbaba320 Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Sat, 7 Apr 2018 17:11:21 +0300 Subject: [PATCH 06/11] Prepare PR for merging - fix Windows build error in no-op TokenPool implementation - improve Acquire() to block for a maximum of 100ms - address review comments --- src/build.h | 2 ++ src/tokenpool-gnu-make.cc | 53 +++++++++++++++++++++++++++++++++------ src/tokenpool-none.cc | 7 +----- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/src/build.h b/src/build.h index dfde576573..66ddefb888 100644 --- a/src/build.h +++ b/src/build.h @@ -151,6 +151,8 @@ struct CommandRunner { bool success() const { return status == ExitSuccess; } }; /// Wait for a command to complete, or return false if interrupted. + /// If more_ready is true then the optional TokenPool is monitored too + /// and we return when a token becomes available. virtual bool WaitForCommand(Result* result, bool more_ready) = 0; virtual std::vector GetActiveEdges() { return std::vector(); } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index b0d3e6ebc4..4132bb06d9 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -1,4 +1,4 @@ -// Copyright 2016-2017 Google Inc. All Rights Reserved. +// Copyright 2016-2018 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -153,6 +154,15 @@ bool GNUmakeTokenPool::Acquire() { if (available_ > 0) return true; + // Please read + // + // http://make.mad-scientist.net/papers/jobserver-implementation/ + // + // for the reasoning behind the following code. + // + // Try to read one character from the pipe. Returns true on success. + // + // First check if read() would succeed without blocking. #ifdef USE_PPOLL pollfd pollfds[] = {{rfd_, POLLIN, 0}}; int ret = poll(pollfds, 1, 0); @@ -164,33 +174,62 @@ bool GNUmakeTokenPool::Acquire() { int ret = select(rfd_ + 1, &set, NULL, NULL, &timeout); #endif if (ret > 0) { + // Handle potential race condition: + // - the above check succeeded, i.e. read() should not block + // - the character disappears before we call read() + // + // Create a duplicate of rfd_. The duplicate file descriptor dup_rfd_ + // can safely be closed by signal handlers without affecting rfd_. dup_rfd_ = dup(rfd_); if (dup_rfd_ != -1) { struct sigaction act, old_act; int ret = 0; + // Temporarily replace SIGCHLD handler with our own memset(&act, 0, sizeof(act)); act.sa_handler = CloseDupRfd; if (sigaction(SIGCHLD, &act, &old_act) == 0) { - char buf; - - // block until token read, child exits or timeout - alarm(1); - ret = read(dup_rfd_, &buf, 1); - alarm(0); + struct itimerval timeout; + + // install a 100ms timeout that generates SIGALARM on expiration + memset(&timeout, 0, sizeof(timeout)); + timeout.it_value.tv_usec = 100 * 1000; // [ms] -> [usec] + if (setitimer(ITIMER_REAL, &timeout, NULL) == 0) { + char buf; + + // Now try to read() from dup_rfd_. Return values from read(): + // + // 1. token read -> 1 + // 2. pipe closed -> 0 + // 3. alarm expires -> -1 (EINTR) + // 4. child exits -> -1 (EINTR) + // 5. alarm expired before entering read() -> -1 (EBADF) + // 6. child exited before entering read() -> -1 (EBADF) + // 7. child exited before handler is installed -> go to 1 - 3 + ret = read(dup_rfd_, &buf, 1); + + // disarm timer + memset(&timeout, 0, sizeof(timeout)); + setitimer(ITIMER_REAL, &timeout, NULL); + } sigaction(SIGCHLD, &old_act, NULL); } CloseDupRfd(0); + // Case 1 from above list if (ret > 0) { available_++; return true; } } } + + // read() would block, i.e. no token available, + // cases 2-6 from above list or + // select() / poll() / dup() / sigaction() / setitimer() failed return false; } diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc index 1c1c499c8d..4c592875b4 100644 --- a/src/tokenpool-none.cc +++ b/src/tokenpool-none.cc @@ -1,4 +1,4 @@ -// Copyright 2016-2017 Google Inc. All Rights Reserved. +// Copyright 2016-2018 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -14,11 +14,6 @@ #include "tokenpool.h" -#include -#include -#include -#include -#include #include // No-op TokenPool implementation From ec6220a0baf7d3a6eaf1a2b75bf8960ddfe24c2f Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Fri, 25 May 2018 00:17:07 +0300 Subject: [PATCH 07/11] Add tests for TokenPool - TokenPool setup - GetMonitorFd() API - implicit token and tokens in jobserver pipe - Acquire() / Reserve() / Release() protocol - Clear() API --- configure.py | 1 + src/tokenpool_test.cc | 198 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+) create mode 100644 src/tokenpool_test.cc diff --git a/configure.py b/configure.py index db3492c93c..dc8a0066b7 100755 --- a/configure.py +++ b/configure.py @@ -590,6 +590,7 @@ def has_re2c(): 'string_piece_util_test', 'subprocess_test', 'test', + 'tokenpool_test', 'util_test']: objs += cxx(name, variables=cxxvariables) if platform.is_windows(): diff --git a/src/tokenpool_test.cc b/src/tokenpool_test.cc new file mode 100644 index 0000000000..6c89064ca4 --- /dev/null +++ b/src/tokenpool_test.cc @@ -0,0 +1,198 @@ +// Copyright 2018 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool.h" + +#include "test.h" + +#ifndef _WIN32 +#include +#include +#include + +#define ENVIRONMENT_CLEAR() unsetenv("MAKEFLAGS") +#define ENVIRONMENT_INIT(v) setenv("MAKEFLAGS", v, true); +#endif + +namespace { + +const double kLoadAverageDefault = -1.23456789; + +struct TokenPoolTest : public testing::Test { + double load_avg_; + TokenPool *tokens_; +#ifndef _WIN32 + char buf_[1024]; + int fds_[2]; +#endif + + virtual void SetUp() { + load_avg_ = kLoadAverageDefault; + tokens_ = NULL; +#ifndef _WIN32 + ENVIRONMENT_CLEAR(); + if (pipe(fds_) < 0) + ASSERT_TRUE(false); +#endif + } + + void CreatePool(const char *format, bool ignore_jobserver) { +#ifndef _WIN32 + if (format) { + sprintf(buf_, format, fds_[0], fds_[1]); + ENVIRONMENT_INIT(buf_); + } +#endif + tokens_ = TokenPool::Get(ignore_jobserver, false, load_avg_); + } + + void CreateDefaultPool() { + CreatePool("foo --jobserver-auth=%d,%d bar", false); + } + + virtual void TearDown() { + if (tokens_) + delete tokens_; +#ifndef _WIN32 + close(fds_[0]); + close(fds_[1]); + ENVIRONMENT_CLEAR(); +#endif + } +}; + +} // anonymous namespace + +// verifies none implementation +TEST_F(TokenPoolTest, NoTokenPool) { + CreatePool(NULL, false); + + EXPECT_EQ(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); +} + +#ifndef _WIN32 +TEST_F(TokenPoolTest, SuccessfulOldSetup) { + // GNUmake <= 4.1 + CreatePool("foo --jobserver-fds=%d,%d bar", false); + + EXPECT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); +} + +TEST_F(TokenPoolTest, SuccessfulNewSetup) { + // GNUmake => 4.2 + CreateDefaultPool(); + + EXPECT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); +} + +TEST_F(TokenPoolTest, IgnoreWithJN) { + CreatePool("foo --jobserver-auth=%d,%d bar", true); + + EXPECT_EQ(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); +} + +TEST_F(TokenPoolTest, HonorLN) { + CreatePool("foo -l9 --jobserver-auth=%d,%d bar", false); + + EXPECT_NE(NULL, tokens_); + EXPECT_EQ(9.0, load_avg_); +} + +TEST_F(TokenPoolTest, MonitorFD) { + CreateDefaultPool(); + + ASSERT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); + + EXPECT_EQ(fds_[0], tokens_->GetMonitorFd()); +} + +TEST_F(TokenPoolTest, ImplicitToken) { + CreateDefaultPool(); + + ASSERT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); + + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_FALSE(tokens_->Acquire()); + tokens_->Release(); + EXPECT_TRUE(tokens_->Acquire()); +} + +TEST_F(TokenPoolTest, TwoTokens) { + CreateDefaultPool(); + + ASSERT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); + + // implicit token + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_FALSE(tokens_->Acquire()); + + // jobserver offers 2nd token + ASSERT_EQ(1u, write(fds_[1], "T", 1)); + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_FALSE(tokens_->Acquire()); + + // release 2nd token + tokens_->Release(); + EXPECT_TRUE(tokens_->Acquire()); + + // release implict token - must return 2nd token back to jobserver + tokens_->Release(); + EXPECT_TRUE(tokens_->Acquire()); + + // there must be one token in the pipe + EXPECT_EQ(1u, read(fds_[0], buf_, sizeof(buf_))); + + // implicit token + EXPECT_TRUE(tokens_->Acquire()); +} + +TEST_F(TokenPoolTest, Clear) { + CreateDefaultPool(); + + ASSERT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); + + // implicit token + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_FALSE(tokens_->Acquire()); + + // jobserver offers 2nd & 3rd token + ASSERT_EQ(2u, write(fds_[1], "TT", 2)); + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_TRUE(tokens_->Acquire()); + tokens_->Reserve(); + EXPECT_FALSE(tokens_->Acquire()); + + tokens_->Clear(); + EXPECT_TRUE(tokens_->Acquire()); + + // there must be two tokens in the pipe + EXPECT_EQ(2u, read(fds_[0], buf_, sizeof(buf_))); + + // implicit token + EXPECT_TRUE(tokens_->Acquire()); +} +#endif From e59d8858327126d1593fd0b8e607975a79072e92 Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Thu, 24 May 2018 18:52:45 +0300 Subject: [PATCH 08/11] Add tests for subprocess module - add TokenPoolTest stub to provide TokenPool::GetMonitorFd() - add two tests * both tests set up a dummy GNUmake jobserver pipe * both tests call DoWork() with TokenPoolTest * test 1: verify that DoWork() detects when a token is available * test 2: verify that DoWork() works as before without a token - the tests are not compiled in under Windows --- src/subprocess_test.cc | 76 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 4bc8083e26..6264c8bf11 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "subprocess.h" +#include "tokenpool.h" #include "test.h" @@ -34,8 +35,23 @@ const char* kSimpleCommand = "cmd /c dir \\"; const char* kSimpleCommand = "ls /"; #endif +struct TokenPoolTest : public TokenPool { + bool Acquire() { return false; } + void Reserve() {} + void Release() {} + void Clear() {} + +#ifdef _WIN32 + // @TODO +#else + int _fd; + int GetMonitorFd() { return _fd; } +#endif +}; + struct SubprocessTest : public testing::Test { SubprocessSet subprocs_; + TokenPoolTest tokens_; }; } // anonymous namespace @@ -280,3 +296,63 @@ TEST_F(SubprocessTest, ReadStdin) { ASSERT_EQ(1u, subprocs_.finished_.size()); } #endif // _WIN32 + +// @TODO: remove once TokenPool implementation for Windows is available +#ifndef _WIN32 +TEST_F(SubprocessTest, TokenAvailable) { + Subprocess* subproc = subprocs_.Add(kSimpleCommand); + ASSERT_NE((Subprocess *) 0, subproc); + + // simulate GNUmake jobserver pipe with 1 token + int fds[2]; + ASSERT_EQ(0u, pipe(fds)); + tokens_._fd = fds[0]; + ASSERT_EQ(1u, write(fds[1], "T", 1)); + + subprocs_.ResetTokenAvailable(); + subprocs_.DoWork(&tokens_); + + EXPECT_TRUE(subprocs_.IsTokenAvailable()); + EXPECT_EQ(0u, subprocs_.finished_.size()); + + // remove token to let DoWork() wait for command again + char token; + ASSERT_EQ(1u, read(fds[0], &token, 1)); + + while (!subproc->Done()) { + subprocs_.DoWork(&tokens_); + } + + close(fds[1]); + close(fds[0]); + + EXPECT_EQ(ExitSuccess, subproc->Finish()); + EXPECT_NE("", subproc->GetOutput()); + + EXPECT_EQ(1u, subprocs_.finished_.size()); +} + +TEST_F(SubprocessTest, TokenNotAvailable) { + Subprocess* subproc = subprocs_.Add(kSimpleCommand); + ASSERT_NE((Subprocess *) 0, subproc); + + // simulate GNUmake jobserver pipe with 0 tokens + int fds[2]; + ASSERT_EQ(0u, pipe(fds)); + tokens_._fd = fds[0]; + + subprocs_.ResetTokenAvailable(); + while (!subproc->Done()) { + subprocs_.DoWork(&tokens_); + } + + close(fds[1]); + close(fds[0]); + + EXPECT_FALSE(subprocs_.IsTokenAvailable()); + EXPECT_EQ(ExitSuccess, subproc->Finish()); + EXPECT_NE("", subproc->GetOutput()); + + EXPECT_EQ(1u, subprocs_.finished_.size()); +} +#endif // _WIN32 From 0145e2d4db64ea6c21aeb371928e4071f65164eb Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Sat, 26 May 2018 23:17:51 +0300 Subject: [PATCH 09/11] Add tests for build module Add tests that verify the token functionality of the builder main loop. We replace the default fake command runner with a special version where the tests can control each call to AcquireToken(), CanRunMore() and WaitForCommand(). --- src/build_test.cc | 364 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 364 insertions(+) diff --git a/src/build_test.cc b/src/build_test.cc index 7a5ff4015a..dd41dfbe1d 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -15,6 +15,7 @@ #include "build.h" #include +#include #include "build_log.h" #include "deps_log.h" @@ -3990,3 +3991,366 @@ TEST_F(BuildTest, ValidationWithCircularDependency) { EXPECT_FALSE(builder_.AddTarget("out", &err)); EXPECT_EQ("dependency cycle: validate -> validate_in -> validate", err); } + +/// The token tests are concerned with the main loop functionality when +// the CommandRunner has an active TokenPool. It is therefore intentional +// that the plan doesn't complete and that builder_.Build() returns false! + +/// Fake implementation of CommandRunner that simulates a TokenPool +struct FakeTokenCommandRunner : public CommandRunner { + explicit FakeTokenCommandRunner() {} + + // CommandRunner impl + virtual bool CanRunMore() const; + virtual bool AcquireToken(); + virtual bool StartCommand(Edge* edge); + virtual bool WaitForCommand(Result* result, bool more_ready); + virtual vector GetActiveEdges(); + virtual void Abort(); + + vector commands_ran_; + vector edges_; + + vector acquire_token_; + vector can_run_more_; + vector wait_for_command_; +}; + +bool FakeTokenCommandRunner::CanRunMore() const { + if (can_run_more_.size() == 0) { + EXPECT_FALSE("unexpected call to CommandRunner::CanRunMore()"); + return false; + } + + bool result = can_run_more_[0]; + + // Unfortunately CanRunMore() isn't "const" for tests + const_cast(this)->can_run_more_.erase( + const_cast(this)->can_run_more_.begin() + ); + + return result; +} + +bool FakeTokenCommandRunner::AcquireToken() { + if (acquire_token_.size() == 0) { + EXPECT_FALSE("unexpected call to CommandRunner::AcquireToken()"); + return false; + } + + bool result = acquire_token_[0]; + acquire_token_.erase(acquire_token_.begin()); + return result; +} + +bool FakeTokenCommandRunner::StartCommand(Edge* edge) { + commands_ran_.push_back(edge->EvaluateCommand()); + edges_.push_back(edge); + return true; +} + +bool FakeTokenCommandRunner::WaitForCommand(Result* result, bool more_ready) { + if (wait_for_command_.size() == 0) { + EXPECT_FALSE("unexpected call to CommandRunner::WaitForCommand()"); + return false; + } + + bool expected = wait_for_command_[0]; + if (expected != more_ready) { + EXPECT_EQ(expected, more_ready); + return false; + } + wait_for_command_.erase(wait_for_command_.begin()); + + if (edges_.size() == 0) + return false; + + Edge* edge = edges_[0]; + result->edge = edge; + + if (more_ready && + (edge->rule().name() == "token-available")) { + result->status = ExitTokenAvailable; + } else { + edges_.erase(edges_.begin()); + result->status = ExitSuccess; + } + + return true; +} + +vector FakeTokenCommandRunner::GetActiveEdges() { + return edges_; +} + +void FakeTokenCommandRunner::Abort() { + edges_.clear(); +} + +struct BuildTokenTest : public BuildTest { + virtual void SetUp(); + virtual void TearDown(); + + FakeTokenCommandRunner token_command_runner_; + + void ExpectAcquireToken(int count, ...); + void ExpectCanRunMore(int count, ...); + void ExpectWaitForCommand(int count, ...); + +private: + void EnqueueBooleans(vector& booleans, int count, va_list ao); +}; + +void BuildTokenTest::SetUp() { + BuildTest::SetUp(); + + // replace FakeCommandRunner with FakeTokenCommandRunner + builder_.command_runner_.release(); + builder_.command_runner_.reset(&token_command_runner_); +} +void BuildTokenTest::TearDown() { + EXPECT_EQ(0u, token_command_runner_.acquire_token_.size()); + EXPECT_EQ(0u, token_command_runner_.can_run_more_.size()); + EXPECT_EQ(0u, token_command_runner_.wait_for_command_.size()); + + BuildTest::TearDown(); +} + +void BuildTokenTest::ExpectAcquireToken(int count, ...) { + va_list ap; + va_start(ap, count); + EnqueueBooleans(token_command_runner_.acquire_token_, count, ap); + va_end(ap); +} + +void BuildTokenTest::ExpectCanRunMore(int count, ...) { + va_list ap; + va_start(ap, count); + EnqueueBooleans(token_command_runner_.can_run_more_, count, ap); + va_end(ap); +} + +void BuildTokenTest::ExpectWaitForCommand(int count, ...) { + va_list ap; + va_start(ap, count); + EnqueueBooleans(token_command_runner_.wait_for_command_, count, ap); + va_end(ap); +} + +void BuildTokenTest::EnqueueBooleans(vector& booleans, int count, va_list ap) { + while (count--) { + int value = va_arg(ap, int); + booleans.push_back(!!value); // force bool + } +} + +TEST_F(BuildTokenTest, CompleteNoWork) { + // plan should not execute anything + string err; + + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + + EXPECT_EQ(0u, token_command_runner_.commands_ran_.size()); +} + +TEST_F(BuildTokenTest, DoNotAquireToken) { + // plan should execute one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat1", &err)); + ASSERT_EQ("", err); + + // pretend we can't run anything + ExpectCanRunMore(1, false); + + EXPECT_FALSE(builder_.Build(&err)); + EXPECT_EQ("stuck [this is a bug]", err); + + EXPECT_EQ(0u, token_command_runner_.commands_ran_.size()); +} + +TEST_F(BuildTokenTest, DoNotStartWithoutToken) { + // plan should execute one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat1", &err)); + ASSERT_EQ("", err); + + // we could run a command but do not have a token for it + ExpectCanRunMore(1, true); + ExpectAcquireToken(1, false); + + EXPECT_FALSE(builder_.Build(&err)); + EXPECT_EQ("stuck [this is a bug]", err); + + EXPECT_EQ(0u, token_command_runner_.commands_ran_.size()); +} + +TEST_F(BuildTokenTest, CompleteOneStep) { + // plan should execute one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat1", &err)); + ASSERT_EQ("", err); + + // allow running of one command + ExpectCanRunMore(1, true); + ExpectAcquireToken(1, true); + // block and wait for command to finalize + ExpectWaitForCommand(1, false); + + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + + EXPECT_EQ(1u, token_command_runner_.commands_ran_.size()); + EXPECT_TRUE(token_command_runner_.commands_ran_[0] == "cat in1 > cat1"); +} + +TEST_F(BuildTokenTest, AcquireOneToken) { + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat12", &err)); + ASSERT_EQ("", err); + + // allow running of one command + ExpectCanRunMore(3, true, false, false); + ExpectAcquireToken(1, true); + // block and wait for command to finalize + ExpectWaitForCommand(1, false); + + EXPECT_FALSE(builder_.Build(&err)); + EXPECT_EQ("stuck [this is a bug]", err); + + EXPECT_EQ(1u, token_command_runner_.commands_ran_.size()); + // any of the two dependencies could have been executed + EXPECT_TRUE(token_command_runner_.commands_ran_[0] == "cat in1 > cat1" || + token_command_runner_.commands_ran_[0] == "cat in1 in2 > cat2"); +} + +TEST_F(BuildTokenTest, WantTwoTokens) { + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat12", &err)); + ASSERT_EQ("", err); + + // allow running of one command + ExpectCanRunMore(3, true, true, false); + ExpectAcquireToken(2, true, false); + // wait for command to finalize or token to become available + ExpectWaitForCommand(1, true); + + EXPECT_FALSE(builder_.Build(&err)); + EXPECT_EQ("stuck [this is a bug]", err); + + EXPECT_EQ(1u, token_command_runner_.commands_ran_.size()); + // any of the two dependencies could have been executed + EXPECT_TRUE(token_command_runner_.commands_ran_[0] == "cat in1 > cat1" || + token_command_runner_.commands_ran_[0] == "cat in1 in2 > cat2"); +} + +TEST_F(BuildTokenTest, CompleteTwoSteps) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"build out1: cat in1\n" +"build out2: cat out1\n")); + + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("out2", &err)); + ASSERT_EQ("", err); + + // allow running of two commands + ExpectCanRunMore(2, true, true); + ExpectAcquireToken(2, true, true); + // wait for commands to finalize + ExpectWaitForCommand(2, false, false); + + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + + EXPECT_EQ(2u, token_command_runner_.commands_ran_.size()); + EXPECT_TRUE(token_command_runner_.commands_ran_[0] == "cat in1 > out1"); + EXPECT_TRUE(token_command_runner_.commands_ran_[1] == "cat out1 > out2"); +} + +TEST_F(BuildTokenTest, TwoCommandsInParallel) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule token-available\n" +" command = cat $in > $out\n" +"build out1: token-available in1\n" +"build out2: token-available in2\n" +"build out12: cat out1 out2\n")); + + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("out12", &err)); + ASSERT_EQ("", err); + + // 1st command: token available -> allow running + // 2nd command: no token available but becomes available later + ExpectCanRunMore(4, true, true, true, false); + ExpectAcquireToken(3, true, false, true); + // 1st call waits for command to finalize or token to become available + // 2nd call waits for command to finalize + // 3rd call waits for command to finalize + ExpectWaitForCommand(3, true, false, false); + + EXPECT_FALSE(builder_.Build(&err)); + EXPECT_EQ("stuck [this is a bug]", err); + + EXPECT_EQ(2u, token_command_runner_.commands_ran_.size()); + EXPECT_TRUE((token_command_runner_.commands_ran_[0] == "cat in1 > out1" && + token_command_runner_.commands_ran_[1] == "cat in2 > out2") || + (token_command_runner_.commands_ran_[0] == "cat in2 > out2" && + token_command_runner_.commands_ran_[1] == "cat in1 > out1")); +} + +TEST_F(BuildTokenTest, CompleteThreeStepsSerial) { + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("cat12", &err)); + ASSERT_EQ("", err); + + // allow running of all commands + ExpectCanRunMore(4, true, true, true, true); + ExpectAcquireToken(4, true, false, true, true); + // wait for commands to finalize + ExpectWaitForCommand(3, true, false, false); + + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + + EXPECT_EQ(3u, token_command_runner_.commands_ran_.size()); + EXPECT_TRUE((token_command_runner_.commands_ran_[0] == "cat in1 > cat1" && + token_command_runner_.commands_ran_[1] == "cat in1 in2 > cat2") || + (token_command_runner_.commands_ran_[0] == "cat in1 in2 > cat2" && + token_command_runner_.commands_ran_[1] == "cat in1 > cat1" )); + EXPECT_TRUE(token_command_runner_.commands_ran_[2] == "cat cat1 cat2 > cat12"); +} + +TEST_F(BuildTokenTest, CompleteThreeStepsParallel) { + ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, +"rule token-available\n" +" command = cat $in > $out\n" +"build out1: token-available in1\n" +"build out2: token-available in2\n" +"build out12: cat out1 out2\n")); + + // plan should execute more than one command + string err; + EXPECT_TRUE(builder_.AddTarget("out12", &err)); + ASSERT_EQ("", err); + + // allow running of all commands + ExpectCanRunMore(4, true, true, true, true); + ExpectAcquireToken(4, true, false, true, true); + // wait for commands to finalize + ExpectWaitForCommand(4, true, false, false, false); + + EXPECT_TRUE(builder_.Build(&err)); + EXPECT_EQ("", err); + + EXPECT_EQ(3u, token_command_runner_.commands_ran_.size()); + EXPECT_TRUE((token_command_runner_.commands_ran_[0] == "cat in1 > out1" && + token_command_runner_.commands_ran_[1] == "cat in2 > out2") || + (token_command_runner_.commands_ran_[0] == "cat in2 > out2" && + token_command_runner_.commands_ran_[1] == "cat in1 > out1")); + EXPECT_TRUE(token_command_runner_.commands_ran_[2] == "cat out1 out2 > out12"); +} From f016e5430c9123d34a73ea7ad28693b20ee59d6d Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Mon, 8 Oct 2018 17:47:50 +0300 Subject: [PATCH 10/11] Add Win32 implementation for GNUmakeTokenPool GNU make uses a semaphore as jobserver protocol on Win32. See also https://www.gnu.org/software/make/manual/html_node/Windows-Jobserver.html Usage is pretty simple and straightforward, i.e. WaitForSingleObject() to obtain a token and ReleaseSemaphore() to return it. Unfortunately subprocess-win32.cc uses an I/O completion port (IOCP). IOCPs aren't waitable objects, i.e. we can't use WaitForMultipleObjects() to wait on the IOCP and the token semaphore at the same time. Therefore GNUmakeTokenPoolWin32 creates a child thread that waits on the token semaphore and posts a dummy I/O completion status on the IOCP when it was able to obtain a token. That unblocks SubprocessSet::DoWork() and it can then check if a token became available or not. - split existing GNUmakeTokenPool into common and platform bits - add GNUmakeTokenPool interface - move the Posix bits to GNUmakeTokenPoolPosix - add the Win32 bits as GNUmakeTokenPoolWin32 - move Setup() method up to TokenPool interface - update Subprocess & TokenPool tests accordingly --- configure.py | 8 +- src/build.cc | 11 +- src/subprocess-win32.cc | 9 ++ src/subprocess_test.cc | 34 ++++- src/tokenpool-gnu-make-posix.cc | 203 +++++++++++++++++++++++++++ src/tokenpool-gnu-make-win32.cc | 237 ++++++++++++++++++++++++++++++++ src/tokenpool-gnu-make.cc | 203 ++------------------------- src/tokenpool-gnu-make.h | 40 ++++++ src/tokenpool-none.cc | 4 +- src/tokenpool.h | 18 ++- src/tokenpool_test.cc | 113 ++++++++++++--- 11 files changed, 653 insertions(+), 227 deletions(-) create mode 100644 src/tokenpool-gnu-make-posix.cc create mode 100644 src/tokenpool-gnu-make-win32.cc create mode 100644 src/tokenpool-gnu-make.h diff --git a/configure.py b/configure.py index dc8a0066b7..a239b90eef 100755 --- a/configure.py +++ b/configure.py @@ -517,12 +517,13 @@ def has_re2c(): 'state', 'status', 'string_piece_util', + 'tokenpool-gnu-make', 'util', 'version']: objs += cxx(name, variables=cxxvariables) if platform.is_windows(): for name in ['subprocess-win32', - 'tokenpool-none', + 'tokenpool-gnu-make-win32', 'includes_normalize-win32', 'msvc_helper-win32', 'msvc_helper_main-win32']: @@ -531,8 +532,9 @@ def has_re2c(): objs += cxx('minidump-win32', variables=cxxvariables) objs += cc('getopt') else: - objs += cxx('subprocess-posix') - objs += cxx('tokenpool-gnu-make') + for name in ['subprocess-posix', + 'tokenpool-gnu-make-posix']: + objs += cxx(name) if platform.is_aix(): objs += cc('getopt') if platform.is_msvc(): diff --git a/src/build.cc b/src/build.cc index 662e4bd7be..20c3bdc2a0 100644 --- a/src/build.cc +++ b/src/build.cc @@ -473,9 +473,14 @@ struct RealCommandRunner : public CommandRunner { RealCommandRunner::RealCommandRunner(const BuildConfig& config) : config_(config) { max_load_average_ = config.max_load_average; - tokens_ = TokenPool::Get(config_.parallelism_from_cmdline, - config_.verbosity == BuildConfig::VERBOSE, - max_load_average_); + if ((tokens_ = TokenPool::Get()) != NULL) { + if (!tokens_->Setup(config_.parallelism_from_cmdline, + config_.verbosity == BuildConfig::VERBOSE, + max_load_average_)) { + delete tokens_; + tokens_ = NULL; + } + } } RealCommandRunner::~RealCommandRunner() { diff --git a/src/subprocess-win32.cc b/src/subprocess-win32.cc index 66d2c2c430..ce3e2c20a4 100644 --- a/src/subprocess-win32.cc +++ b/src/subprocess-win32.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "subprocess.h" +#include "tokenpool.h" #include #include @@ -256,6 +257,9 @@ bool SubprocessSet::DoWork(struct TokenPool* tokens) { Subprocess* subproc; OVERLAPPED* overlapped; + if (tokens) + tokens->WaitForTokenAvailability(ioport_); + if (!GetQueuedCompletionStatus(ioport_, &bytes_read, (PULONG_PTR)&subproc, &overlapped, INFINITE)) { if (GetLastError() != ERROR_BROKEN_PIPE) @@ -266,6 +270,11 @@ bool SubprocessSet::DoWork(struct TokenPool* tokens) { // delivered by NotifyInterrupted above. return true; + if (tokens && tokens->TokenIsAvailable((ULONG_PTR)subproc)) { + token_available_ = true; + return false; + } + subproc->OnPipeReady(); if (subproc->Done()) { diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index 6264c8bf11..f625963462 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -40,9 +40,16 @@ struct TokenPoolTest : public TokenPool { void Reserve() {} void Release() {} void Clear() {} + bool Setup(bool ignore_unused, bool verbose, double& max_load_average) { return false; } #ifdef _WIN32 - // @TODO + bool _token_available; + void WaitForTokenAvailability(HANDLE ioport) { + if (_token_available) + // unblock GetQueuedCompletionStatus() + PostQueuedCompletionStatus(ioport, 0, (ULONG_PTR) this, NULL); + } + bool TokenIsAvailable(ULONG_PTR key) { return key == (ULONG_PTR) this; } #else int _fd; int GetMonitorFd() { return _fd; } @@ -297,34 +304,48 @@ TEST_F(SubprocessTest, ReadStdin) { } #endif // _WIN32 -// @TODO: remove once TokenPool implementation for Windows is available -#ifndef _WIN32 TEST_F(SubprocessTest, TokenAvailable) { Subprocess* subproc = subprocs_.Add(kSimpleCommand); ASSERT_NE((Subprocess *) 0, subproc); // simulate GNUmake jobserver pipe with 1 token +#ifdef _WIN32 + tokens_._token_available = true; +#else int fds[2]; ASSERT_EQ(0u, pipe(fds)); tokens_._fd = fds[0]; ASSERT_EQ(1u, write(fds[1], "T", 1)); +#endif subprocs_.ResetTokenAvailable(); subprocs_.DoWork(&tokens_); +#ifdef _WIN32 + tokens_._token_available = false; + // we need to loop here as we have no conrol where the token + // I/O completion post ends up in the queue + while (!subproc->Done() && !subprocs_.IsTokenAvailable()) { + subprocs_.DoWork(&tokens_); + } +#endif EXPECT_TRUE(subprocs_.IsTokenAvailable()); EXPECT_EQ(0u, subprocs_.finished_.size()); // remove token to let DoWork() wait for command again +#ifndef _WIN32 char token; ASSERT_EQ(1u, read(fds[0], &token, 1)); +#endif while (!subproc->Done()) { subprocs_.DoWork(&tokens_); } +#ifndef _WIN32 close(fds[1]); close(fds[0]); +#endif EXPECT_EQ(ExitSuccess, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -337,17 +358,23 @@ TEST_F(SubprocessTest, TokenNotAvailable) { ASSERT_NE((Subprocess *) 0, subproc); // simulate GNUmake jobserver pipe with 0 tokens +#ifdef _WIN32 + tokens_._token_available = false; +#else int fds[2]; ASSERT_EQ(0u, pipe(fds)); tokens_._fd = fds[0]; +#endif subprocs_.ResetTokenAvailable(); while (!subproc->Done()) { subprocs_.DoWork(&tokens_); } +#ifndef _WIN32 close(fds[1]); close(fds[0]); +#endif EXPECT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitSuccess, subproc->Finish()); @@ -355,4 +382,3 @@ TEST_F(SubprocessTest, TokenNotAvailable) { EXPECT_EQ(1u, subprocs_.finished_.size()); } -#endif // _WIN32 diff --git a/src/tokenpool-gnu-make-posix.cc b/src/tokenpool-gnu-make-posix.cc new file mode 100644 index 0000000000..70d84bfff7 --- /dev/null +++ b/src/tokenpool-gnu-make-posix.cc @@ -0,0 +1,203 @@ +// Copyright 2016-2018 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool-gnu-make.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// TokenPool implementation for GNU make jobserver - POSIX implementation +// (http://make.mad-scientist.net/papers/jobserver-implementation/) +struct GNUmakeTokenPoolPosix : public GNUmakeTokenPool { + GNUmakeTokenPoolPosix(); + virtual ~GNUmakeTokenPoolPosix(); + + virtual int GetMonitorFd(); + + virtual const char *GetEnv(const char *name) { return getenv(name); }; + virtual bool ParseAuth(const char *jobserver); + virtual bool AcquireToken(); + virtual bool ReturnToken(); + + private: + int rfd_; + int wfd_; + + struct sigaction old_act_; + bool restore_; + + static int dup_rfd_; + static void CloseDupRfd(int signum); + + bool CheckFd(int fd); + bool SetAlarmHandler(); +}; + +GNUmakeTokenPoolPosix::GNUmakeTokenPoolPosix() : rfd_(-1), wfd_(-1), restore_(false) { +} + +GNUmakeTokenPoolPosix::~GNUmakeTokenPoolPosix() { + Clear(); + if (restore_) + sigaction(SIGALRM, &old_act_, NULL); +} + +bool GNUmakeTokenPoolPosix::CheckFd(int fd) { + if (fd < 0) + return false; + int ret = fcntl(fd, F_GETFD); + if (ret < 0) + return false; + return true; +} + +int GNUmakeTokenPoolPosix::dup_rfd_ = -1; + +void GNUmakeTokenPoolPosix::CloseDupRfd(int signum) { + close(dup_rfd_); + dup_rfd_ = -1; +} + +bool GNUmakeTokenPoolPosix::SetAlarmHandler() { + struct sigaction act; + memset(&act, 0, sizeof(act)); + act.sa_handler = CloseDupRfd; + if (sigaction(SIGALRM, &act, &old_act_) < 0) { + perror("sigaction:"); + return(false); + } else { + restore_ = true; + return(true); + } +} + +bool GNUmakeTokenPoolPosix::ParseAuth(const char *jobserver) { + int rfd = -1; + int wfd = -1; + if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && + CheckFd(rfd) && + CheckFd(wfd) && + SetAlarmHandler()) { + rfd_ = rfd; + wfd_ = wfd; + return true; + } + + return false; +} + +bool GNUmakeTokenPoolPosix::AcquireToken() { + // Please read + // + // http://make.mad-scientist.net/papers/jobserver-implementation/ + // + // for the reasoning behind the following code. + // + // Try to read one character from the pipe. Returns true on success. + // + // First check if read() would succeed without blocking. +#ifdef USE_PPOLL + pollfd pollfds[] = {{rfd_, POLLIN, 0}}; + int ret = poll(pollfds, 1, 0); +#else + fd_set set; + struct timeval timeout = { 0, 0 }; + FD_ZERO(&set); + FD_SET(rfd_, &set); + int ret = select(rfd_ + 1, &set, NULL, NULL, &timeout); +#endif + if (ret > 0) { + // Handle potential race condition: + // - the above check succeeded, i.e. read() should not block + // - the character disappears before we call read() + // + // Create a duplicate of rfd_. The duplicate file descriptor dup_rfd_ + // can safely be closed by signal handlers without affecting rfd_. + dup_rfd_ = dup(rfd_); + + if (dup_rfd_ != -1) { + struct sigaction act, old_act; + int ret = 0; + + // Temporarily replace SIGCHLD handler with our own + memset(&act, 0, sizeof(act)); + act.sa_handler = CloseDupRfd; + if (sigaction(SIGCHLD, &act, &old_act) == 0) { + struct itimerval timeout; + + // install a 100ms timeout that generates SIGALARM on expiration + memset(&timeout, 0, sizeof(timeout)); + timeout.it_value.tv_usec = 100 * 1000; // [ms] -> [usec] + if (setitimer(ITIMER_REAL, &timeout, NULL) == 0) { + char buf; + + // Now try to read() from dup_rfd_. Return values from read(): + // + // 1. token read -> 1 + // 2. pipe closed -> 0 + // 3. alarm expires -> -1 (EINTR) + // 4. child exits -> -1 (EINTR) + // 5. alarm expired before entering read() -> -1 (EBADF) + // 6. child exited before entering read() -> -1 (EBADF) + // 7. child exited before handler is installed -> go to 1 - 3 + ret = read(dup_rfd_, &buf, 1); + + // disarm timer + memset(&timeout, 0, sizeof(timeout)); + setitimer(ITIMER_REAL, &timeout, NULL); + } + + sigaction(SIGCHLD, &old_act, NULL); + } + + CloseDupRfd(0); + + // Case 1 from above list + if (ret > 0) + return true; + } + } + + // read() would block, i.e. no token available, + // cases 2-6 from above list or + // select() / poll() / dup() / sigaction() / setitimer() failed + return false; +} + +bool GNUmakeTokenPoolPosix::ReturnToken() { + const char buf = '+'; + while (1) { + int ret = write(wfd_, &buf, 1); + if (ret > 0) + return true; + if ((ret != -1) || (errno != EINTR)) + return false; + // write got interrupted - retry + } +} + +int GNUmakeTokenPoolPosix::GetMonitorFd() { + return(rfd_); +} + +struct TokenPool *TokenPool::Get() { + return new GNUmakeTokenPoolPosix; +} diff --git a/src/tokenpool-gnu-make-win32.cc b/src/tokenpool-gnu-make-win32.cc new file mode 100644 index 0000000000..2719f2c1fc --- /dev/null +++ b/src/tokenpool-gnu-make-win32.cc @@ -0,0 +1,237 @@ +// Copyright 2018 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool-gnu-make.h" + +// always include first to make sure other headers do the correct thing... +#include + +#include +#include +#include + +#include "util.h" + +// TokenPool implementation for GNU make jobserver - Win32 implementation +// (https://www.gnu.org/software/make/manual/html_node/Windows-Jobserver.html) +struct GNUmakeTokenPoolWin32 : public GNUmakeTokenPool { + GNUmakeTokenPoolWin32(); + virtual ~GNUmakeTokenPoolWin32(); + + virtual void WaitForTokenAvailability(HANDLE ioport); + virtual bool TokenIsAvailable(ULONG_PTR key); + + virtual const char *GetEnv(const char *name); + virtual bool ParseAuth(const char *jobserver); + virtual bool AcquireToken(); + virtual bool ReturnToken(); + + private: + // Semaphore for GNU make jobserver protocol + HANDLE semaphore_jobserver_; + // Semaphore Child -> Parent + // - child releases it before entering wait on jobserver semaphore + // - parent blocks on it to know when child enters wait + HANDLE semaphore_enter_wait_; + // Semaphore Parent -> Child + // - parent releases it to allow child to restart loop + // - child blocks on it to know when to restart loop + HANDLE semaphore_restart_; + // set to false if child should exit loop and terminate thread + bool running_; + // child thread + HANDLE child_; + // I/O completion port from SubprocessSet + HANDLE ioport_; + + + DWORD SemaphoreThread(); + void ReleaseSemaphore(HANDLE semaphore); + void WaitForObject(HANDLE object); + static DWORD WINAPI SemaphoreThreadWrapper(LPVOID param); + static void NoopAPCFunc(ULONG_PTR param); +}; + +GNUmakeTokenPoolWin32::GNUmakeTokenPoolWin32() : semaphore_jobserver_(NULL), + semaphore_enter_wait_(NULL), + semaphore_restart_(NULL), + running_(false), + child_(NULL), + ioport_(NULL) { +} + +GNUmakeTokenPoolWin32::~GNUmakeTokenPoolWin32() { + Clear(); + CloseHandle(semaphore_jobserver_); + semaphore_jobserver_ = NULL; + + if (child_) { + // tell child thread to exit + running_ = false; + ReleaseSemaphore(semaphore_restart_); + + // wait for child thread to exit + WaitForObject(child_); + CloseHandle(child_); + child_ = NULL; + } + + if (semaphore_restart_) { + CloseHandle(semaphore_restart_); + semaphore_restart_ = NULL; + } + + if (semaphore_enter_wait_) { + CloseHandle(semaphore_enter_wait_); + semaphore_enter_wait_ = NULL; + } +} + +const char *GNUmakeTokenPoolWin32::GetEnv(const char *name) { + // getenv() does not work correctly together with tokenpool_tests.cc + static char buffer[MAX_PATH + 1]; + if (GetEnvironmentVariable("MAKEFLAGS", buffer, sizeof(buffer)) == 0) + return NULL; + return(buffer); +} + +bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) { + // match "--jobserver-auth=gmake_semaphore_..." + const char *start = strchr(jobserver, '='); + if (start) { + const char *end = start; + unsigned int len; + char c, *auth; + + while ((c = *++end) != '\0') + if (!(isalnum(c) || (c == '_'))) + break; + len = end - start; // includes string terminator in count + + if ((len > 1) && ((auth = (char *)malloc(len)) != NULL)) { + strncpy(auth, start + 1, len - 1); + auth[len - 1] = '\0'; + + if ((semaphore_jobserver_ = OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */ + FALSE, /* Child processes DON'T inherit */ + auth /* Semaphore name */ + )) != NULL) { + free(auth); + return true; + } + + free(auth); + } + } + + return false; +} + +bool GNUmakeTokenPoolWin32::AcquireToken() { + return WaitForSingleObject(semaphore_jobserver_, 0) == WAIT_OBJECT_0; +} + +bool GNUmakeTokenPoolWin32::ReturnToken() { + ReleaseSemaphore(semaphore_jobserver_); + return true; +} + +DWORD GNUmakeTokenPoolWin32::SemaphoreThread() { + while (running_) { + // indicate to parent that we are entering wait + ReleaseSemaphore(semaphore_enter_wait_); + + // alertable wait forever on token semaphore + if (WaitForSingleObjectEx(semaphore_jobserver_, INFINITE, TRUE) == WAIT_OBJECT_0) { + // release token again for AcquireToken() + ReleaseSemaphore(semaphore_jobserver_); + + // indicate to parent on ioport that a token might be available + if (!PostQueuedCompletionStatus(ioport_, 0, (ULONG_PTR) this, NULL)) + Win32Fatal("PostQueuedCompletionStatus"); + } + + // wait for parent to allow loop restart + WaitForObject(semaphore_restart_); + // semaphore is now in nonsignaled state again for next run... + } + + return 0; +} + +DWORD WINAPI GNUmakeTokenPoolWin32::SemaphoreThreadWrapper(LPVOID param) { + GNUmakeTokenPoolWin32 *This = (GNUmakeTokenPoolWin32 *) param; + return This->SemaphoreThread(); +} + +void GNUmakeTokenPoolWin32::NoopAPCFunc(ULONG_PTR param) { +} + +void GNUmakeTokenPoolWin32::WaitForTokenAvailability(HANDLE ioport) { + if (child_ == NULL) { + // first invocation + // + // subprocess-win32.cc uses I/O completion port (IOCP) which can't be + // used as a waitable object. Therefore we can't use WaitMultipleObjects() + // to wait on the IOCP and the token semaphore at the same time. Create + // a child thread that waits on the semaphore and posts an I/O completion + ioport_ = ioport; + + // create both semaphores in nonsignaled state + if ((semaphore_enter_wait_ = CreateSemaphore(NULL, 0, 1, NULL)) + == NULL) + Win32Fatal("CreateSemaphore/enter_wait"); + if ((semaphore_restart_ = CreateSemaphore(NULL, 0, 1, NULL)) + == NULL) + Win32Fatal("CreateSemaphore/restart"); + + // start child thread + running_ = true; + if ((child_ = CreateThread(NULL, 0, &SemaphoreThreadWrapper, this, 0, NULL)) + == NULL) + Win32Fatal("CreateThread"); + + } else { + // all further invocations - allow child thread to loop + ReleaseSemaphore(semaphore_restart_); + } + + // wait for child thread to enter wait + WaitForObject(semaphore_enter_wait_); + // semaphore is now in nonsignaled state again for next run... + + // now SubprocessSet::DoWork() can enter GetQueuedCompletionStatus()... +} + +bool GNUmakeTokenPoolWin32::TokenIsAvailable(ULONG_PTR key) { + // alert child thread to break wait on token semaphore + QueueUserAPC(&NoopAPCFunc, child_, (ULONG_PTR)NULL); + + // return true when GetQueuedCompletionStatus() returned our key + return key == (ULONG_PTR) this; +} + +void GNUmakeTokenPoolWin32::ReleaseSemaphore(HANDLE semaphore) { + if (!::ReleaseSemaphore(semaphore, 1, NULL)) + Win32Fatal("ReleaseSemaphore"); +} + +void GNUmakeTokenPoolWin32::WaitForObject(HANDLE object) { + if (WaitForSingleObject(object, INFINITE) != WAIT_OBJECT_0) + Win32Fatal("WaitForSingleObject"); +} + +struct TokenPool *TokenPool::Get() { + return new GNUmakeTokenPoolWin32; +} diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index 4132bb06d9..92ff611721 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -12,101 +12,26 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "tokenpool.h" +#include "tokenpool-gnu-make.h" -#include -#include -#include -#include -#include -#include +#include #include #include -#include #include "line_printer.h" -// TokenPool implementation for GNU make jobserver -// (http://make.mad-scientist.net/papers/jobserver-implementation/) -struct GNUmakeTokenPool : public TokenPool { - GNUmakeTokenPool(); - virtual ~GNUmakeTokenPool(); - - virtual bool Acquire(); - virtual void Reserve(); - virtual void Release(); - virtual void Clear(); - virtual int GetMonitorFd(); - - bool Setup(bool ignore, bool verbose, double& max_load_average); - - private: - int available_; - int used_; - -#ifdef _WIN32 - // @TODO -#else - int rfd_; - int wfd_; - - struct sigaction old_act_; - bool restore_; - - static int dup_rfd_; - static void CloseDupRfd(int signum); - - bool CheckFd(int fd); - bool SetAlarmHandler(); -#endif - - void Return(); -}; - +// TokenPool implementation for GNU make jobserver - common bits // every instance owns an implicit token -> available_ == 1 -GNUmakeTokenPool::GNUmakeTokenPool() : available_(1), used_(0), - rfd_(-1), wfd_(-1), restore_(false) { +GNUmakeTokenPool::GNUmakeTokenPool() : available_(1), used_(0) { } GNUmakeTokenPool::~GNUmakeTokenPool() { - Clear(); - if (restore_) - sigaction(SIGALRM, &old_act_, NULL); -} - -bool GNUmakeTokenPool::CheckFd(int fd) { - if (fd < 0) - return false; - int ret = fcntl(fd, F_GETFD); - if (ret < 0) - return false; - return true; -} - -int GNUmakeTokenPool::dup_rfd_ = -1; - -void GNUmakeTokenPool::CloseDupRfd(int signum) { - close(dup_rfd_); - dup_rfd_ = -1; -} - -bool GNUmakeTokenPool::SetAlarmHandler() { - struct sigaction act; - memset(&act, 0, sizeof(act)); - act.sa_handler = CloseDupRfd; - if (sigaction(SIGALRM, &act, &old_act_) < 0) { - perror("sigaction:"); - return(false); - } else { - restore_ = true; - return(true); - } } bool GNUmakeTokenPool::Setup(bool ignore, bool verbose, double& max_load_average) { - const char *value = getenv("MAKEFLAGS"); + const char *value = GetEnv("MAKEFLAGS"); if (value) { // GNU make <= 4.1 const char *jobserver = strstr(value, "--jobserver-fds="); @@ -119,20 +44,13 @@ bool GNUmakeTokenPool::Setup(bool ignore, if (ignore) { printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); } else { - int rfd = -1; - int wfd = -1; - if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && - CheckFd(rfd) && - CheckFd(wfd) && - SetAlarmHandler()) { + if (ParseAuth(jobserver)) { const char *l_arg = strstr(value, " -l"); int load_limit = -1; if (verbose) { printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); } - rfd_ = rfd; - wfd_ = wfd; // translate GNU make -lN to ninja -lN if (l_arg && @@ -154,83 +72,14 @@ bool GNUmakeTokenPool::Acquire() { if (available_ > 0) return true; - // Please read - // - // http://make.mad-scientist.net/papers/jobserver-implementation/ - // - // for the reasoning behind the following code. - // - // Try to read one character from the pipe. Returns true on success. - // - // First check if read() would succeed without blocking. -#ifdef USE_PPOLL - pollfd pollfds[] = {{rfd_, POLLIN, 0}}; - int ret = poll(pollfds, 1, 0); -#else - fd_set set; - struct timeval timeout = { 0, 0 }; - FD_ZERO(&set); - FD_SET(rfd_, &set); - int ret = select(rfd_ + 1, &set, NULL, NULL, &timeout); -#endif - if (ret > 0) { - // Handle potential race condition: - // - the above check succeeded, i.e. read() should not block - // - the character disappears before we call read() - // - // Create a duplicate of rfd_. The duplicate file descriptor dup_rfd_ - // can safely be closed by signal handlers without affecting rfd_. - dup_rfd_ = dup(rfd_); - - if (dup_rfd_ != -1) { - struct sigaction act, old_act; - int ret = 0; - - // Temporarily replace SIGCHLD handler with our own - memset(&act, 0, sizeof(act)); - act.sa_handler = CloseDupRfd; - if (sigaction(SIGCHLD, &act, &old_act) == 0) { - struct itimerval timeout; - - // install a 100ms timeout that generates SIGALARM on expiration - memset(&timeout, 0, sizeof(timeout)); - timeout.it_value.tv_usec = 100 * 1000; // [ms] -> [usec] - if (setitimer(ITIMER_REAL, &timeout, NULL) == 0) { - char buf; - - // Now try to read() from dup_rfd_. Return values from read(): - // - // 1. token read -> 1 - // 2. pipe closed -> 0 - // 3. alarm expires -> -1 (EINTR) - // 4. child exits -> -1 (EINTR) - // 5. alarm expired before entering read() -> -1 (EBADF) - // 6. child exited before entering read() -> -1 (EBADF) - // 7. child exited before handler is installed -> go to 1 - 3 - ret = read(dup_rfd_, &buf, 1); - - // disarm timer - memset(&timeout, 0, sizeof(timeout)); - setitimer(ITIMER_REAL, &timeout, NULL); - } - - sigaction(SIGCHLD, &old_act, NULL); - } - - CloseDupRfd(0); - - // Case 1 from above list - if (ret > 0) { - available_++; - return true; - } - } + if (AcquireToken()) { + // token acquired + available_++; + return true; + } else { + // no token available + return false; } - - // read() would block, i.e. no token available, - // cases 2-6 from above list or - // select() / poll() / dup() / sigaction() / setitimer() failed - return false; } void GNUmakeTokenPool::Reserve() { @@ -239,15 +88,8 @@ void GNUmakeTokenPool::Reserve() { } void GNUmakeTokenPool::Return() { - const char buf = '+'; - while (1) { - int ret = write(wfd_, &buf, 1); - if (ret > 0) - available_--; - if ((ret != -1) || (errno != EINTR)) - return; - // write got interrupted - retry - } + if (ReturnToken()) + available_--; } void GNUmakeTokenPool::Release() { @@ -263,18 +105,3 @@ void GNUmakeTokenPool::Clear() { while (available_ > 1) Return(); } - -int GNUmakeTokenPool::GetMonitorFd() { - return(rfd_); -} - -struct TokenPool *TokenPool::Get(bool ignore, - bool verbose, - double& max_load_average) { - GNUmakeTokenPool *tokenpool = new GNUmakeTokenPool; - if (tokenpool->Setup(ignore, verbose, max_load_average)) - return tokenpool; - else - delete tokenpool; - return NULL; -} diff --git a/src/tokenpool-gnu-make.h b/src/tokenpool-gnu-make.h new file mode 100644 index 0000000000..d3852088e2 --- /dev/null +++ b/src/tokenpool-gnu-make.h @@ -0,0 +1,40 @@ +// Copyright 2016-2018 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tokenpool.h" + +// interface to GNU make token pool +struct GNUmakeTokenPool : public TokenPool { + GNUmakeTokenPool(); + virtual ~GNUmakeTokenPool(); + + // token pool implementation + virtual bool Acquire(); + virtual void Reserve(); + virtual void Release(); + virtual void Clear(); + virtual bool Setup(bool ignore, bool verbose, double& max_load_average); + + // platform specific implementation + virtual const char *GetEnv(const char *name) = 0; + virtual bool ParseAuth(const char *jobserver) = 0; + virtual bool AcquireToken() = 0; + virtual bool ReturnToken() = 0; + + private: + int available_; + int used_; + + void Return(); +}; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc index 4c592875b4..613d16882d 100644 --- a/src/tokenpool-none.cc +++ b/src/tokenpool-none.cc @@ -17,8 +17,6 @@ #include // No-op TokenPool implementation -struct TokenPool *TokenPool::Get(bool ignore, - bool verbose, - double& max_load_average) { +struct TokenPool *TokenPool::Get() { return NULL; } diff --git a/src/tokenpool.h b/src/tokenpool.h index 4bf477f20c..1be8e1d5ce 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -1,4 +1,4 @@ -// Copyright 2016-2017 Google Inc. All Rights Reserved. +// Copyright 2016-2018 Google Inc. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -12,6 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. +#ifdef _WIN32 +#include +#endif + // interface to token pool struct TokenPool { virtual ~TokenPool() {} @@ -21,14 +25,18 @@ struct TokenPool { virtual void Release() = 0; virtual void Clear() = 0; + // returns false if token pool setup failed + virtual bool Setup(bool ignore, bool verbose, double& max_load_average) = 0; + #ifdef _WIN32 - // @TODO + virtual void WaitForTokenAvailability(HANDLE ioport) = 0; + // returns true if a token has become available + // key is result from GetQueuedCompletionStatus() + virtual bool TokenIsAvailable(ULONG_PTR key) = 0; #else virtual int GetMonitorFd() = 0; #endif // returns NULL if token pool is not available - static struct TokenPool *Get(bool ignore, - bool verbose, - double& max_load_average); + static struct TokenPool *Get(); }; diff --git a/src/tokenpool_test.cc b/src/tokenpool_test.cc index 6c89064ca4..8d4fd7d33a 100644 --- a/src/tokenpool_test.cc +++ b/src/tokenpool_test.cc @@ -16,13 +16,25 @@ #include "test.h" -#ifndef _WIN32 +#ifdef _WIN32 +#include +#else +#include +#endif + #include #include -#include +#ifdef _WIN32 +// should contain all valid characters +#define SEMAPHORE_NAME "abcdefghijklmnopqrstwxyz01234567890_" +#define AUTH_FORMAT(tmpl) "foo " tmpl "=%s bar" +#define ENVIRONMENT_CLEAR() SetEnvironmentVariable("MAKEFLAGS", NULL) +#define ENVIRONMENT_INIT(v) SetEnvironmentVariable("MAKEFLAGS", v) +#else +#define AUTH_FORMAT(tmpl) "foo " tmpl "=%d,%d bar" #define ENVIRONMENT_CLEAR() unsetenv("MAKEFLAGS") -#define ENVIRONMENT_INIT(v) setenv("MAKEFLAGS", v, true); +#define ENVIRONMENT_INIT(v) setenv("MAKEFLAGS", v, true) #endif namespace { @@ -32,43 +44,60 @@ const double kLoadAverageDefault = -1.23456789; struct TokenPoolTest : public testing::Test { double load_avg_; TokenPool *tokens_; -#ifndef _WIN32 char buf_[1024]; +#ifdef _WIN32 + const char *semaphore_name_; + HANDLE semaphore_; +#else int fds_[2]; #endif virtual void SetUp() { load_avg_ = kLoadAverageDefault; tokens_ = NULL; -#ifndef _WIN32 ENVIRONMENT_CLEAR(); +#ifdef _WIN32 + semaphore_name_ = SEMAPHORE_NAME; + if ((semaphore_ = CreateSemaphore(0, 0, 2, SEMAPHORE_NAME)) == NULL) +#else if (pipe(fds_) < 0) - ASSERT_TRUE(false); #endif + ASSERT_TRUE(false); } - void CreatePool(const char *format, bool ignore_jobserver) { -#ifndef _WIN32 + void CreatePool(const char *format, bool ignore_jobserver = false) { if (format) { - sprintf(buf_, format, fds_[0], fds_[1]); + sprintf(buf_, format, +#ifdef _WIN32 + semaphore_name_ +#else + fds_[0], fds_[1] +#endif + ); ENVIRONMENT_INIT(buf_); } -#endif - tokens_ = TokenPool::Get(ignore_jobserver, false, load_avg_); + if ((tokens_ = TokenPool::Get()) != NULL) { + if (!tokens_->Setup(ignore_jobserver, false, load_avg_)) { + delete tokens_; + tokens_ = NULL; + } + } } void CreateDefaultPool() { - CreatePool("foo --jobserver-auth=%d,%d bar", false); + CreatePool(AUTH_FORMAT("--jobserver-auth")); } virtual void TearDown() { if (tokens_) delete tokens_; -#ifndef _WIN32 +#ifdef _WIN32 + CloseHandle(semaphore_); +#else close(fds_[0]); close(fds_[1]); - ENVIRONMENT_CLEAR(); #endif + ENVIRONMENT_CLEAR(); } }; @@ -82,10 +111,9 @@ TEST_F(TokenPoolTest, NoTokenPool) { EXPECT_EQ(kLoadAverageDefault, load_avg_); } -#ifndef _WIN32 TEST_F(TokenPoolTest, SuccessfulOldSetup) { // GNUmake <= 4.1 - CreatePool("foo --jobserver-fds=%d,%d bar", false); + CreatePool(AUTH_FORMAT("--jobserver-fds")); EXPECT_NE(NULL, tokens_); EXPECT_EQ(kLoadAverageDefault, load_avg_); @@ -100,19 +128,37 @@ TEST_F(TokenPoolTest, SuccessfulNewSetup) { } TEST_F(TokenPoolTest, IgnoreWithJN) { - CreatePool("foo --jobserver-auth=%d,%d bar", true); + CreatePool(AUTH_FORMAT("--jobserver-auth"), true); EXPECT_EQ(NULL, tokens_); EXPECT_EQ(kLoadAverageDefault, load_avg_); } TEST_F(TokenPoolTest, HonorLN) { - CreatePool("foo -l9 --jobserver-auth=%d,%d bar", false); + CreatePool(AUTH_FORMAT("-l9 --jobserver-auth")); EXPECT_NE(NULL, tokens_); EXPECT_EQ(9.0, load_avg_); } +#ifdef _WIN32 +TEST_F(TokenPoolTest, SemaphoreNotFound) { + semaphore_name_ = SEMAPHORE_NAME "_foobar"; + CreateDefaultPool(); + + EXPECT_EQ(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); +} + +TEST_F(TokenPoolTest, TokenIsAvailable) { + CreateDefaultPool(); + + ASSERT_NE(NULL, tokens_); + EXPECT_EQ(kLoadAverageDefault, load_avg_); + + EXPECT_TRUE(tokens_->TokenIsAvailable((ULONG_PTR)tokens_)); +} +#else TEST_F(TokenPoolTest, MonitorFD) { CreateDefaultPool(); @@ -121,6 +167,7 @@ TEST_F(TokenPoolTest, MonitorFD) { EXPECT_EQ(fds_[0], tokens_->GetMonitorFd()); } +#endif TEST_F(TokenPoolTest, ImplicitToken) { CreateDefaultPool(); @@ -147,7 +194,13 @@ TEST_F(TokenPoolTest, TwoTokens) { EXPECT_FALSE(tokens_->Acquire()); // jobserver offers 2nd token +#ifdef _WIN32 + LONG previous; + ASSERT_TRUE(ReleaseSemaphore(semaphore_, 1, &previous)); + ASSERT_EQ(0, previous); +#else ASSERT_EQ(1u, write(fds_[1], "T", 1)); +#endif EXPECT_TRUE(tokens_->Acquire()); tokens_->Reserve(); EXPECT_FALSE(tokens_->Acquire()); @@ -160,8 +213,14 @@ TEST_F(TokenPoolTest, TwoTokens) { tokens_->Release(); EXPECT_TRUE(tokens_->Acquire()); - // there must be one token in the pipe + // there must be one token available +#ifdef _WIN32 + EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(semaphore_, 0)); + EXPECT_TRUE(ReleaseSemaphore(semaphore_, 1, &previous)); + EXPECT_EQ(0, previous); +#else EXPECT_EQ(1u, read(fds_[0], buf_, sizeof(buf_))); +#endif // implicit token EXPECT_TRUE(tokens_->Acquire()); @@ -179,7 +238,13 @@ TEST_F(TokenPoolTest, Clear) { EXPECT_FALSE(tokens_->Acquire()); // jobserver offers 2nd & 3rd token +#ifdef _WIN32 + LONG previous; + ASSERT_TRUE(ReleaseSemaphore(semaphore_, 2, &previous)); + ASSERT_EQ(0, previous); +#else ASSERT_EQ(2u, write(fds_[1], "TT", 2)); +#endif EXPECT_TRUE(tokens_->Acquire()); tokens_->Reserve(); EXPECT_TRUE(tokens_->Acquire()); @@ -189,10 +254,16 @@ TEST_F(TokenPoolTest, Clear) { tokens_->Clear(); EXPECT_TRUE(tokens_->Acquire()); - // there must be two tokens in the pipe + // there must be two tokens available +#ifdef _WIN32 + EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(semaphore_, 0)); + EXPECT_EQ(WAIT_OBJECT_0, WaitForSingleObject(semaphore_, 0)); + EXPECT_TRUE(ReleaseSemaphore(semaphore_, 2, &previous)); + EXPECT_EQ(0, previous); +#else EXPECT_EQ(2u, read(fds_[0], buf_, sizeof(buf_))); +#endif // implicit token EXPECT_TRUE(tokens_->Acquire()); } -#endif From 2b9c81c0ec1226d8795e7725529f13be41eaa385 Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Fri, 14 Dec 2018 13:27:11 +0200 Subject: [PATCH 11/11] Prepare PR for merging - part II - remove unnecessary "struct" from TokenPool - add PAPCFUNC cast to QueryUserAPC() - remove hard-coded MAKEFLAGS string from win32 - remove useless build test CompleteNoWork - rename TokenPoolTest to TestTokenPool - add tokenpool modules to CMake build - remove unused no-op TokenPool implementation - fix errors flagged by codespell & clang-tidy - address review comments from https://github.com/ninja-build/ninja/pull/1140#pullrequestreview-195195803 https://github.com/ninja-build/ninja/pull/1140#pullrequestreview-185089255 https://github.com/ninja-build/ninja/pull/1140#issuecomment-473898963 https://github.com/ninja-build/ninja/pull/1140#issuecomment-596624610 --- CMakeLists.txt | 8 ++++- src/build.cc | 2 +- src/build_test.cc | 12 +------ src/subprocess-posix.cc | 4 +-- src/subprocess-win32.cc | 2 +- src/subprocess.h | 2 +- src/subprocess_test.cc | 26 +++++++------- src/tokenpool-gnu-make-posix.cc | 21 +++++------ src/tokenpool-gnu-make-win32.cc | 36 ++++++++++--------- src/tokenpool-gnu-make.cc | 63 +++++++++++++++++---------------- src/tokenpool-gnu-make.h | 6 ++-- src/tokenpool-none.cc | 22 ------------ src/tokenpool.h | 2 +- src/tokenpool_test.cc | 8 ++--- 14 files changed, 94 insertions(+), 120 deletions(-) delete mode 100644 src/tokenpool-none.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 57ae548f5b..e2876fe413 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -112,6 +112,7 @@ add_library(libninja OBJECT src/state.cc src/status.cc src/string_piece_util.cc + src/tokenpool-gnu-make.cc src/util.cc src/version.cc ) @@ -123,9 +124,13 @@ if(WIN32) src/msvc_helper_main-win32.cc src/getopt.c src/minidump-win32.cc + src/tokenpool-gnu-make-win32.cc ) else() - target_sources(libninja PRIVATE src/subprocess-posix.cc) + target_sources(libninja PRIVATE + src/subprocess-posix.cc + src/tokenpool-gnu-make-posix.cc + ) if(CMAKE_SYSTEM_NAME STREQUAL "OS400" OR CMAKE_SYSTEM_NAME STREQUAL "AIX") target_sources(libninja PRIVATE src/getopt.c) endif() @@ -204,6 +209,7 @@ if(BUILD_TESTING) src/string_piece_util_test.cc src/subprocess_test.cc src/test.cc + src/tokenpool_test.cc src/util_test.cc ) if(WIN32) diff --git a/src/build.cc b/src/build.cc index 20c3bdc2a0..854df08c2a 100644 --- a/src/build.cc +++ b/src/build.cc @@ -467,7 +467,7 @@ struct RealCommandRunner : public CommandRunner { // copy of config_.max_load_average; can be modified by TokenPool setup double max_load_average_; SubprocessSet subprocs_; - TokenPool *tokens_; + TokenPool* tokens_; map subproc_to_edge_; }; diff --git a/src/build_test.cc b/src/build_test.cc index dd41dfbe1d..8901c9518f 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -4098,7 +4098,7 @@ struct BuildTokenTest : public BuildTest { void ExpectWaitForCommand(int count, ...); private: - void EnqueueBooleans(vector& booleans, int count, va_list ao); + void EnqueueBooleans(vector& booleans, int count, va_list ap); }; void BuildTokenTest::SetUp() { @@ -4144,16 +4144,6 @@ void BuildTokenTest::EnqueueBooleans(vector& booleans, int count, va_list } } -TEST_F(BuildTokenTest, CompleteNoWork) { - // plan should not execute anything - string err; - - EXPECT_TRUE(builder_.Build(&err)); - EXPECT_EQ("", err); - - EXPECT_EQ(0u, token_command_runner_.commands_ran_.size()); -} - TEST_F(BuildTokenTest, DoNotAquireToken) { // plan should execute one command string err; diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index 74451b0be2..31839276c4 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -250,7 +250,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { } #ifdef USE_PPOLL -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { vector fds; nfds_t nfds = 0; @@ -315,7 +315,7 @@ bool SubprocessSet::DoWork(struct TokenPool* tokens) { } #else // !defined(USE_PPOLL) -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { fd_set set; int nfds = 0; FD_ZERO(&set); diff --git a/src/subprocess-win32.cc b/src/subprocess-win32.cc index ce3e2c20a4..2926e9a221 100644 --- a/src/subprocess-win32.cc +++ b/src/subprocess-win32.cc @@ -252,7 +252,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { return subprocess; } -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { DWORD bytes_read; Subprocess* subproc; OVERLAPPED* overlapped; diff --git a/src/subprocess.h b/src/subprocess.h index 9ea67ea477..1ec78171e8 100644 --- a/src/subprocess.h +++ b/src/subprocess.h @@ -86,7 +86,7 @@ struct SubprocessSet { ~SubprocessSet(); Subprocess* Add(const std::string& command, bool use_console = false); - bool DoWork(struct TokenPool* tokens); + bool DoWork(TokenPool* tokens); Subprocess* NextFinished(); void Clear(); diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index f625963462..7b146f31be 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -35,7 +35,7 @@ const char* kSimpleCommand = "cmd /c dir \\"; const char* kSimpleCommand = "ls /"; #endif -struct TokenPoolTest : public TokenPool { +struct TestTokenPool : public TokenPool { bool Acquire() { return false; } void Reserve() {} void Release() {} @@ -58,7 +58,7 @@ struct TokenPoolTest : public TokenPool { struct SubprocessTest : public testing::Test { SubprocessSet subprocs_; - TokenPoolTest tokens_; + TestTokenPool tokens_; }; } // anonymous namespace @@ -73,7 +73,7 @@ TEST_F(SubprocessTest, BadCommandStderr) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -89,7 +89,7 @@ TEST_F(SubprocessTest, NoSuchCommand) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -109,7 +109,7 @@ TEST_F(SubprocessTest, InterruptChild) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -135,7 +135,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigTerm) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -161,7 +161,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigHup) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -190,7 +190,7 @@ TEST_F(SubprocessTest, Console) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitSuccess, subproc->Finish()); } @@ -206,7 +206,7 @@ TEST_F(SubprocessTest, SetWithSingle) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_NE("", subproc->GetOutput()); @@ -243,7 +243,7 @@ TEST_F(SubprocessTest, SetWithMulti) { ASSERT_GT(subprocs_.running_.size(), 0u); subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(0u, subprocs_.running_.size()); ASSERT_EQ(3u, subprocs_.finished_.size()); @@ -278,7 +278,7 @@ TEST_F(SubprocessTest, SetWithLots) { subprocs_.ResetTokenAvailable(); while (!subprocs_.running_.empty()) subprocs_.DoWork(NULL); - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); for (size_t i = 0; i < procs.size(); ++i) { ASSERT_EQ(ExitSuccess, procs[i]->Finish()); ASSERT_NE("", procs[i]->GetOutput()); @@ -298,7 +298,7 @@ TEST_F(SubprocessTest, ReadStdin) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_EQ(1u, subprocs_.finished_.size()); } @@ -322,7 +322,7 @@ TEST_F(SubprocessTest, TokenAvailable) { subprocs_.DoWork(&tokens_); #ifdef _WIN32 tokens_._token_available = false; - // we need to loop here as we have no conrol where the token + // we need to loop here as we have no control where the token // I/O completion post ends up in the queue while (!subproc->Done() && !subprocs_.IsTokenAvailable()) { subprocs_.DoWork(&tokens_); diff --git a/src/tokenpool-gnu-make-posix.cc b/src/tokenpool-gnu-make-posix.cc index 70d84bfff7..353bda226a 100644 --- a/src/tokenpool-gnu-make-posix.cc +++ b/src/tokenpool-gnu-make-posix.cc @@ -32,8 +32,8 @@ struct GNUmakeTokenPoolPosix : public GNUmakeTokenPool { virtual int GetMonitorFd(); - virtual const char *GetEnv(const char *name) { return getenv(name); }; - virtual bool ParseAuth(const char *jobserver); + virtual const char* GetEnv(const char* name) { return getenv(name); }; + virtual bool ParseAuth(const char* jobserver); virtual bool AcquireToken(); virtual bool ReturnToken(); @@ -64,9 +64,7 @@ bool GNUmakeTokenPoolPosix::CheckFd(int fd) { if (fd < 0) return false; int ret = fcntl(fd, F_GETFD); - if (ret < 0) - return false; - return true; + return ret >= 0; } int GNUmakeTokenPoolPosix::dup_rfd_ = -1; @@ -82,14 +80,13 @@ bool GNUmakeTokenPoolPosix::SetAlarmHandler() { act.sa_handler = CloseDupRfd; if (sigaction(SIGALRM, &act, &old_act_) < 0) { perror("sigaction:"); - return(false); - } else { - restore_ = true; - return(true); + return false; } + restore_ = true; + return true; } -bool GNUmakeTokenPoolPosix::ParseAuth(const char *jobserver) { +bool GNUmakeTokenPoolPosix::ParseAuth(const char* jobserver) { int rfd = -1; int wfd = -1; if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && @@ -195,9 +192,9 @@ bool GNUmakeTokenPoolPosix::ReturnToken() { } int GNUmakeTokenPoolPosix::GetMonitorFd() { - return(rfd_); + return rfd_; } -struct TokenPool *TokenPool::Get() { +TokenPool* TokenPool::Get() { return new GNUmakeTokenPoolPosix; } diff --git a/src/tokenpool-gnu-make-win32.cc b/src/tokenpool-gnu-make-win32.cc index 2719f2c1fc..b2bb52fadb 100644 --- a/src/tokenpool-gnu-make-win32.cc +++ b/src/tokenpool-gnu-make-win32.cc @@ -14,7 +14,8 @@ #include "tokenpool-gnu-make.h" -// always include first to make sure other headers do the correct thing... +// Always include this first. +// Otherwise the other system headers don't work correctly under Win32 #include #include @@ -32,8 +33,8 @@ struct GNUmakeTokenPoolWin32 : public GNUmakeTokenPool { virtual void WaitForTokenAvailability(HANDLE ioport); virtual bool TokenIsAvailable(ULONG_PTR key); - virtual const char *GetEnv(const char *name); - virtual bool ParseAuth(const char *jobserver); + virtual const char* GetEnv(const char* name); + virtual bool ParseAuth(const char* jobserver); virtual bool AcquireToken(); virtual bool ReturnToken(); @@ -98,19 +99,19 @@ GNUmakeTokenPoolWin32::~GNUmakeTokenPoolWin32() { } } -const char *GNUmakeTokenPoolWin32::GetEnv(const char *name) { +const char* GNUmakeTokenPoolWin32::GetEnv(const char* name) { // getenv() does not work correctly together with tokenpool_tests.cc static char buffer[MAX_PATH + 1]; - if (GetEnvironmentVariable("MAKEFLAGS", buffer, sizeof(buffer)) == 0) + if (GetEnvironmentVariable(name, buffer, sizeof(buffer)) == 0) return NULL; - return(buffer); + return buffer; } -bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) { +bool GNUmakeTokenPoolWin32::ParseAuth(const char* jobserver) { // match "--jobserver-auth=gmake_semaphore_..." - const char *start = strchr(jobserver, '='); + const char* start = strchr(jobserver, '='); if (start) { - const char *end = start; + const char* end = start; unsigned int len; char c, *auth; @@ -119,14 +120,15 @@ bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) { break; len = end - start; // includes string terminator in count - if ((len > 1) && ((auth = (char *)malloc(len)) != NULL)) { + if ((len > 1) && ((auth = (char*)malloc(len)) != NULL)) { strncpy(auth, start + 1, len - 1); auth[len - 1] = '\0'; - if ((semaphore_jobserver_ = OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */ - FALSE, /* Child processes DON'T inherit */ - auth /* Semaphore name */ - )) != NULL) { + if ((semaphore_jobserver_ = + OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */ + FALSE, /* Child processes DON'T inherit */ + auth /* Semaphore name */ + )) != NULL) { free(auth); return true; } @@ -171,7 +173,7 @@ DWORD GNUmakeTokenPoolWin32::SemaphoreThread() { } DWORD WINAPI GNUmakeTokenPoolWin32::SemaphoreThreadWrapper(LPVOID param) { - GNUmakeTokenPoolWin32 *This = (GNUmakeTokenPoolWin32 *) param; + GNUmakeTokenPoolWin32* This = (GNUmakeTokenPoolWin32*) param; return This->SemaphoreThread(); } @@ -216,7 +218,7 @@ void GNUmakeTokenPoolWin32::WaitForTokenAvailability(HANDLE ioport) { bool GNUmakeTokenPoolWin32::TokenIsAvailable(ULONG_PTR key) { // alert child thread to break wait on token semaphore - QueueUserAPC(&NoopAPCFunc, child_, (ULONG_PTR)NULL); + QueueUserAPC((PAPCFUNC)&NoopAPCFunc, child_, (ULONG_PTR)NULL); // return true when GetQueuedCompletionStatus() returned our key return key == (ULONG_PTR) this; @@ -232,6 +234,6 @@ void GNUmakeTokenPoolWin32::WaitForObject(HANDLE object) { Win32Fatal("WaitForSingleObject"); } -struct TokenPool *TokenPool::Get() { +TokenPool* TokenPool::Get() { return new GNUmakeTokenPoolWin32; } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index 92ff611721..60e0552924 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -31,36 +31,37 @@ GNUmakeTokenPool::~GNUmakeTokenPool() { bool GNUmakeTokenPool::Setup(bool ignore, bool verbose, double& max_load_average) { - const char *value = GetEnv("MAKEFLAGS"); - if (value) { - // GNU make <= 4.1 - const char *jobserver = strstr(value, "--jobserver-fds="); + const char* value = GetEnv("MAKEFLAGS"); + if (!value) + return false; + + // GNU make <= 4.1 + const char* jobserver = strstr(value, "--jobserver-fds="); + if (!jobserver) // GNU make => 4.2 - if (!jobserver) - jobserver = strstr(value, "--jobserver-auth="); - if (jobserver) { - LinePrinter printer; - - if (ignore) { - printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); - } else { - if (ParseAuth(jobserver)) { - const char *l_arg = strstr(value, " -l"); - int load_limit = -1; - - if (verbose) { - printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); - } - - // translate GNU make -lN to ninja -lN - if (l_arg && - (sscanf(l_arg + 3, "%d ", &load_limit) == 1) && - (load_limit > 0)) { - max_load_average = load_limit; - } - - return true; + jobserver = strstr(value, "--jobserver-auth="); + if (jobserver) { + LinePrinter printer; + + if (ignore) { + printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); + } else { + if (ParseAuth(jobserver)) { + const char* l_arg = strstr(value, " -l"); + int load_limit = -1; + + if (verbose) { + printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); + } + + // translate GNU make -lN to ninja -lN + if (l_arg && + (sscanf(l_arg + 3, "%d ", &load_limit) == 1) && + (load_limit > 0)) { + max_load_average = load_limit; } + + return true; } } } @@ -76,10 +77,10 @@ bool GNUmakeTokenPool::Acquire() { // token acquired available_++; return true; - } else { - // no token available - return false; } + + // no token available + return false; } void GNUmakeTokenPool::Reserve() { diff --git a/src/tokenpool-gnu-make.h b/src/tokenpool-gnu-make.h index d3852088e2..c94cca5e2d 100644 --- a/src/tokenpool-gnu-make.h +++ b/src/tokenpool-gnu-make.h @@ -17,7 +17,7 @@ // interface to GNU make token pool struct GNUmakeTokenPool : public TokenPool { GNUmakeTokenPool(); - virtual ~GNUmakeTokenPool(); + ~GNUmakeTokenPool(); // token pool implementation virtual bool Acquire(); @@ -27,8 +27,8 @@ struct GNUmakeTokenPool : public TokenPool { virtual bool Setup(bool ignore, bool verbose, double& max_load_average); // platform specific implementation - virtual const char *GetEnv(const char *name) = 0; - virtual bool ParseAuth(const char *jobserver) = 0; + virtual const char* GetEnv(const char* name) = 0; + virtual bool ParseAuth(const char* jobserver) = 0; virtual bool AcquireToken() = 0; virtual bool ReturnToken() = 0; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc deleted file mode 100644 index 613d16882d..0000000000 --- a/src/tokenpool-none.cc +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2016-2018 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "tokenpool.h" - -#include - -// No-op TokenPool implementation -struct TokenPool *TokenPool::Get() { - return NULL; -} diff --git a/src/tokenpool.h b/src/tokenpool.h index 1be8e1d5ce..931c22754d 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -38,5 +38,5 @@ struct TokenPool { #endif // returns NULL if token pool is not available - static struct TokenPool *Get(); + static TokenPool* Get(); }; diff --git a/src/tokenpool_test.cc b/src/tokenpool_test.cc index 8d4fd7d33a..8d3061cb30 100644 --- a/src/tokenpool_test.cc +++ b/src/tokenpool_test.cc @@ -43,10 +43,10 @@ const double kLoadAverageDefault = -1.23456789; struct TokenPoolTest : public testing::Test { double load_avg_; - TokenPool *tokens_; + TokenPool* tokens_; char buf_[1024]; #ifdef _WIN32 - const char *semaphore_name_; + const char* semaphore_name_; HANDLE semaphore_; #else int fds_[2]; @@ -65,7 +65,7 @@ struct TokenPoolTest : public testing::Test { ASSERT_TRUE(false); } - void CreatePool(const char *format, bool ignore_jobserver = false) { + void CreatePool(const char* format, bool ignore_jobserver = false) { if (format) { sprintf(buf_, format, #ifdef _WIN32 @@ -209,7 +209,7 @@ TEST_F(TokenPoolTest, TwoTokens) { tokens_->Release(); EXPECT_TRUE(tokens_->Acquire()); - // release implict token - must return 2nd token back to jobserver + // release implicit token - must return 2nd token back to jobserver tokens_->Release(); EXPECT_TRUE(tokens_->Acquire());