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
clemenswasser authored and Clemens Wasser committed May 7, 2024
1 parent 4ef30b1 commit e922611
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 22 deletions.
3 changes: 2 additions & 1 deletion src/build.cc
View file
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
View file
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
View file
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"
" - 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
16 changes: 13 additions & 3 deletions src/subprocess-posix.cc
View file
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 @@ -123,6 +127,12 @@ bool Subprocess::Start(SubprocessSet* set, const string& command) {
if (err != 0)
Fatal("posix_spawn: %s", strerror(err));

if (priority != INT_MIN) {
err = setpriority(PRIO_PROCESS, pid_, priority);

This comment has been minimized.

Sign in to view

Copy link
@digit-google

digit-google May 7, 2024

Contributor

no, you should save the current priority level to restore it after the spawn() call.
Be aware that getpriority() can return a negative value even if no error occured, so you should clear errno specifically before that, e.g.:

errno = 0;
int current_priority = getpriority(PRIO_PROCESS, 0);
if (current_priority == -1 && errno != 0) {
  Fatal("getpriority: %s", strerror(errno));
}
// Temporarily change the niceness of the current process, to let the child
// process inherit it. This will be restored just after the spawn() to avoid race
// conditions. Note that there is no posix_spawnattr_xxx function to deal with
// this value, posix_spawnattr_set_schedparam() only handles priorities for
// real-time threads that are something completely different from process niceness.
err = setpriority(PRIO_PROCESS, 0, priority);
if (err != 0)
  Fatal("setpriority: %s", strerror(errno));

Then later,

if (priority != INT_MIN) {
  // Restore Ninja process niceness, since it was changed to adjust the child one.
  setpriority(PRIO_PROCESS, 0, current_priority);
}

This comment has been minimized.

Sign in to view

Copy link
@digit-google

digit-google May 7, 2024

Contributor

Oh, scratch that, ignoring ESERCH should be good enough, since the kernel will round-robin process IDs, even under heavy load there is likely no chance this is going to set the priority on the wrong process :-)

I suggest clearing errno in this case, just to be sure though.

Please address other commands about testing though. Thank you.
if (err != 0 && err != ESRCH)
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 +248,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
View file
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
9 changes: 7 additions & 2 deletions src/subprocess.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,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)
bool Start(struct SubprocessSet* set, const std::string& command, int priority);
void OnPipeReady();

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

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

This comment has been minimized.

Sign in to view

Copy link
@digit-google

digit-google May 7, 2024

Contributor

Please fix lines 152-153 or src/subprocess_test.cc to read as:

    Subprocess* subproc =
        subprocs_.Add("test -t 0 -a -t 1 -a -t 2", /*priority=*/INT_MIN, /*use_console=*/true);

This comment has been minimized.

Sign in to view

Copy link
@digit-google

digit-google May 7, 2024

Contributor

Also shouldn't the default value be INT_MIN instead?
bool DoWork();
Subprocess* NextFinished();
void Clear();
Expand Down

1 comment on commit e922611

@digit-google
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you check that this works correctly? Can you write a SubprocessTest unit-test where the spawned command prints its niceness value, then compare it with an expected value?

Please sign in to comment.