Provide CommandLine::HasSwitch(const char*) to save 76kB in sizes perf

Provide CommandLine::HasSwitch(const char*) to save 76kB in sizes perf

Currently every callsite to CommandLine::HasSwitch() has to generate
code to allocate a string on the stack, malloc(), copy characters from
the switch constant, and then free things when it goes out of scope.

Testing on a Mac release build, this results in about 76kB of generated
object code. (about 40 bytes for each of the ~1928 calls in the non-test
codebase).

Adding a char[] overload allows this codegen to happen only once. And
there's no lost inlining benefit since the new overload can inline its
call instead.

BUG=468184
Committed: https://crrev.com/009a1dc860942184fc6abdb4bedf854b15da7d91
Cr-Commit-Position: refs/heads/master@{#322737}

[tapted, 2015-03-18 04:53:05]

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

[tapted, 2015-03-18 08:32:13]

Patchset #4 (id:60001) has been deleted

[tapted, 2015-03-18 08:32:23]

Patchset #2 (id:20001) has been deleted

[tapted, 2015-03-18 08:51:45]

Hi Nico, wdyt?

https://codereview.chromium.org/1007283012/diff/80001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/1007283012/diff/80001/base/command_line.h#new...
base/command_line.h:145: // (Switch names are case-insensitive).
This comment isn't really true except on Windows....  earlier patchsets rolled
in a change for the new overload to DCHECK that the string constant argument was
lowercase to begin with. But that would be an orthogonal change

mallocing/copying the string *again* every time HasSwitch is called on windows
is probably a non-trivial hit to performance (likely startup path), and would be
good to analyze separately.

[Nico, 2015-03-19 04:51:54]

lgtm (say const char*, not const char[] tho)

I thought I remembered some discussion on a StringRef / StringView kind of class
in base (a char* and a length, with constructors for const char* and string&
– this is small enough to be passed in registers, very efficient to construct
from both string and char*, and generally very useful), but I can't find it now.

[Nico, 2015-03-19 04:53:52]

lgtm (say const char*, not const char[] tho)

I thought I remembered some discussion on a StringRef / StringView kind of class
in base (a char* and a length, with constructors for const char* and string&
– this is small enough to be passed in registers, very efficient to construct
from both string and char*, and generally very useful), but I can't find it now.

[jackhou1, 2015-03-19 05:59:19]

On 2015/03/19 04:53:52, Nico (traveling) wrote:
> lgtm (say const char*, not const char[] tho)
> 
> I thought I remembered some discussion on a StringRef / StringView kind of
class
> in base (a char* and a length, with constructors for const char* and string&
> – this is small enough to be passed in registers, very efficient to
construct
> from both string and char*, and generally very useful), but I can't find it
now.

*vroom* base/strings/string_piece.h

[tapted, 2015-03-19 12:32:52]

On 2015/03/19 05:59:19, jackhou1 wrote:
> On 2015/03/19 04:53:52, Nico (traveling) wrote:
> > lgtm (say const char*, not const char[] tho)

Done (updated the CL description/subject, since I think that's what you meant --
I could go either way on the code (compiler shouldn't care), but `const char
string_constant[]` seemed like a marginally better fit.

> > I thought I remembered some discussion on a StringRef / StringView kind of
> class
> > in base (a char* and a length, with constructors for const char* and string&
> > – this is small enough to be passed in registers, very efficient to
> construct
> > from both string and char*, and generally very useful), but I can't find it
> now.
> 
> *vroom* base/strings/string_piece.h

Thanks! I'll have a play with that in my reference build. I'm curious if nothing
else :). If there's no difference to sizes improvement, I'll update this CL for
re-review. Otherwise I'll commit this when it's quiet and keep an eye on sizes
perf to make sure it all checks out.

[Nico, 2015-03-19 15:07:43]

On 2015/03/19 05:59:19, jackhou1 wrote:
> On 2015/03/19 04:53:52, Nico (traveling) wrote:
> > lgtm (say const char*, not const char[] tho)
> > 
> > I thought I remembered some discussion on a StringRef / StringView kind of
> class
> > in base (a char* and a length, with constructors for const char* and string&
> > – this is small enough to be passed in registers, very efficient to
> construct
> > from both string and char*, and generally very useful), but I can't find it
> now.
> 
> *vroom* base/strings/string_piece.h

There it is, thanks! Too much llvm recently. (StringRef is the corresponding
thing in llvm, string_view is the current name of the proposal to add something
like this to the c++ standard library.)

[tapted, 2015-03-29 23:18:54]

On 2015/03/19 12:32:52, tapted wrote:
> On 2015/03/19 05:59:19, jackhou1 wrote:
> > On 2015/03/19 04:53:52, Nico (traveling) wrote:
> > > lgtm (say const char*, not const char[] tho)
> 
> Done (updated the CL description/subject, since I think that's what you meant
--
> I could go either way on the code (compiler shouldn't care), but `const char
> string_constant[]` seemed like a marginally better fit.
> 
> > > I thought I remembered some discussion on a StringRef / StringView kind of
> > class
> > > in base (a char* and a length, with constructors for const char* and
string&
> > > – this is small enough to be passed in registers, very efficient to
> > construct
> > > from both string and char*, and generally very useful), but I can't find
it
> > now.
> > 
> > *vroom* base/strings/string_piece.h
> 
> Thanks! I'll have a play with that in my reference build. I'm curious if
nothing
> else :). If there's no difference to sizes improvement, I'll update this CL
for
> re-review. Otherwise I'll commit this when it's quiet and keep an eye on sizes
> perf to make sure it all checks out.

Finally got back to my "lab" to check this - StringPiece still requires a
constructor call, as well as a call to ~strlen that gets inlined, so ends up
generating a noticeable amount of code at each callsite. On my mac reference
build, we get:
ToT: 175132 kB
StringPiece: 175080 kB
char[]: 175056 kB

StringPiece saves 52kb, char[] saves 76kB.

So, I think I'll stick with what's here. I have something more ambitious in
https://codereview.chromium.org/19925003/diff/1/base/command_line.h for *really*
optimizing HasSwitch. But the CL here is a step towards better measurement of
the overheads of HasSwitch (i.e. before this, the work for the string allocation
is spread out all over the codebase rather than being attributed to HasSwitch),
and an easy win on sizes perf that might otherwise be mis-attributed (i.e. some
other optimization CL might "claim" this sizes perf for itself, when it's really
quite mundane).

For completeness, the StringPiece CL I tested with is
https://codereview.chromium.org/1043803002/ 

And, since it's quiet, now *would* be a good time to commit this.. except the
sizes bots are currently offline - http://build.chromium.org/p/chromium/console
returns 500. I'll see if there's something I can do to fix that, otherwise this
will have to wait a little bit longer.

[tapted, 2015-03-30 02:07:42]

On 2015/03/29 23:18:54, tapted wrote:
> And, since it's quiet, now *would* be a good time to commit this.. except the
> sizes bots are currently offline -
http://build.chromium.org/p/chromium/console
> returns 500. I'll see if there's something I can do to fix that, otherwise
this
> will have to wait a little bit longer.

So actually the bots and perf graphs are fine, it's just the buildbot webui
that's failing to scrape the data.

I verified this on the perf graphs, and checked on the baselines. Turns out...
sizes regressions of 1528kB (filed http://crbug.com/471609 - mac only) and 320kB
(filed http://crbug.com/471610) had recently crept in - wow.

I'll go ahead with this CL while it's quiet (this week... is unlikely to get
quiet again :/), and make sure the perf graphs agree with me.

[tapted, 2015-03-30 02:08:04]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-03-30 02:08:14]

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

[commit-bot: I haz the power, 2015-03-30 03:57:20]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-30 03:57:49]

Patchset 3 (id:??) landed as
https://crrev.com/009a1dc860942184fc6abdb4bedf854b15da7d91
Cr-Commit-Position: refs/heads/master@{#322737}

[tapted, 2015-03-30 05:12:45]

On 2015/03/19 12:32:52, tapted wrote:
> and keep an eye on sizes
> perf to make sure it all checks out.

It all checks out - details at http://crbug.com/468184#c3 (tl;dr 80-112kB
recovered on all desktop platforms, and even 21kB on android).

[Nico, 2015-03-30 18:44:46]

Thanks for filing the size issues!

( :-( )

On Sun, Mar 29, 2015 at 7:07 PM, <tapted@chromium.org> wrote:

> On 2015/03/29 23:18:54, tapted wrote:
>
>> And, since it's quiet, now *would* be a good time to commit this.. except
>> the
>> sizes bots are currently offline -
>>
> http://build.chromium.org/p/chromium/console
>
>> returns 500. I'll see if there's something I can do to fix that, otherwise
>>
> this
>
>> will have to wait a little bit longer.
>>
>
> So actually the bots and perf graphs are fine, it's just the buildbot webui
> that's failing to scrape the data.
>
> I verified this on the perf graphs, and checked on the baselines. Turns
> out...
> sizes regressions of 1528kB (filed http://crbug.com/471609 - mac only)
> and 320kB
> (filed http://crbug.com/471610) had recently crept in - wow.
>
> I'll go ahead with this CL while it's quiet (this week... is unlikely to
> get
> quiet again :/), and make sure the perf graphs agree with me.
>
> https://codereview.chromium.org/1007283012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.