Avoid string copies on lookup methods

Create a transparent comparator `StringPieceLess` that can allow
us to lookup up values in a `std::map<std::string, V>` using a
`StringPiece` without needing to create a temporary `std::string`.

Similarly, there are other methods on classes in `graph.h` that
take a `std::string` that can work just fine with a `StringPIece` and
avoid more temporaries.

Author: elliotgoodrich

src/eval_env.cc

@@ -18,32 +18,32 @@
 
 using namespace std;
 
-string BindingEnv::LookupVariable(const string& var) {
-  map<string, string>::iterator i = bindings_.find(var);
+std::string BindingEnv::LookupVariable(StringPiece var) {
+  const auto i = bindings_.find(var);
   if (i != bindings_.end())
     return i->second;
   if (parent_)
     return parent_->LookupVariable(var);
   return "";
 }
 
-void BindingEnv::AddBinding(const string& key, const string& val) {
-  bindings_[key] = val;
+void BindingEnv::AddBinding(const std::string& key, StringPiece val) {
+  bindings_[key].assign(val.begin(), val.end());
 }
 
 void BindingEnv::AddRule(std::unique_ptr<const Rule> rule) {
   assert(LookupRuleCurrentScope(rule->name()) == NULL);
   rules_[rule->name()] = std::move(rule);
 }
 
-const Rule* BindingEnv::LookupRuleCurrentScope(const string& rule_name) {
+const Rule* BindingEnv::LookupRuleCurrentScope(StringPiece rule_name) {
   auto i = rules_.find(rule_name);
   if (i == rules_.end())
     return NULL;
   return i->second.get();
 }
 
-const Rule* BindingEnv::LookupRule(const string& rule_name) {
+const Rule* BindingEnv::LookupRule(StringPiece rule_name) {
   auto i = rules_.find(rule_name);
   if (i != rules_.end())
     return i->second.get();
@@ -56,7 +56,7 @@ void Rule::AddBinding(const string& key, const EvalString& val) {
   bindings_[key] = val;
 }
 
-const EvalString* Rule::GetBinding(const string& key) const {
+const EvalString* Rule::GetBinding(StringPiece key) const {
   Bindings::const_iterator i = bindings_.find(key);
   if (i == bindings_.end())
     return NULL;
@@ -74,7 +74,7 @@ bool Rule::IsPhony() const {
 }
 
 // static
-bool Rule::IsReservedBinding(const string& var) {
+bool Rule::IsReservedBinding(StringPiece var) {
   return var == "command" ||
       var == "depfile" ||
       var == "dyndep" ||
@@ -88,14 +88,14 @@ bool Rule::IsReservedBinding(const string& var) {
       var == "msvc_deps_prefix";
 }
 
-const map<string, std::unique_ptr<const Rule>>& BindingEnv::GetRules() const {
+const map<string, std::unique_ptr<const Rule>, StringPieceLess>& BindingEnv::GetRules() const {
   return rules_;
 }
 
-string BindingEnv::LookupWithFallback(const string& var,
-                                      const EvalString* eval,
-                                      Env* env) {
-  map<string, string>::iterator i = bindings_.find(var);
+std::string BindingEnv::LookupWithFallback(StringPiece var,
+                                           const EvalString* eval,
+                                           Env* env) {
+  const auto i = bindings_.find(var);
   if (i != bindings_.end())
     return i->second;
 

src/eval_env.h

@@ -21,13 +21,14 @@
 #include <vector>
 
 #include "string_piece.h"
+#include "string_piece_util.h"
 
 struct Rule;
 
 /// An interface for a scope for variable (e.g. "$foo") lookups.
 struct Env {
   virtual ~Env() {}
-  virtual std::string LookupVariable(const std::string& var) = 0;
+  virtual std::string LookupVariable(StringPiece var) = 0;
 };
 
 /// A tokenized string that contains variable references.
@@ -74,16 +75,16 @@ struct Rule {
 
   void AddBinding(const std::string& key, const EvalString& val);
 
-  static bool IsReservedBinding(const std::string& var);
+  static bool IsReservedBinding(StringPiece var);
 
-  const EvalString* GetBinding(const std::string& key) const;
+  const EvalString* GetBinding(StringPiece key) const;
 
  private:
   // Allow the parsers to reach into this object and fill out its fields.
   friend struct ManifestParser;
 
   std::string name_;
-  typedef std::map<std::string, EvalString> Bindings;
+  typedef std::map<std::string, EvalString, StringPieceLess> Bindings;
   Bindings bindings_;
   bool phony_ = false;
 };
@@ -95,26 +96,27 @@ struct BindingEnv : public Env {
   explicit BindingEnv(BindingEnv* parent) : parent_(parent) {}
 
   virtual ~BindingEnv() {}
-  virtual std::string LookupVariable(const std::string& var);
+  virtual std::string LookupVariable(StringPiece var);
 
   void AddRule(std::unique_ptr<const Rule> rule);
-  const Rule* LookupRule(const std::string& rule_name);
-  const Rule* LookupRuleCurrentScope(const std::string& rule_name);
-  const std::map<std::string, std::unique_ptr<const Rule>>& GetRules() const;
+  const Rule* LookupRule(StringPiece rule_name);
+  const Rule* LookupRuleCurrentScope(StringPiece rule_name);
+  const std::map<std::string, std::unique_ptr<const Rule>, StringPieceLess>&
+  GetRules() const;
 
-  void AddBinding(const std::string& key, const std::string& val);
+  void AddBinding(const std::string& key, StringPiece val);
 
   /// This is tricky.  Edges want lookup scope to go in this order:
   /// 1) value set on edge itself (edge_->env_)
   /// 2) value set on rule, with expansion in the edge's scope
   /// 3) value set on enclosing scope of edge (edge_->env_->parent_)
   /// This function takes as parameters the necessary info to do (2).
-  std::string LookupWithFallback(const std::string& var, const EvalString* eval,
+  std::string LookupWithFallback(StringPiece var, const EvalString* eval,
                                  Env* env);
 
 private:
-  std::map<std::string, std::string> bindings_;
-  std::map<std::string, std::unique_ptr<const Rule>> rules_;
+  std::map<std::string, std::string, StringPieceLess> bindings_;
+  std::map<std::string, std::unique_ptr<const Rule>, StringPieceLess> rules_;
   BindingEnv* parent_;
 };
 

src/graph.cc

@@ -393,7 +393,7 @@ struct EdgeEnv : public Env {
 
   EdgeEnv(const Edge* const edge, const EscapeKind escape)
       : edge_(edge), escape_in_out_(escape), recursive_(false) {}
-  virtual string LookupVariable(const string& var);
+  virtual std::string LookupVariable(StringPiece var);
 
   /// Given a span of Nodes, construct a list of paths suitable for a command
   /// line.
@@ -406,7 +406,7 @@ struct EdgeEnv : public Env {
   bool recursive_;
 };
 
-string EdgeEnv::LookupVariable(const string& var) {
+std::string EdgeEnv::LookupVariable(StringPiece var) {
   if (var == "in" || var == "in_newline") {
     int explicit_deps_count =
         static_cast<int>(edge_->inputs_.size() - edge_->implicit_deps_ -
@@ -453,12 +453,16 @@ string EdgeEnv::LookupVariable(const string& var) {
   // at all.
   //
   if (recursive_) {
-    auto it = std::find(lookups_.begin(), lookups_.end(), var);
+    auto it = std::find_if(lookups_.begin(),
+                           lookups_.end(),
+                           [var](const std::string& lookup){
+      return var == lookup;
+    });
     if (it != lookups_.end()) {
       std::string cycle;
       for (; it != lookups_.end(); ++it)
         cycle.append(*it + " -> ");
-      cycle.append(var);
+      cycle.append(var.str_, var.len_);
       Fatal(("cycle in rule variables: " + cycle).c_str());
     }
   }
@@ -467,7 +471,7 @@ string EdgeEnv::LookupVariable(const string& var) {
   const EvalString* eval = edge_->rule_->GetBinding(var);
   bool record_varname = recursive_ && eval;
   if (record_varname)
-    lookups_.push_back(var);
+    lookups_.push_back(var.AsString());
 
   // In practice, variables defined on rules never use another rule variable.
   // For performance, only start checking for cycles after the first lookup.
@@ -508,12 +512,12 @@ std::string Edge::EvaluateCommand(const bool incl_rsp_file) const {
   return command;
 }
 
-std::string Edge::GetBinding(const std::string& key) const {
+std::string Edge::GetBinding(StringPiece key) const {
   EdgeEnv env(this, EdgeEnv::kShellEscape);
   return env.LookupVariable(key);
 }
 
-bool Edge::GetBindingBool(const string& key) const {
+bool Edge::GetBindingBool(StringPiece key) const {
   return !GetBinding(key).empty();
 }
 

src/graph.h

@@ -190,8 +190,8 @@ struct Edge {
   std::string EvaluateCommand(bool incl_rsp_file = false) const;
 
   /// Returns the shell-escaped value of |key|.
-  std::string GetBinding(const std::string& key) const;
-  bool GetBindingBool(const std::string& key) const;
+  std::string GetBinding(StringPiece key) const;
+  bool GetBindingBool(StringPiece key) const;
 
   /// Like GetBinding("depfile"), but without shell escaping.
   std::string GetUnescapedDepfile() const;

src/ninja.cc

@@ -685,12 +685,10 @@ int NinjaMain::ToolRules(const Options* options, int argc, char* argv[]) {
 
   // Print rules
 
-  typedef map<string, std::unique_ptr<const Rule>> Rules;
-  const Rules& rules = state_.bindings_.GetRules();
-  for (Rules::const_iterator i = rules.begin(); i != rules.end(); ++i) {
-    printf("%s", i->first.c_str());
+  for (const auto& r : state_.bindings_.GetRules()) {
+    printf("%s", r.first.c_str());
     if (print_description) {
-      const Rule* rule = i->second.get();
+      const Rule* rule = r.second.get();
       const EvalString* description = rule->GetBinding("description");
       if (description != NULL) {
         printf(": %s", description->Unparse().c_str());

src/string_piece_util.cc

@@ -14,9 +14,10 @@
 
 #include "string_piece_util.h"
 
-#include <algorithm>
+#include <cstring>
 #include <string>
 #include <vector>
+
 using namespace std;
 
 vector<StringPiece> SplitStringPiece(StringPiece input, char sep) {
@@ -76,3 +77,15 @@ bool EqualsCaseInsensitiveASCII(StringPiece a, StringPiece b) {
 
   return true;
 }
+
+bool StringPieceLess::operator()(StringPiece lhs,
+                                 StringPiece rhs) const {
+
+  const std::size_t min_length = std::min(lhs.size(), rhs.size());
+  const int cmp = std::memcmp(lhs.str_, rhs.str_, min_length);
+  if (cmp == 0) {
+    return lhs.size() < rhs.size();
+  } else {
+    return cmp < 0;
+  }
+}

src/string_piece_util.h

@@ -15,6 +15,7 @@
 #ifndef NINJA_STRINGPIECE_UTIL_H_
 #define NINJA_STRINGPIECE_UTIL_H_
 
+#include <algorithm>
 #include <string>
 #include <vector>
 
@@ -30,4 +31,13 @@ inline char ToLowerASCII(char c) {
 
 bool EqualsCaseInsensitiveASCII(StringPiece a, StringPiece b);
 
+/// A comparator for StringPiece that can be used in
+/// `std::map<std::string, V, StringPieceLess>` that will allow using a
+/// StringPiece as a key.
+struct StringPieceLess {
+  using is_transparent = void;
+
+  bool operator()(StringPiece lhs, StringPiece rhs) const;
+};
+
 #endif  // NINJA_STRINGPIECE_UTIL_H_

src/string_piece_util_test.cc

@@ -129,3 +129,16 @@ TEST(StringPieceUtilTest, EqualsCaseInsensitiveASCII) {
   EXPECT_FALSE(EqualsCaseInsensitiveASCII("/", "\\"));
   EXPECT_FALSE(EqualsCaseInsensitiveASCII("1", "10"));
 }
+
+
+TEST(StringPieceUtilTest, StringPieceLess) {
+  const StringPiece strings[] = { "", "A", "AbC", "a", "abc", "c", "ca" };
+  const StringPieceLess less;
+
+  for (int l = 0; l < std::size(strings); ++l) {
+    for (int r = 0; r < std::size(strings); ++r) {
+      EXPECT_EQ(less(strings[l], strings[r]), l < r)
+          << strings[l].AsString() << " < " << strings[r].AsString();
+    }
+  }
+}