Take a StringPiece when looking up CommandLine switches.

This avoids the string allocation when searching for a char* in a
std::map<std::string>.

CommandLine now maintains a parallel map of StringPieces that reference
the strings in |switches_|. StringPiece is trivial to construct from a
string, and only requires a strlen to construct from a char*.

On a profile with 2 extensions, HasSwitch is called ~12k times during
startup. In an ideal situation (no paging/cache pressure), the
string allocation under Windows takes ~137ns on a Xeon E5-2690 @
2.9Ghz. A strlen on a typical switch takes about 50ns, and 91% of calls
pass a char*, so there's a net saving of at least
(137 - 0.9 * 50)ns * 12k = 1.1ms from a typical startup with this
hardware. For context, Startup.BrowserMessageLoopStartTimeFromMainEntry
is typically 280-300ms on the same hardware, so we should get a ~0.4%
improvement.

BUG=472383

Review URL: https://codereview.chromium.org/1063933002

Cr-Commit-Position: refs/heads/master@{#330902}

base/command_line.cc

@@ -93,18 +93,6 @@ void AppendSwitchesAndArguments(CommandLine* command_line,
   }
 }
 
-// Lowercase switches for backwards compatiblity *on Windows*.
-#if defined(OS_WIN)
-std::string LowerASCIIOnWindows(const std::string& string) {
-  return StringToLowerASCII(string);
-}
-#elif defined(OS_POSIX)
-const std::string& LowerASCIIOnWindows(const std::string& string) {
-  return string;
-}
-#endif
-
-
 #if defined(OS_WIN)
 // Quote a string as necessary for CommandLineToArgvW compatiblity *on Windows*.
 string16 QuoteForCommandLineToArgvW(const string16& arg,
@@ -180,6 +168,21 @@ CommandLine::CommandLine(const StringVector& argv)
   InitFromArgv(argv);
 }
 
+CommandLine::CommandLine(const CommandLine& other)
+    : argv_(other.argv_),
+      switches_(other.switches_),
+      begin_args_(other.begin_args_) {
+  ResetStringPieces();
+}
+
+CommandLine& CommandLine::operator=(const CommandLine& other) {
+  argv_ = other.argv_;
+  switches_ = other.switches_;
+  begin_args_ = other.begin_args_;
+  ResetStringPieces();
+  return *this;
+}
+
 CommandLine::~CommandLine() {
 }
 
@@ -249,6 +252,7 @@ void CommandLine::InitFromArgv(int argc,
 void CommandLine::InitFromArgv(const StringVector& argv) {
   argv_ = StringVector(1);
   switches_.clear();
+  switches_by_stringpiece_.clear();
   begin_args_ = 1;
   SetProgram(argv.empty() ? FilePath() : FilePath(argv[0]));
   AppendSwitchesAndArguments(this, argv);
@@ -262,17 +266,18 @@ void CommandLine::SetProgram(const FilePath& program) {
   TrimWhitespace(program.value(), TRIM_ALL, &argv_[0]);
 }
 
-bool CommandLine::HasSwitch(const std::string& switch_string) const {
-  DCHECK_EQ(StringToLowerASCII(switch_string), switch_string);
-  return switches_.find(switch_string) != switches_.end();
+bool CommandLine::HasSwitch(const base::StringPiece& switch_string) const {
+  DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
+  return switches_by_stringpiece_.find(switch_string) !=
+         switches_by_stringpiece_.end();
 }
 
-bool CommandLine::HasSwitch(const char string_constant[]) const {
-  return HasSwitch(std::string(string_constant));
+bool CommandLine::HasSwitch(const char switch_constant[]) const {
+  return HasSwitch(base::StringPiece(switch_constant));
 }
 
 std::string CommandLine::GetSwitchValueASCII(
-    const std::string& switch_string) const {
+    const base::StringPiece& switch_string) const {
   StringType value = GetSwitchValueNative(switch_string);
   if (!IsStringASCII(value)) {
     DLOG(WARNING) << "Value of switch (" << switch_string << ") must be ASCII.";
@@ -286,15 +291,16 @@ std::string CommandLine::GetSwitchValueASCII(
 }
 
 FilePath CommandLine::GetSwitchValuePath(
-    const std::string& switch_string) const {
+    const base::StringPiece& switch_string) const {
   return FilePath(GetSwitchValueNative(switch_string));
 }
 
 CommandLine::StringType CommandLine::GetSwitchValueNative(
-    const std::string& switch_string) const {
-  SwitchMap::const_iterator result =
-    switches_.find(LowerASCIIOnWindows(switch_string));
-  return result == switches_.end() ? StringType() : result->second;
+    const base::StringPiece& switch_string) const {
+  DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
+  auto result = switches_by_stringpiece_.find(switch_string);
+  return result == switches_by_stringpiece_.end() ? StringType()
+                                                  : *(result->second);
 }
 
 void CommandLine::AppendSwitch(const std::string& switch_string) {
@@ -308,14 +314,19 @@ void CommandLine::AppendSwitchPath(const std::string& switch_string,
 
 void CommandLine::AppendSwitchNative(const std::string& switch_string,
                                      const CommandLine::StringType& value) {
-  std::string switch_key(LowerASCIIOnWindows(switch_string));
 #if defined(OS_WIN)
+  const std::string switch_key = StringToLowerASCII(switch_string);
   StringType combined_switch_string(ASCIIToUTF16(switch_key));
 #elif defined(OS_POSIX)
-  StringType combined_switch_string(switch_string);
+  const std::string& switch_key = switch_string;
+  StringType combined_switch_string(switch_key);
 #endif
   size_t prefix_length = GetSwitchPrefixLength(combined_switch_string);
-  switches_[switch_key.substr(prefix_length)] = value;
+  auto insertion =
+      switches_.insert(make_pair(switch_key.substr(prefix_length), value));
+  if (!insertion.second)
+    insertion.first->second = value;
+  switches_by_stringpiece_[insertion.first->first] = &(insertion.first->second);
   // Preserve existing switch prefixes in |argv_|; only append one if necessary.
   if (prefix_length == 0)
     combined_switch_string = kSwitchPrefixes[0] + combined_switch_string;
@@ -453,4 +464,10 @@ CommandLine::StringType CommandLine::GetArgumentsStringInternal(
   return params;
 }
 
+void CommandLine::ResetStringPieces() {
+  switches_by_stringpiece_.clear();
+  for (const auto& entry : switches_)
+    switches_by_stringpiece_[entry.first] = &(entry.second);
+}
+
 }  // namespace base

base/command_line.h

@@ -22,6 +22,7 @@
 
 #include "base/base_export.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_piece.h"
 #include "build/build_config.h"
 
 namespace base {
@@ -40,6 +41,7 @@ class BASE_EXPORT CommandLine {
   typedef StringType::value_type CharType;
   typedef std::vector<StringType> StringVector;
   typedef std::map<std::string, StringType> SwitchMap;
+  typedef std::map<base::StringPiece, const StringType*> StringPieceSwitchMap;
 
   // A constructor for CommandLines that only carry switches and arguments.
   enum NoProgram { NO_PROGRAM };
@@ -52,6 +54,10 @@ class BASE_EXPORT CommandLine {
   CommandLine(int argc, const CharType* const* argv);
   explicit CommandLine(const StringVector& argv);
 
+  // Override copy and assign to ensure |switches_by_stringpiece_| is valid.
+  CommandLine(const CommandLine& other);
+  CommandLine& operator=(const CommandLine& other);
+
   ~CommandLine();
 
 #if defined(OS_WIN)
@@ -142,17 +148,19 @@ class BASE_EXPORT CommandLine {
   void SetProgram(const FilePath& program);
 
   // Returns true if this command line contains the given switch.
-  // Switch names should only be lowercase.
-  // The second override provides an optimized version to avoid inlining the
-  // codegen for the string allocation.
-  bool HasSwitch(const std::string& switch_string) const;
+  // Switch names must be lowercase.
+  // The second override provides an optimized version to avoid inlining codegen
+  // at every callsite to find the length of the constant and construct a
+  // StringPiece.
+  bool HasSwitch(const base::StringPiece& switch_string) const;
   bool HasSwitch(const char switch_constant[]) const;
 
   // Returns the value associated with the given switch. If the switch has no
   // value or isn't present, this method returns the empty string.
-  std::string GetSwitchValueASCII(const std::string& switch_string) const;
-  FilePath GetSwitchValuePath(const std::string& switch_string) const;
-  StringType GetSwitchValueNative(const std::string& switch_string) const;
+  // Switch names must be lowercase.
+  std::string GetSwitchValueASCII(const base::StringPiece& switch_string) const;
+  FilePath GetSwitchValuePath(const base::StringPiece& switch_string) const;
+  StringType GetSwitchValueNative(const base::StringPiece& switch_string) const;
 
   // Get a copy of all switches, along with their values.
   const SwitchMap& GetSwitches() const { return switches_; }
@@ -214,6 +222,11 @@ class BASE_EXPORT CommandLine {
   // also quotes parts with '%' in them.
   StringType GetArgumentsStringInternal(bool quote_placeholders) const;
 
+  // Reconstruct |switches_by_stringpiece| to be a mirror of |switches|.
+  // |switches_by_stringpiece| only contains pointers to objects owned by
+  // |switches|.
+  void ResetStringPieces();
+
   // The singleton CommandLine representing the current process's command line.
   static CommandLine* current_process_commandline_;
 
@@ -223,6 +236,12 @@ class BASE_EXPORT CommandLine {
   // Parsed-out switch keys and values.
   SwitchMap switches_;
 
+  // A mirror of |switches_| with only references to the actual strings.
+  // The StringPiece internally holds a pointer to a key in |switches_| while
+  // the mapped_type points to a value in |switches_|.
+  // Used for allocation-free lookups.
+  StringPieceSwitchMap switches_by_stringpiece_;
+
   // The index after the program and switches, any arguments start here.
   size_t begin_args_;
 };

base/command_line_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -71,7 +72,7 @@ TEST(CommandLineTest, CommandLineConstructor) {
   EXPECT_TRUE(cl.HasSwitch("input-translation"));
 
   EXPECT_EQ("Crepe", cl.GetSwitchValueASCII("spaetzle"));
-  EXPECT_EQ("", cl.GetSwitchValueASCII("Foo"));
+  EXPECT_EQ("", cl.GetSwitchValueASCII("foo"));
   EXPECT_EQ("", cl.GetSwitchValueASCII("bar"));
   EXPECT_EQ("", cl.GetSwitchValueASCII("cruller"));
   EXPECT_EQ("--dog=canine --cat=feline", cl.GetSwitchValueASCII(
@@ -134,7 +135,7 @@ TEST(CommandLineTest, CommandLineFromString) {
   EXPECT_TRUE(cl.HasSwitch("quotes"));
 
   EXPECT_EQ("Crepe", cl.GetSwitchValueASCII("spaetzle"));
-  EXPECT_EQ("", cl.GetSwitchValueASCII("Foo"));
+  EXPECT_EQ("", cl.GetSwitchValueASCII("foo"));
   EXPECT_EQ("", cl.GetSwitchValueASCII("bar"));
   EXPECT_EQ("", cl.GetSwitchValueASCII("cruller"));
   EXPECT_EQ("--dog=canine --cat=feline", cl.GetSwitchValueASCII(
@@ -273,6 +274,7 @@ TEST(CommandLineTest, AppendSwitches) {
   cl.AppendSwitchASCII(switch2, value2);
   cl.AppendSwitchASCII(switch3, value3);
   cl.AppendSwitchASCII(switch4, value4);
+  cl.AppendSwitchASCII(switch5, value4);
   cl.AppendSwitchNative(switch5, value5);
 
   EXPECT_TRUE(cl.HasSwitch(switch1));
@@ -291,6 +293,9 @@ TEST(CommandLineTest, AppendSwitches) {
             L"--switch2=value "
             L"--switch3=\"a value with spaces\" "
             L"--switch4=\"\\\"a value with quotes\\\"\" "
+            // Even though the switches are unique, appending can add repeat
+            // switches to argv.
+            L"--quotes=\"\\\"a value with quotes\\\"\" "
             L"--quotes=\"" + kTrickyQuoted + L"\"",
             cl.GetCommandLineString());
 #endif
@@ -379,4 +384,20 @@ TEST(CommandLineTest, Init) {
   EXPECT_EQ(initial, current);
 }
 
+// Test that copies of CommandLine have a valid StringPiece map.
+TEST(CommandLineTest, Copy) {
+  scoped_ptr<CommandLine> initial(new CommandLine(CommandLine::NO_PROGRAM));
+  initial->AppendSwitch("a");
+  initial->AppendSwitch("bbbbbbbbbbbbbbb");
+  initial->AppendSwitch("c");
+  CommandLine copy_constructed(*initial);
+  CommandLine assigned = *initial;
+  CommandLine::SwitchMap switch_map = initial->GetSwitches();
+  initial.reset();
+  for (const auto& pair : switch_map)
+    EXPECT_TRUE(copy_constructed.HasSwitch(pair.first));
+  for (const auto& pair : switch_map)
+    EXPECT_TRUE(assigned.HasSwitch(pair.first));
+}
+
 } // namespace base