Optimize RunCommand by removing /bin/sh wrapper when possible
For every $(shell echo "test: $PATH") command, when SHELL is /bin/bash,
we essentially run: (each arg wrapped in [])
[/bin/sh] [-c] [/bin/bash -c "echo \"test: \$PATH\""]
This is redundant, since we can just use SHELL, and then we don't need
to do an extra level of shell escaping either. This change makes us run
this instead:
[/bin/bash] [-c] [echo "test: $PATH"]
If SHELL is more complicated than an absolute path to a binary, then
we'll fall back to /bin/sh.
Using the benchmark introduced in the last change, this reduces a
minimal RunCommand execution with a simple SHELL from 3.7ms to 1.3ms.
For a more complex benchmark (though less normalized), for an AOSP
Android build, this change shrinks the average time spent in $(shell)
functions from 4.5ms to 3ms.
Change-Id: I622116e33565e58bb123ee9e9bdd302616a6609c
diff --git a/exec.cc b/exec.cc
index 6065169..1569519 100644
--- a/exec.cc
+++ b/exec.cc
@@ -46,7 +46,8 @@
explicit Executor(Evaluator* ev)
: ce_(ev),
num_commands_(0) {
- shell_ = ev->GetShellAndFlag();
+ shell_ = ev->GetShell();
+ shellflag_ = ev->GetShellFlag();
}
double ExecNode(DepNode* n, DepNode* needed_by) {
@@ -106,7 +107,8 @@
}
if (!g_flags.is_dry_run) {
string out;
- int result = RunCommand(shell_, command->cmd.c_str(),
+ int result = RunCommand(shell_, shellflag_,
+ command->cmd.c_str(),
RedirectStderr::STDOUT,
&out);
printf("%s", out.c_str());
@@ -136,6 +138,7 @@
CommandEvaluator ce_;
unordered_map<Symbol, double> done_;
string shell_;
+ string shellflag_;
uint64_t num_commands_;
};
diff --git a/fileutil.cc b/fileutil.cc
index 0d3c2d6..4cf653b 100644
--- a/fileutil.cc
+++ b/fileutil.cc
@@ -60,15 +60,24 @@
return GetTimestampFromStat(st);
}
-int RunCommand(const string& shell, const string& cmd,
- RedirectStderr redirect_stderr,
+int RunCommand(const string& shell, const string& shellflag,
+ const string& cmd, RedirectStderr redirect_stderr,
string* s) {
- string cmd_escaped = cmd;
- EscapeShell(&cmd_escaped);
- string cmd_with_shell = shell + " \"" + cmd_escaped + "\"";
- const char* argv[] = {
- "/bin/sh", "-c", cmd_with_shell.c_str(), NULL
- };
+ const char* argv[] = { NULL, NULL, NULL, NULL };
+ string cmd_with_shell;
+ if (shell[0] != '/' || shell.find_first_of(" $") != string::npos) {
+ string cmd_escaped = cmd;
+ EscapeShell(&cmd_escaped);
+ cmd_with_shell = shell + " " + shellflag + " \"" + cmd_escaped + "\"";
+ argv[0] = "/bin/sh";
+ argv[1] = "-c";
+ argv[2] = cmd_with_shell.c_str();
+ } else {
+ // If the shell isn't complicated, we don't need to wrap in /bin/sh
+ argv[0] = shell.c_str();
+ argv[1] = shellflag.c_str();
+ argv[2] = cmd.c_str();
+ }
int pipefd[2];
if (pipe(pipefd) != 0)
diff --git a/fileutil.h b/fileutil.h
index 7cc3c8e..264d9e1 100644
--- a/fileutil.h
+++ b/fileutil.h
@@ -36,8 +36,8 @@
DEV_NULL,
};
-int RunCommand(const string& shell, const string& cmd,
- RedirectStderr redirect_stderr,
+int RunCommand(const string& shell, const string& shellflag,
+ const string& cmd, RedirectStderr redirect_stderr,
string* out);
void GetExecutablePath(string* path);
diff --git a/fileutil_bench.cc b/fileutil_bench.cc
index dc07d9a..4b1f033 100644
--- a/fileutil_bench.cc
+++ b/fileutil_bench.cc
@@ -17,21 +17,23 @@
#include <cstdio>
static void BM_RunCommand(benchmark::State& state) {
- std::string shell = "/bin/bash -c";
+ std::string shell = "/bin/bash";
+ std::string shellflag = "-c";
std::string cmd = "echo $((1+3))";
while (state.KeepRunning()) {
std::string result;
- RunCommand(shell, cmd, RedirectStderr::NONE, &result);
+ RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result);
}
}
BENCHMARK(BM_RunCommand);
static void BM_RunCommand_ComplexShell(benchmark::State& state) {
- std::string shell = "/bin/bash -c";
+ std::string shell = "/bin/bash ";
+ std::string shellflag = "-c";
std::string cmd = "echo $((1+3))";
while (state.KeepRunning()) {
std::string result;
- RunCommand(shell, cmd, RedirectStderr::NONE, &result);
+ RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result);
}
}
BENCHMARK(BM_RunCommand_ComplexShell);
diff --git a/func.cc b/func.cc
index 623e56e..7915aea 100644
--- a/func.cc
+++ b/func.cc
@@ -491,8 +491,8 @@
return false;
}
-static void ShellFuncImpl(const string& shell, const string& cmd,
- string* s, FindCommand** fc) {
+static void ShellFuncImpl(const string& shell, const string& shellflag,
+ const string& cmd, string* s, FindCommand** fc) {
LOG("ShellFunc: %s", cmd.c_str());
#ifdef TEST_FIND_EMULATOR
@@ -517,7 +517,7 @@
}
COLLECT_STATS_WITH_SLOW_REPORT("func shell time", cmd.c_str());
- RunCommand(shell, cmd, RedirectStderr::NONE, s);
+ RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, s);
FormatForCommandSubstitution(s);
#ifdef TEST_FIND_EMULATOR
@@ -564,14 +564,16 @@
return;
}
- const string&& shell = ev->GetShellAndFlag();
+ const string&& shell = ev->GetShell();
+ const string&& shellflag = ev->GetShellFlag();
string out;
FindCommand* fc = NULL;
- ShellFuncImpl(shell, cmd, &out, &fc);
+ ShellFuncImpl(shell, shellflag, cmd, &out, &fc);
if (ShouldStoreCommandResult(cmd)) {
CommandResult* cr = new CommandResult();
cr->shell = shell;
+ cr->shellflag = shellflag;
cr->cmd = cmd;
cr->find.reset(fc);
cr->result = out;
diff --git a/func.h b/func.h
index e78deb7..82e821c 100644
--- a/func.h
+++ b/func.h
@@ -43,6 +43,7 @@
struct CommandResult {
string shell;
+ string shellflag;
string cmd;
unique_ptr<FindCommand> find;
string result;
diff --git a/ninja.cc b/ninja.cc
index 90fe404..e2bc9cd 100644
--- a/ninja.cc
+++ b/ninja.cc
@@ -737,6 +737,7 @@
DumpInt(fp, crs.size());
for (CommandResult* cr : crs) {
DumpString(fp, cr->shell);
+ DumpString(fp, cr->shellflag);
DumpString(fp, cr->cmd);
DumpString(fp, cr->result);
if (!cr->find.get()) {
diff --git a/regen.cc b/regen.cc
index f5ed50a..a322691 100644
--- a/regen.cc
+++ b/regen.cc
@@ -53,6 +53,7 @@
struct ShellResult {
string shell;
+ string shellflag;
string cmd;
string result;
vector<string> missing_dirs;
@@ -232,6 +233,7 @@
ShellResult* sr = new ShellResult;
commands_.push_back(sr);
LOAD_STRING(fp, &sr->shell);
+ LOAD_STRING(fp, &sr->shellflag);
LOAD_STRING(fp, &sr->cmd);
LOAD_STRING(fp, &sr->result);
sr->has_condition = LOAD_INT(fp);
@@ -340,7 +342,7 @@
COLLECT_STATS_WITH_SLOW_REPORT("shell time (regen)", sr->cmd.c_str());
string result;
- RunCommand(sr->shell, sr->cmd, RedirectStderr::DEV_NULL, &result);
+ RunCommand(sr->shell, sr->shellflag, sr->cmd, RedirectStderr::DEV_NULL, &result);
FormatForCommandSubstitution(&result);
if (sr->result != result) {
if (g_flags.dump_kati_stamp) {