Change uses of base::JoinString to pass StringPieces where possible.

base/feature_list.cc

Index: base/feature_list.cc
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 353136c12be5c73516c5c89a644558a60fba336b..8dbde97eb4b2c356ac1aad0055e7230a77fced18 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -228,9 +228,9 @@ FieldTrial* FeatureList::GetFieldTrial(const Feature& feature) {
 }
 
 // static
-std::vector<std::string> FeatureList::SplitFeatureListString(
-    const std::string& input) {
-  return SplitString(input, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+std::vector<base::StringPiece> FeatureList::SplitFeatureListString(
+    base::StringPiece input) {
+  return SplitStringPiece(input, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
 }
 
 // static
@@ -339,8 +339,8 @@ FieldTrial* FeatureList::GetAssociatedFieldTrial(const Feature& feature) {
 void FeatureList::RegisterOverridesFromCommandLine(
     const std::string& feature_list,
     OverrideState overridden_state) {
-  for (const auto& value : SplitFeatureListString(feature_list)) {
-    StringPiece feature_name(value);
+  for (auto value : SplitFeatureListString(feature_list)) {

[dcheng, 2017/03/15 10:02:36]

Hmmm... StringPiece are pretty cheap, but I suspect const auto& might be better
here. Or does the compiler optimize them to the same thing?

[Matt Giuca, 2017/03/17 00:16:27]

I'm just going by the summary at the top of string_piece.h:

    Prefer passing StringPieces by value:
      void MyFunction(StringPiece arg);
    If circumstances require, you may also pass by const reference:
      void MyFunction(const StringPiece& arg);  // not preferred
    Both of these have the same lifetime semantics.  Passing by value
    generates slightly smaller code.

Even though it isn't an argument passing, it seems like the same logic would
apply.

I had a brief look at the asm code on Linux. The by-value version seems to
allocate an additional 16 bytes in the stack frame (136 vs 120 bytes) (weird
since the difference between a const StringPiece& and a StringPiece is only 8
bytes; I guess padding). This implies that the by-reference version does an
additional dereference somewhere, though I can't see where. I think the point of
this edict is that in the case of StringPiece, the trade-off between speed and
space is decided in favour of speed.

[dcheng, 2017/03/17 05:28:34]

Mind pasting the assembly somewhere?

(I know it's what the advice at the top says, but we haven't measured it a lot,
and I've heard anecdotal reports that passing anything bigger than a pointer by
value ends up being less optimal for both size and speed)

[Matt Giuca, 2017/03/21 08:28:48]

OK I did a deeper analysis. I'm not sure about the trade-off of passing 8-byte
pointers versus 16-byte objects.

For a simple loop (that doesn't call any *methods* on the stringpieces), it does
generate exactly the same code:
https://gist.github.com/mgiuca/baf51bad7e3bb2d98b9965fda9e8982c

The problem I ran into here is the call to StringPiece::find; because find()
itself for some reason takes a reference, it forces a stack allocation if you
are iterating by value (but not if you are iterating by reference), and
therefore the by-value version is much less efficient. Write-up here, with more
assembly:
https://bugs.chromium.org/p/chromium/issues/detail?id=703547

So for this code, the immediate fix it to iterate by reference, not value. But
if I changed the implementation of the methods in StringPiece, it would make
this code efficient (and in fact equivalent, I think).

You kind of have to globally commit to passing StringPieces by value or by
reference. The question is whether passing by value (if we did it everywhere) is
actually less efficient. I suspect it is more expensive to pass 16 bytes than 8
bytes, BUT it means big savings if you get to avoid stack allocations.

[Matt Giuca, 2017/03/27 03:24:58]

After an extensive analysis, my conclusion was that iterating by-value is
*probably* ideal if all the functions take StringPiece by value, but since they
don't (notably, the StringPiece methods), we should iterate by reference for
now.

Updated throughout this CL.

+    StringPiece feature_name = value;
     base::FieldTrial* trial = nullptr;
 
     // The entry may be of the form FeatureName<FieldTrialName - in which case,
@@ -348,7 +348,7 @@ void FeatureList::RegisterOverridesFromCommandLine(
     std::string::size_type pos = feature_name.find('<');
     if (pos != std::string::npos) {
       feature_name.set(value.data(), pos);
-      trial = base::FieldTrialList::Find(value.substr(pos + 1));
+      trial = base::FieldTrialList::Find(value.substr(pos + 1).as_string());
     }
 
     RegisterOverride(feature_name, overridden_state, trial);