Merge pull request #96 from danw/opt_runcommand

Optimize RunCommand by removing /bin/sh wrapper when possible

10 files changed, 92 insertions, 19 deletions

diff --git a/Android.bp b/Android.bp
index 91c2972..5dd8ef5 100644
--- a/Android.bp
+++ b/Android.bp
@@ -79,3 +79,16 @@ cc_test_host {
         },
     },
 }
+
+cc_benchmark_host {
+    name: "ckati_fileutil_bench",
+    srcs: [
+        "fileutil_bench.cc",
+    ],
+    whole_static_libs: ["libckati"],
+    target: {
+        linux: {
+            host_ldlibs: ["-lrt", "-lpthread"],
+        },
+    },
+}
diff --git a/Makefile.ckati b/Makefile.ckati
index df4c6a5..e4067bb 100644
--- a/Makefile.ckati
+++ b/Makefile.ckati
@@ -57,7 +57,8 @@ KATI_CXX_GENERATED_SRCS := \
 KATI_CXX_SRCS := $(addprefix $(KATI_SRC_PATH)/,$(KATI_CXX_SRCS))
 KATI_CXX_TEST_SRCS := \
   $(wildcard $(KATI_SRC_PATH)/*_test.cc) \
-  $(wildcard $(KATI_SRC_PATH)/*_bench.cc)
+  $(filter-out $(KATI_SRC_PATH)/fileutil_bench.cc,\
+    $(wildcard $(KATI_SRC_PATH)/*_bench.cc))
 
 KATI_CXX_OBJS := $(patsubst $(KATI_SRC_PATH)/%.cc,$(KATI_INTERMEDIATES_PATH)/%.o,\
 	$(KATI_CXX_SRCS))
diff --git a/exec.cc b/exec.cc
index 6065169..1569519 100644
--- a/exec.cc
+++ b/exec.cc
@@ -46,7 +46,8 @@ class Executor {
   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 @@ class Executor {
       }
       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 @@ class Executor {
   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 @@ double GetTimestamp(StringPiece filename) {
   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 @@ enum struct RedirectStderr {
   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
new file mode 100644
index 0000000..4b1f033
--- /dev/null
+++ b/fileutil_bench.cc
@@ -0,0 +1,41 @@
+// Copyright 2016 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 "fileutil.h"
+#include <benchmark/benchmark.h>
+#include <cstdio>
+
+static void BM_RunCommand(benchmark::State& state) {
+  std::string shell = "/bin/bash";
+  std::string shellflag = "-c";
+  std::string cmd = "echo $((1+3))";
+  while (state.KeepRunning()) {
+    std::string result;
+    RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result);
+  }
+}
+BENCHMARK(BM_RunCommand);
+
+static void BM_RunCommand_ComplexShell(benchmark::State& state) {
+  std::string shell = "/bin/bash ";
+  std::string shellflag = "-c";
+  std::string cmd = "echo $((1+3))";
+  while (state.KeepRunning()) {
+    std::string result;
+    RunCommand(shell, shellflag, cmd, RedirectStderr::NONE, &result);
+  }
+}
+BENCHMARK(BM_RunCommand_ComplexShell);
+
+BENCHMARK_MAIN();
diff --git a/func.cc b/func.cc
index 623e56e..7915aea 100644
--- a/func.cc
+++ b/func.cc
@@ -491,8 +491,8 @@ static bool HasNoIoInShellScript(const string& cmd) {
   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 @@ static void ShellFuncImpl(const string& shell, const string& cmd,
   }
 
   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 @@ void ShellFunc(const vector<Value*>& args, Evaluator* ev, string* s) {
     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 FindCommand;
 
 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 @@ class NinjaGenerator {
     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 @@ class StampChecker {
 
   struct ShellResult {
     string shell;
+    string shellflag;
     string cmd;
     string result;
     vector<string> missing_dirs;
@@ -232,6 +233,7 @@ class StampChecker {
       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 @@ class StampChecker {
 
     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) {