Skip to content

Commit

Permalink
Allow setting the priority of spawned subprocess
Browse files Browse the repository at this point in the history
  • Loading branch information
clemenswasser authored and Clemens Wasser committed May 7, 2024
1 parent 4ef30b1 commit 51c2682
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 23 deletions.
3 changes: 2 additions & 1 deletion src/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,8 @@ size_t RealCommandRunner::CanRunMore() const {

bool RealCommandRunner::StartCommand(Edge* edge) {
string command = edge->EvaluateCommand();
Subprocess* subproc = subprocs_.Add(command, edge->use_console());
Subprocess* subproc =
subprocs_.Add(command, config_.subprocess_priority, edge->use_console());
if (!subproc)
return false;
subproc_to_edge_.insert(make_pair(subproc, edge));
Expand Down
6 changes: 4 additions & 2 deletions src/build.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#ifndef NINJA_BUILD_H_
#define NINJA_BUILD_H_

#include <climits>
#include <cstdio>
#include <map>
#include <memory>
Expand Down Expand Up @@ -165,8 +166,8 @@ struct CommandRunner {

/// Options (e.g. verbosity, parallelism) passed to a build.
struct BuildConfig {
BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1),
failures_allowed(1), max_load_average(-0.0f) {}
BuildConfig() : verbosity(NORMAL), dry_run(false), parallelism(1), failures_allowed(1),
subprocess_priority(INT_MIN), max_load_average(-0.0f) {}

enum Verbosity {
QUIET, // No output -- used when testing.
Expand All @@ -178,6 +179,7 @@ struct BuildConfig {
bool dry_run;
int parallelism;
int failures_allowed;
int subprocess_priority;
/// The maximum load average we must not exceed. A negative value
/// means that we do not have any limit.
double max_load_average;
Expand Down
35 changes: 24 additions & 11 deletions src/ninja.cc
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,22 @@ void Usage(const BuildConfig& config) {
" -v, --verbose show all command lines while building\n"
" --quiet don't show progress status, just command output\n"
"\n"
" -C DIR change to DIR before doing anything else\n"
" -f FILE specify input build file [default=build.ninja]\n"
" -C DIR change to DIR before doing anything else\n"
" -f FILE specify input build file [default=build.ninja]\n"
"\n"
" -j N run N jobs in parallel (0 means infinity) [default=%d on this system]\n"
" -k N keep going until N jobs fail (0 means infinity) [default=1]\n"
" -l N do not start new jobs if the load average is greater than N\n"
" -n dry run (don't run commands but act like they succeeded)\n"
" -j N run N jobs in parallel (0 means infinity) [default=%d on this system]\n"
" -k N keep going until N jobs fail (0 means infinity) [default=1]\n"
" -l N do not start new jobs if the load average is greater than N\n"
" -n dry run (don't run commands but act like they succeeded)\n"
" -p PRIORITY set the priority/niceness of spawned processes\n"
" the range of possible values is platform dependent:\n"
" - POSIX: depdending on your scheduler\n"

This comment has been minimized.

Copy link
@digit-google

digit-google May 7, 2024

Contributor

nit: s/depdening/depending/. Also prefer something like POSIX: In -20..19 range, and -20..20 on some systems, see 'man setpriority'.

" - Windows: between -20 (highest priority) and 20 (lowest priority)\n"
"\n"
" -d MODE enable debugging (use '-d list' to list modes)\n"
" -t TOOL run a subtool (use '-t list' to list subtools)\n"
" terminates toplevel options; further flags are passed to the tool\n"
" -w FLAG adjust warnings (use '-w list' to list warnings)\n",
" -d MODE enable debugging (use '-d list' to list modes)\n"
" -t TOOL run a subtool (use '-t list' to list subtools)\n"
" terminates toplevel options; further flags are passed to the tool\n"
" -w FLAG adjust warnings (use '-w list' to list warnings)\n",
kNinjaVersion, config.parallelism);
}

Expand Down Expand Up @@ -1447,7 +1451,7 @@ int ReadFlags(int* argc, char*** argv,

int opt;
while (!options->tool &&
(opt = getopt_long(*argc, *argv, "d:f:j:k:l:nt:vw:C:h", kLongOptions,
(opt = getopt_long(*argc, *argv, "d:f:j:k:l:p:nt:vw:C:h", kLongOptions,
NULL)) != -1) {
switch (opt) {
case 'd':
Expand Down Expand Up @@ -1492,6 +1496,15 @@ int ReadFlags(int* argc, char*** argv,
case 'n':
config->dry_run = true;
break;
case 'p': {
char* end;
int value = strtol(optarg, &end, 10);
if (*end != 0)
Fatal("-p parameter not numeric: did you mean -p 0?");

config->subprocess_priority = value;
break;
}
case 't':
options->tool = ChooseTool(optarg);
if (!options->tool)
Expand Down
35 changes: 32 additions & 3 deletions src/subprocess-posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
#include <string.h>
#include <sys/wait.h>
#include <spawn.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <limits.h>

#if defined(USE_PPOLL)
#include <poll.h>
Expand All @@ -48,7 +51,8 @@ Subprocess::~Subprocess() {
Finish();
}

bool Subprocess::Start(SubprocessSet* set, const string& command) {
bool Subprocess::Start(SubprocessSet* set, const string& command,
int priority) {
int output_pipe[2];
if (pipe(output_pipe) < 0)
Fatal("pipe: %s", strerror(errno));
Expand Down Expand Up @@ -117,12 +121,37 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
if (err != 0)
Fatal("posix_spawnattr_setflags: %s", strerror(err));

int prev_priority;
if (priority != INT_MIN) {
// We set the priority of our own process so that the spawned child process
// directly inherits the wanted priority. Because of that, we have to save
// our current priority and reset it after the subprocess was spawned.

errno = 0;
prev_priority = getpriority(PRIO_PROCESS, 0);
if (errno != 0) {
Fatal("getpriority: %s", strerror(errno));
}

err = setpriority(PRIO_PROCESS, 0, priority);
if (err != 0) {
Fatal("setpriority: %s", strerror(errno));
}
}

const char* spawned_args[] = { "/bin/sh", "-c", command.c_str(), NULL };
err = posix_spawn(&pid_, "/bin/sh", &action, &attr,
const_cast<char**>(spawned_args), environ);
if (err != 0)
Fatal("posix_spawn: %s", strerror(err));

if (priority != INT_MIN) {
err = setpriority(PRIO_PROCESS, 0, prev_priority);
if (err != 0) {
Fatal("setpriority: %s", strerror(errno));
}
}

err = posix_spawnattr_destroy(&attr);
if (err != 0)
Fatal("posix_spawnattr_destroy: %s", strerror(err));
Expand Down Expand Up @@ -238,9 +267,9 @@ SubprocessSet::~SubprocessSet() {
Fatal("sigprocmask: %s", strerror(errno));
}

Subprocess *SubprocessSet::Add(const string& command, bool use_console) {
Subprocess *SubprocessSet::Add(const string& command, int priority, bool use_console) {
Subprocess *subprocess = new Subprocess(use_console);
if (!subprocess->Start(this, command)) {
if (!subprocess->Start(this, command, priority)) {
delete subprocess;
return 0;
}
Expand Down
23 changes: 20 additions & 3 deletions src/subprocess-win32.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,22 @@ HANDLE Subprocess::SetupPipe(HANDLE ioport) {
return output_write_child;
}

bool Subprocess::Start(SubprocessSet* set, const string& command) {
static DWORD POSIXPriorityToWin32(int unixPriority) {
if (unixPriority <= -20) {
return HIGH_PRIORITY_CLASS; // ..-20
} else if (unixPriority < 0) {
return ABOVE_NORMAL_PRIORITY_CLASS; // -19..-1
} else if (unixPriority == 0) {
return NORMAL_PRIORITY_CLASS; // 0
} else if (unixPriority < 20) {
return BELOW_NORMAL_PRIORITY_CLASS; // 1..19
} else {
return IDLE_PRIORITY_CLASS; // 20..
}
}

bool Subprocess::Start(SubprocessSet* set, const string& command,
int priority) {
HANDLE child_pipe = SetupPipe(set->ioport_);

SECURITY_ATTRIBUTES security_attributes;
Expand Down Expand Up @@ -106,6 +121,8 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {

// Ninja handles ctrl-c, except for subprocesses in console pools.
DWORD process_flags = use_console_ ? 0 : CREATE_NEW_PROCESS_GROUP;
if (priority != INT_MIN)
process_flags |= POSIXPriorityToWin32(priority);

// Do not prepend 'cmd /c' on Windows, this breaks command
// lines greater than 8,191 chars.
Expand Down Expand Up @@ -238,9 +255,9 @@ BOOL WINAPI SubprocessSet::NotifyInterrupted(DWORD dwCtrlType) {
return FALSE;
}

Subprocess *SubprocessSet::Add(const string& command, bool use_console) {
Subprocess* SubprocessSet::Add(const string& command, int priority, bool use_console) {
Subprocess *subprocess = new Subprocess(use_console);
if (!subprocess->Start(this, command)) {
if (!subprocess->Start(this, command, priority)) {
delete subprocess;
return 0;
}
Expand Down
10 changes: 8 additions & 2 deletions src/subprocess.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <string>
#include <vector>
#include <queue>
#include <limits.h>

#ifdef _WIN32
#include <windows.h>
Expand Down Expand Up @@ -52,7 +53,11 @@ struct Subprocess {

private:
Subprocess(bool use_console);
bool Start(struct SubprocessSet* set, const std::string& command);
// priority:
// - The range of possible values is platform dependent
// - POSIX: variable between scheduling policies between sched_get_priority_min() and sched_get_priority_max()
// - Windows: between -20 (highest priority) and 20 (lowest priority)

This comment has been minimized.

Copy link
@digit-google

digit-google May 7, 2024

Contributor

Sorry to bother you, but can you clarify here that INT_MIN is a special value used to mean "inherit" current priority / niceness from Ninja?

Also we now know that sched_get_priority_min() and sched_get_priority_max() have nothing to do with the niceness level, so can you change this for Posix with a reference to RLIMIT_NICE instead?

bool Start(struct SubprocessSet* set, const std::string& command, int priority);
void OnPipeReady();

std::string buf_;
Expand Down Expand Up @@ -83,7 +88,8 @@ struct SubprocessSet {
SubprocessSet();
~SubprocessSet();

Subprocess* Add(const std::string& command, bool use_console = false);
Subprocess* Add(const std::string& command, int priority = INT_MIN,
bool use_console = false);

This comment has been minimized.

Copy link
@digit-google

digit-google May 7, 2024

Contributor

nit: Can you add a comment telling that priority here is interpreted as in Subprocess::Start() ?

bool DoWork();
Subprocess* NextFinished();
void Clear();
Expand Down
13 changes: 12 additions & 1 deletion src/subprocess_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ TEST_F(SubprocessTest, Console) {
// Skip test if we don't have the console ourselves.
if (isatty(0) && isatty(1) && isatty(2)) {
Subprocess* subproc =
subprocs_.Add("test -t 0 -a -t 1 -a -t 2", /*use_console=*/true);
subprocs_.Add("test -t 0 -a -t 1 -a -t 2", /*priority=*/INT_MIN, /*use_console=*/true);
ASSERT_NE((Subprocess*)0, subproc);

while (!subproc->Done()) {
Expand All @@ -161,6 +161,17 @@ TEST_F(SubprocessTest, Console) {
}
}

TEST_F(SubprocessTest, Priority) {
Subprocess* subproc = subprocs_.Add("ps -o nice -p $$", /*priority=*/10);
ASSERT_NE((Subprocess*)0, subproc);

while (!subproc->Done()) {
subprocs_.DoWork();
}

EXPECT_EQ(" NI\n 10\n", subproc->GetOutput());

This comment has been minimized.

Copy link
@digit-google

digit-google May 7, 2024

Contributor

Very nice, thank you.

}

#endif

TEST_F(SubprocessTest, SetWithSingle) {
Expand Down

0 comments on commit 51c2682

Please sign in to comment.