Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement GNU jobserver posix client support #2474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ if(WIN32)
# errors by telling windows.h to not define those two.
add_compile_definitions(NOMINMAX)
else()
target_sources(libninja PRIVATE src/subprocess-posix.cc)
target_sources(libninja PRIVATE
src/jobserver-posix.cc
src/subprocess-posix.cc
)
if(CMAKE_SYSTEM_NAME STREQUAL "OS400" OR CMAKE_SYSTEM_NAME STREQUAL "AIX")
target_sources(libninja PRIVATE src/getopt.c)
# Build getopt.c, which can be compiled as either C or C++, as C++
Expand Down
1 change: 1 addition & 0 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,7 @@ def has_re2c() -> bool:
objs += cxx('minidump-win32', variables=cxxvariables)
objs += cc('getopt')
else:
objs += cxx('jobserver-posix')
objs += cxx('subprocess-posix')
if platform.is_aix():
objs += cc('getopt')
Expand Down
44 changes: 38 additions & 6 deletions src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,16 @@ Edge* Plan::FindWork() {
return NULL;

Edge* work = ready_.top();

// Only initiate work if the jobserver client can acquire a token.
if (builder_ && builder_->jobserver_ &&
builder_->jobserver_->Enabled()) {
int job_tokens = builder_->jobserver_->Tokens();
work->job_token_ = builder_->jobserver_->Acquire();
if (job_tokens == builder_->jobserver_->Tokens())
return NULL;
}

ready_.pop();
return work;
}
Expand Down Expand Up @@ -199,6 +209,10 @@ bool Plan::EdgeFinished(Edge* edge, EdgeResult result, string* err) {
edge->pool()->EdgeFinished(*edge);
edge->pool()->RetrieveReadyEdges(&ready_);

// If jobserver is used, return the token for this job.
if (builder_ && builder_->jobserver_)
builder_->jobserver_->Release(&edge->job_token_);

// The rest of this function only applies to successful commands.
if (result != kEdgeSucceeded)
return true;
Expand Down Expand Up @@ -592,14 +606,18 @@ void Plan::Dump() const {
}

struct RealCommandRunner : public CommandRunner {
explicit RealCommandRunner(const BuildConfig& config) : config_(config) {}
explicit RealCommandRunner(const BuildConfig& config, Jobserver* jobserver) :
config_(config), jobserver_(jobserver) {}

size_t CanRunMore() const override;
bool StartCommand(Edge* edge) override;
bool WaitForCommand(Result* result) override;
vector<Edge*> GetActiveEdges() override;
void ClearJobTokens(const std::vector<Edge*>&) override;
void Abort() override;

const BuildConfig& config_;
Jobserver* jobserver_;
SubprocessSet subprocs_;
map<const Subprocess*, Edge*> subproc_to_edge_;
};
Expand All @@ -612,7 +630,13 @@ vector<Edge*> RealCommandRunner::GetActiveEdges() {
return edges;
}

void RealCommandRunner::ClearJobTokens(const std::vector<Edge*> &edges) {
for (Edge* edge : edges)
jobserver_->Release(&edge->job_token_);
}

void RealCommandRunner::Abort() {
ClearJobTokens(GetActiveEdges());
subprocs_.Clear();
}

Expand All @@ -628,6 +652,14 @@ size_t RealCommandRunner::CanRunMore() const {
capacity = load_capacity;
}

// When initialized, behave as if the implicit token is acquired already.
// Otherwise, this happens after a token is released but before it is replaced,
// so the base capacity is represented by job_tokens + 1 when positive.
// Add an extra loop on capacity for each job in order to get an extra token.
int job_tokens = jobserver_->Tokens();
if (job_tokens)
capacity = abs(job_tokens) - subproc_number + 2;

if (capacity < 0)
capacity = 0;

Expand Down Expand Up @@ -667,10 +699,10 @@ bool RealCommandRunner::WaitForCommand(Result* result) {
return true;
}

Builder::Builder(State* state, const BuildConfig& config, BuildLog* build_log,
DepsLog* deps_log, DiskInterface* disk_interface,
Status* status, int64_t start_time_millis)
: state_(state), config_(config), plan_(this), status_(status),
Builder::Builder(State* state, const BuildConfig& config, Jobserver* jobserver,
BuildLog* build_log, DepsLog* deps_log, DiskInterface* disk_interface,
Status* status, int64_t start_time_millis) : state_(state),
config_(config), jobserver_(jobserver), plan_(this), status_(status),
start_time_millis_(start_time_millis), disk_interface_(disk_interface),
explanations_(g_explaining ? new Explanations() : nullptr),
scan_(state, build_log, deps_log, disk_interface,
Expand Down Expand Up @@ -775,7 +807,7 @@ bool Builder::Build(string* err) {
if (config_.dry_run)
command_runner_.reset(new DryRunCommandRunner);
else
command_runner_.reset(new RealCommandRunner(config_));
command_runner_.reset(new RealCommandRunner(config_, jobserver_));
}

// We are about to start the build process.
Expand Down
9 changes: 6 additions & 3 deletions src/build.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "depfile_parser.h"
#include "exit_status.h"
#include "graph.h"
#include "jobserver.h"
#include "util.h" // int64_t

struct BuildLog;
Expand Down Expand Up @@ -161,6 +162,7 @@ struct CommandRunner {
virtual bool WaitForCommand(Result* result) = 0;

virtual std::vector<Edge*> GetActiveEdges() { return std::vector<Edge*>(); }
virtual void ClearJobTokens(const std::vector<Edge*>&) {}
virtual void Abort() {}
};

Expand All @@ -187,9 +189,9 @@ struct BuildConfig {

/// Builder wraps the build process: starting commands, updating status.
struct Builder {
Builder(State* state, const BuildConfig& config, BuildLog* build_log,
DepsLog* deps_log, DiskInterface* disk_interface, Status* status,
int64_t start_time_millis);
Builder(State* state, const BuildConfig& config, Jobserver* jobserver,
BuildLog* build_log, DepsLog* deps_log, DiskInterface* disk_interface,
Status* status, int64_t start_time_millis);
~Builder();

/// Clean up after interrupted commands by deleting output files.
Expand Down Expand Up @@ -224,6 +226,7 @@ struct Builder {

State* state_;
const BuildConfig& config_;
Jobserver* jobserver_;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document what it means for jobserver_ to be NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's never NULL when running the real ninja, only for the ninja_test binary

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Add as a comment:

Suggested change
Jobserver* jobserver_;
Jobserver* jobserver_; // Never NULL running the real ninja, only for the ninja_test binary

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the code easier to follow, I recommend to instead make JobServer a base class that does nothing (never enabled), then add a derived PosixJobServer class. This will allow you to add a Win32JobServer class later, and remove the need for all the #ifdef _WIN32 section sprinkled over the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@digit-google I understand class inheritance can work, but in this case I think I would have to fill the base class with no-op functions to get rid of the preprocessor stuff. Would that not be equally ugly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you would need no-op methods for the base class, but this leads to code that is easier to read and maintain over time. You can have test-specific implementations if you want as well. Since this class is not performance critical, you can do something like that:

  1. Add an std::unique_ptr<Jobserver> jobserver_ member to this class, and default-initialize it with a base/null instance that does nothing. This avoids modifying tests that don't care about the feature at all.

  2. Add a SetJobserver(std::unique_ptr<Jobserver> jobserver) method to replace the instance's jobserver with a new value. This can be called after constructing a Builder instance where it is needed only. This passes ownership to avoid lifecycle management issues.

  3. The rest of the code here can use jobserver_->xxxx() without worrying about the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the class into a basic pair of base and derived classes, now it is down to one #ifdef _WIN32. The rest of these tips revolving around unique_ptr is a little advanced for me, so I'm pushing what I have and we can take some more time to figure out that bit and how useful it would be.

Plan plan_;
std::unique_ptr<CommandRunner> command_runner_;
Status* status_;
Expand Down
Loading
Loading