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 Make 4.4+ jobserver fifo / semaphore client support #2450

Open
wants to merge 2 commits 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
8 changes: 7 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ add_library(libninja OBJECT
src/eval_env.cc
src/graph.cc
src/graphviz.cc
src/jobserver.cc
src/json.cc
src/line_printer.cc
src/manifest_parser.cc
Expand All @@ -153,6 +154,7 @@ add_library(libninja OBJECT
if(WIN32)
target_sources(libninja PRIVATE
src/subprocess-win32.cc
src/jobserver-win32.cc
src/includes_normalize-win32.cc
src/msvc_helper-win32.cc
src/msvc_helper_main-win32.cc
Expand All @@ -169,7 +171,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/subprocess-posix.cc
src/jobserver-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 Expand Up @@ -267,6 +272,7 @@ if(BUILD_TESTING)
src/edit_distance_test.cc
src/explanations_test.cc
src/graph_test.cc
src/jobserver_test.cc
src/json_test.cc
src/lexer_test.cc
src/manifest_parser_test.cc
Expand Down
3 changes: 3 additions & 0 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ def has_re2c() -> bool:
'eval_env',
'graph',
'graphviz',
'jobserver',
'json',
'line_printer',
'manifest_parser',
Expand All @@ -556,6 +557,7 @@ def has_re2c() -> bool:
objs += cxx(name, variables=cxxvariables)
if platform.is_windows():
for name in ['subprocess-win32',
'jobserver-win32',
'includes_normalize-win32',
'msvc_helper-win32',
'msvc_helper_main-win32']:
Expand All @@ -565,6 +567,7 @@ def has_re2c() -> bool:
objs += cc('getopt')
else:
objs += cxx('subprocess-posix')
objs += cxx('jobserver-posix')
if platform.is_aix():
objs += cc('getopt')
if platform.is_msvc():
Expand Down
39 changes: 33 additions & 6 deletions src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ struct DryRunCommandRunner : public CommandRunner {
virtual ~DryRunCommandRunner() {}

// Overridden from CommandRunner:
virtual size_t CanRunMore() const;
size_t CanRunMore(bool jobserver_enabled) const override;
virtual bool StartCommand(Edge* edge);
virtual bool WaitForCommand(Result* result);

private:
queue<Edge*> finished_;
};

size_t DryRunCommandRunner::CanRunMore() const {
size_t DryRunCommandRunner::CanRunMore(bool jobserver_enabled) const {
return SIZE_MAX;
}

Expand Down Expand Up @@ -164,8 +164,18 @@ Edge* Plan::FindWork() {
if (ready_.empty())
return NULL;

// Don't initiate more work if the jobserver cannot acquire more tokens
if (jobserver_.Enabled() && !jobserver_.Acquire()) {
return nullptr;
}

Edge* work = ready_.top();
ready_.pop();

// Mark this edge as holding a token to release the token once the edge work
// is finished.
work->acquired_job_server_token_ = jobserver_.Enabled();

return work;
}

Expand Down Expand Up @@ -201,6 +211,12 @@ bool Plan::EdgeFinished(Edge* edge, EdgeResult result, string* err) {
edge->pool()->EdgeFinished(*edge);
edge->pool()->RetrieveReadyEdges(&ready_);

// Return the token acquired for this very edge to the jobserver, but only
// if it holds a token.
if (edge->acquired_job_server_token_) {
jobserver_.Release();
}

// The rest of this function only applies to successful commands.
if (result != kEdgeSucceeded)
return true;
Expand Down Expand Up @@ -579,10 +595,15 @@ void Plan::ScheduleInitialEdges() {
}

void Plan::PrepareQueue() {
jobserver_.Init();
ComputeCriticalPath();
ScheduleInitialEdges();
}

bool Plan::JobserverEnabled() const {
return jobserver_.Enabled();
}

void Plan::Dump() const {
printf("pending: %d\n", (int)want_.size());
for (map<Edge*, Want>::const_iterator e = want_.begin(); e != want_.end(); ++e) {
Expand All @@ -596,7 +617,7 @@ void Plan::Dump() const {
struct RealCommandRunner : public CommandRunner {
explicit RealCommandRunner(const BuildConfig& config) : config_(config) {}
virtual ~RealCommandRunner() {}
virtual size_t CanRunMore() const;
size_t CanRunMore(bool jobserver_enabled) const override;
virtual bool StartCommand(Edge* edge);
virtual bool WaitForCommand(Result* result);
virtual vector<Edge*> GetActiveEdges();
Expand All @@ -619,7 +640,13 @@ void RealCommandRunner::Abort() {
subprocs_.Clear();
}

size_t RealCommandRunner::CanRunMore() const {
size_t RealCommandRunner::CanRunMore(bool jobserver_enabled) const {
// Return "infinite" capacity if a jobserver is used to limit the number
// of parallel subprocesses instead.
if (jobserver_enabled) {
hundeboll marked this conversation as resolved.
Show resolved Hide resolved
return SIZE_MAX;
}

hundeboll marked this conversation as resolved.
Show resolved Hide resolved
size_t subproc_number =
subprocs_.running_.size() + subprocs_.finished_.size();

Expand Down Expand Up @@ -792,7 +819,7 @@ bool Builder::Build(string* err) {
while (plan_.more_to_do()) {
// See if we can start any more commands.
if (failures_allowed) {
size_t capacity = command_runner_->CanRunMore();
size_t capacity = command_runner_->CanRunMore(plan_.JobserverEnabled());
while (capacity > 0) {
hundeboll marked this conversation as resolved.
Show resolved Hide resolved
Edge* edge = plan_.FindWork();
if (!edge)
Expand Down Expand Up @@ -820,7 +847,7 @@ bool Builder::Build(string* err) {
--capacity;

// Re-evaluate capacity.
size_t current_capacity = command_runner_->CanRunMore();
size_t current_capacity = command_runner_->CanRunMore(plan_.JobserverEnabled());
if (current_capacity < capacity)
capacity = current_capacity;
}
Expand Down
9 changes: 8 additions & 1 deletion 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 @@ -52,6 +53,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; }

/// Jobserver status used to skip capacity based on load average
bool JobserverEnabled() const;

/// Dumps the current state of the plan.
void Dump() const;

Expand Down Expand Up @@ -139,14 +143,17 @@ struct Plan {

/// Total remaining number of wanted edges.
int wanted_edges_;

/// Jobserver client
Jobserver jobserver_;
};

/// CommandRunner is an interface that wraps running the build
/// subcommands. This allows tests to abstract out running commands.
/// RealCommandRunner is an implementation that actually runs commands.
struct CommandRunner {
virtual ~CommandRunner() {}
virtual size_t CanRunMore() const = 0;
virtual size_t CanRunMore(bool jobserver_enabled) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not changing this method's signature at all, to minimize changes.

Since the flag value is not going to change during a build, you can simply pass plan_.JobserverEnabled() to the RealCommandRunner constructor to record its value, and have RealCommandRunner::CanRunMore() return SIZE_MAX when the flag is enabled.

This should work identically, without having to change the DryCommandRunner and TestCommandRunner interfaces / implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

virtual bool StartCommand(Edge* edge) = 0;

/// The result of waiting for a command.
Expand Down
6 changes: 4 additions & 2 deletions src/build_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ struct FakeCommandRunner : public CommandRunner {
max_active_edges_(1), fs_(fs) {}

// CommandRunner impl
virtual size_t CanRunMore() const;
size_t CanRunMore(bool jobserver_enabled) const override;
virtual bool StartCommand(Edge* edge);
virtual bool WaitForCommand(Result* result);
virtual vector<Edge*> GetActiveEdges();
Expand Down Expand Up @@ -622,7 +622,9 @@ void BuildTest::RebuildTarget(const string& target, const char* manifest,
builder.command_runner_.release();
}

size_t FakeCommandRunner::CanRunMore() const {
size_t FakeCommandRunner::CanRunMore(bool jobserver_enabled) const {
assert(!jobserver_enabled);

if (active_edges_.size() < max_active_edges_)
return SIZE_MAX;
hundeboll marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
1 change: 1 addition & 0 deletions src/graph.h
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ struct Edge {
bool deps_loaded_ = false;
bool deps_missing_ = false;
bool generated_by_dep_loader_ = false;
bool acquired_job_server_token_ = false;
TimeStamp command_start_time_ = 0;

const Rule& rule() const { return *rule_; }
Expand Down
71 changes: 71 additions & 0 deletions src/jobserver-posix.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// Copyright 2024 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 "jobserver.h"

#include <fcntl.h>
#include <unistd.h>

#include <cassert>
#include <cstring>

#include "util.h"

void Jobserver::Init() {
assert(fd_ < 0);

if (!ParseJobserverAuth("fifo")) {
return;
}

const char* jobserver = jobserver_name_.c_str();

fd_ = open(jobserver, O_NONBLOCK | O_CLOEXEC | O_RDWR);
if (fd_ < 0) {
Fatal("failed to open jobserver: %s: %s", jobserver, strerror(errno));
}

Info("using jobserver: %s", jobserver);
}

Jobserver::~Jobserver() {
assert(token_count_ == 0);

if (fd_ >= 0) {
close(fd_);
}
}

bool Jobserver::Enabled() const {
return fd_ >= 0;
}

bool Jobserver::AcquireToken() {
char token;
int res = read(fd_, &token, 1);
hundeboll marked this conversation as resolved.
Show resolved Hide resolved
if (res < 0 && errno != EAGAIN && errno != EWOULDBLOCK) {
Fatal("failed to read jobserver token: %s", strerror(errno));
}

return res > 0;
}

void Jobserver::ReleaseToken() {
char token = '+';
Comment on lines +64 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

from the Make manual:

It’s important that when you release the job slot, you write back the same character you read. Don’t assume that all tokens are the same character; different characters may have different meanings to GNU make.

I have not looked at Make's code relevant to this yet, so I don't know the significance. It might be that the character being anything other than a + is a rare occurence... maybe someone else can clarify how important it is.

I will follow up with another comment with more information as I continue to play with the PR's changes and review Make's source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've noticed the same from the Make manual. It might make sense to store the acquired token in the (recently added) Edge::acquired_job_server_token_ member. I'll look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at GNU Make source history, I changed my mind. I don't think this is important. There is no sign that they are going to introduce new characters that have special meaning, and if they somehow do that, it would be pretty easy to add support for it later. Let's save the idea for the future...

int res = write(fd_, &token, 1);
hundeboll marked this conversation as resolved.
Show resolved Hide resolved
if (res != 1) {
Fatal("failed to write token: %s: %s", jobserver_name_.c_str(),
strerror(errno));
}
}
60 changes: 60 additions & 0 deletions src/jobserver-win32.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright 2024 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 "jobserver.h"

#include <windows.h>

#include <cassert>

#include "util.h"

void Jobserver::Init() {
assert(sem_ == INVALID_HANDLE_VALUE);

if (!ParseJobserverAuth("sem")) {
return;
}

const char* name = jobserver_name_.c_str();

sem_ = OpenSemaphore(SYNCHRONIZE|SEMAPHORE_MODIFY_STATE, false, name);
if (sem_ == nullptr) {
Win32Fatal("OpenSemaphore", name);
}

Info("using jobserver: %s", name);
hundeboll marked this conversation as resolved.
Show resolved Hide resolved
}

Jobserver::~Jobserver() {
assert(token_count_ == 0);

if (sem_ != INVALID_HANDLE_VALUE) {
CloseHandle(sem_);
}
}

bool Jobserver::Enabled() const {
return sem_ != INVALID_HANDLE_VALUE;
}

bool Jobserver::AcquireToken() {
return WaitForSingleObject(sem_, 0) == WAIT_OBJECT_0;
}

void Jobserver::ReleaseToken() {
if (!ReleaseSemaphore(sem_, 1, nullptr)) {
Win32Fatal("ReleaseSemaphore");
}
}
Loading
Loading