Take a StringPiece when looking up CommandLine switches.

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
Committed: https://crrev.com/1bd9da921215cca875ec3610db0976ac7ed9909a
Cr-Commit-Position: refs/heads/master@{#330902}

[jackhou1, 2015-04-07 07:59:05]

Patchset #3 (id:40001) has been deleted

[tapted, 2015-04-15 00:07:52]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2015-04-15 00:07:53]

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:146: void ResetStringPieces(
needs a comment

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:280: std::string(switch_string.data(),
switch_string.size())),
switch_string.as_string() more below

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:327: std::string switch_key =
StringToLowerASCII(switch_string);
const?

(const reference would work too, but it relies on obscure C++ behavior to
properly scope the reference to a temporary and tends to scare people)

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:334: auto result =
nit: result -> inserted/insertion

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:57: CommandLine(const CommandLine& other);
it's tempting to make this explicit... I bet a lot of calls are unintentionally
copying this by value when they can just take a reference instead. That's for a
follow-up in any case.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:150: // Switch names should only be lowercase.
nit: should only -> must

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:152: // codegen for the string allocation.
This last sentence in the comment is no longer valid. For a StringPiece, there
is still codegen for the strlen and the stack allocation of the StringPiece,
versus just passing a pointer to the char* in a register. I'd expect a ~24kB hit
to sizes by removing the overload.

I'd suggest keeping the overload, but updating this comment to say something
like "... to avoid inlining the codegen to find the length of the constant and
construct a StringPiece."

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:232: // Used for allocation-free lookups.
nit: mention mapped_type points to an object in |switches_|

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:233: std::map<base::StringPiece, const StringType*>
switches_by_stringpiece_;
It might be less confusing to just store a value for the mapped type - it would
be good to have numbers on the benefit of avoiding the extra copies

[jackhou1, 2015-04-17 00:56:10]

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:146: void ResetStringPieces(
On 2015/04/15 00:07:53, tapted wrote:
> needs a comment

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:280: std::string(switch_string.data(),
switch_string.size())),
On 2015/04/15 00:07:53, tapted wrote:
> switch_string.as_string() more below

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:327: std::string switch_key =
StringToLowerASCII(switch_string);
On 2015/04/15 00:07:53, tapted wrote:
> const?
> 
> (const reference would work too, but it relies on obscure C++ behavior to
> properly scope the reference to a temporary and tends to scare people)

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.cc#n...
base/command_line.cc:334: auto result =
On 2015/04/15 00:07:53, tapted wrote:
> nit: result -> inserted/insertion

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:150: // Switch names should only be lowercase.
On 2015/04/15 00:07:53, tapted wrote:
> nit: should only -> must

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:152: // codegen for the string allocation.
On 2015/04/15 00:07:53, tapted wrote:
> This last sentence in the comment is no longer valid. For a StringPiece, there
> is still codegen for the strlen and the stack allocation of the StringPiece,
> versus just passing a pointer to the char* in a register. I'd expect a ~24kB
hit
> to sizes by removing the overload.
> 
> I'd suggest keeping the overload, but updating this comment to say something
> like "... to avoid inlining the codegen to find the length of the constant and
> construct a StringPiece."

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:232: // Used for allocation-free lookups.
On 2015/04/15 00:07:53, tapted wrote:
> nit: mention mapped_type points to an object in |switches_|

Done.

https://codereview.chromium.org/1063933002/diff/180001/base/command_line.h#ne...
base/command_line.h:233: std::map<base::StringPiece, const StringType*>
switches_by_stringpiece_;
On 2015/04/15 00:07:53, tapted wrote:
> It might be less confusing to just store a value for the mapped type - it
would
> be good to have numbers on the benefit of avoiding the extra copies

I'm think the memory/performance impact would be negligible since CommandLines
typically have very few switches. But I also think the impact on code clarity of
storing a pointer is not that big. Since the key is basically a pointer into
|switches| it seems natural that the mapped_type is as well.

But I don't mind either way if you feel strongly about it.

[tapted, 2015-04-17 01:14:33]

code lg but the CL description needs some sciencey numbers in it to provide
rationale, like the last CL

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc#n...
base/command_line.cc:151: std::map<base::StringPiece, const
CommandLine::StringType*>*
nit: this should have a typedef (I know there's different camps about how useful
they are, but it should be consistent with SwitchMap at least).

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc#n...
base/command_line.cc:282:
DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
rebase?

[jackhou1, 2015-04-22 04:09:42]

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc#n...
base/command_line.cc:151: std::map<base::StringPiece, const
CommandLine::StringType*>*
On 2015/04/17 01:14:33, tapted (OOO until 2015-04-27) wrote:
> nit: this should have a typedef (I know there's different camps about how
useful
> they are, but it should be consistent with SwitchMap at least).

Done.

https://codereview.chromium.org/1063933002/diff/200001/base/command_line.cc#n...
base/command_line.cc:282:
DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
On 2015/04/17 01:14:33, tapted (OOO until 2015-04-27) wrote:
> rebase?

Done.

[jackhou1, 2015-04-22 06:01:36]

jackhou@chromium.org changed reviewers:
+ brettw@chromium.org

[jackhou1, 2015-04-22 06:01:37]

brettw, could you take a look?

This is the follow-up to:
"Enforce lowercase switches when calling CommandLine::HasSwitch(const char*)."
https://codereview.chromium.org/1046363002/

[brettw, 2015-04-24 19:50:50]

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:149: void ResetStringPieces(
Why not make this a private member function? You call it with the same arguments
each time which are just class members.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:153: for (auto it = switches.begin(); it != switches.end();
++it)
auto -> const auto&
Always use const refs when applicable (see style guide).

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:311:
DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
Shouldn't this function require that the switches are lowercase just like the
HasSwitch ones? It seems like the whole API should be consistent.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:329: const std::string switch_key =
StringToLowerASCII(switch_string);
Ditto for lower-case.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.h#ne...
base/command_line.h:155: bool HasSwitch(const char switch_constant[]) const;
I think this second version is no longer necessary, and the comment above is
wrong (you construct a string piece in the implementation so it doesn't save
anything).

[jackhou1, 2015-05-11 13:51:42]

brettw, thanks for reviewing. Apologies for the long hiatus as I was on
vacation. PTAL

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc
File base/command_line.cc (right):

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:149: void ResetStringPieces(
On 2015/04/24 19:50:49, brettw wrote:
> Why not make this a private member function? You call it with the same
arguments
> each time which are just class members.

Done.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:153: for (auto it = switches.begin(); it != switches.end();
++it)
On 2015/04/24 19:50:50, brettw wrote:
> auto -> const auto&
> Always use const refs when applicable (see style guide).

Done.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:311:
DCHECK_EQ(StringToLowerASCII(switch_string.as_string()), switch_string);
On 2015/04/24 19:50:49, brettw wrote:
> Shouldn't this function require that the switches are lowercase just like the
> HasSwitch ones? It seems like the whole API should be consistent.

It has the same DCHECK as HasSwitch. AppendSwitch* ensures incoming switches are
converted to lowercase.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.cc#n...
base/command_line.cc:329: const std::string switch_key =
StringToLowerASCII(switch_string);
On 2015/04/24 19:50:50, brettw wrote:
> Ditto for lower-case.

With AppendSwitch* the string may have come from an external source like argv. I
think we should keep the behavior that CommandLine converts them to lowercase to
save callers from having to.

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/1063933002/diff/240001/base/command_line.h#ne...
base/command_line.h:155: bool HasSwitch(const char switch_constant[]) const;
On 2015/04/24 19:50:50, brettw wrote:
> I think this second version is no longer necessary, and the comment above is
> wrong (you construct a string piece in the implementation so it doesn't save
> anything).

IIUC having the second version saves an appreciable amount on binary size
because the codegen for constructing a StringPiece from a char* exists once in
the implementation, rather than at every callsite.

tapted@'s measurements for when HasSwitch took a std::string are here:
https://chromium.googlesource.com/chromium/src/+/009a1dc860942184fc6abdb4bedf...

Made the comment a bit clearer.

[jackhou1, 2015-05-19 02:47:53]

brettw, ping

[brettw, 2015-05-20 20:33:47]

lgtm

[jackhou1, 2015-05-21 01:30:52]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-05-21 01:32:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1063933002/280001

[commit-bot: I haz the power, 2015-05-21 04:48:07]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-05-21 04:49:53]

Patchset 14 (id:??) landed as
https://crrev.com/1bd9da921215cca875ec3610db0976ac7ed9909a
Cr-Commit-Position: refs/heads/master@{#330902}