Optimize size of UMA histogram macros.

Optimize machine code size of UMA histogram macros.

Adds overloads to the histogram FactoryGet() functions
that take a const char* param, instead of std::string for
the name. This avoids the std::string constructor being
inlined at call-sites when a C string is passed (but call
sites that pass std::strings already will continue to use
the std::string functions).

In my Mac Release build, this change results in a 271K
reduction to Chromium Framework binary size.

BUG=343946
Committed: https://crrev.com/5c2d502d34ea0f162d7fc5c916b1cff9e6acb0b4
Cr-Commit-Position: refs/heads/master@{#335178}

[Alexei Svitkine (slow), 2015-06-17 15:18:39]

asvitkine@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-06-17 22:25:38]

Can we use StringPiece instead of overloads?

[Alexei Svitkine (slow), 2015-06-17 23:05:33]

That would be inlining the StringPiece constructor instead of the
std::string one. I suspect it will result in less savings compared to the
overload.

If you prefer, I can still try it, but I don't think it will help quite as
much.
On 17 Jun 2015 6:25 pm, <isherman@chromium.org> wrote:

> Can we use StringPiece instead of overloads?
>
> https://codereview.chromium.org/1195473002/
>

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

[Ilya Sherman, 2015-06-17 23:30:38]

On 2015/06/17 23:05:33, Alexei Svitkine wrote:
> That would be inlining the StringPiece constructor instead of the
> std::string one. I suspect it will result in less savings compared to the
> overload.
> 
> If you prefer, I can still try it, but I don't think it will help quite as
> much.

Could you please test it?  It's good to have concrete data =)

[Alexei Svitkine (slow), 2015-06-18 16:36:49]

Okay, I tried using StringPiece and that resulted in 246k in savings (as opposed
to 271k - so 25k worse).

That approach would also introduce a few string copy operations (going from
string -> string piece -> string) in the case when the param to the macro is
already a std::string (though admittedly this should be pretty rare, so maybe
not worth worrying about).

[Alexei Svitkine (slow), 2015-06-18 19:01:29]

(So based on the results above, I still think the overloads are a better
solution. 25k is still worthwhile savings.)

[Ilya Sherman, 2015-06-18 21:10:04]

On 2015/06/18 16:36:49, Alexei Svitkine wrote:
> Okay, I tried using StringPiece and that resulted in 246k in savings (as
opposed
> to 271k - so 25k worse).

Thanks for running the numbers!

> That approach would also introduce a few string copy operations (going from
> string -> string piece -> string) in the case when the param to the macro is
> already a std::string (though admittedly this should be pretty rare, so maybe
> not worth worrying about).

I'm pretty sure that the intent behind StringPiece is to never copy strings, but
instead to capture const-references (or the equivalent for C-strings).  So, I
don't think this is going to be an issue.

> (So based on the results above, I still think the overloads are a better
> solution. 25k is still worthwhile savings.)

I'd personally prefer to go with a simpler API.  Using StringPiece gives us 90%
of the savings that overloading does, so it's still a huge win.  I'm open to
being swayed if 25K is really a significant amount, but I currently think it's
an acceptable cost to save 100 lines of (admittedly, not very complex) code.

[Alexei Svitkine (slow), 2015-06-18 21:28:13]

On Thu, Jun 18, 2015 at 5:10 PM, <isherman@chromium.org> wrote:

> On 2015/06/18 16:36:49, Alexei Svitkine wrote:
>
>> Okay, I tried using StringPiece and that resulted in 246k in savings (as
>>
> opposed
>
>> to 271k - so 25k worse).
>>
>
> Thanks for running the numbers!
>
>  That approach would also introduce a few string copy operations (going
>> from
>> string -> string piece -> string) in the case when the param to the macro
>> is
>> already a std::string (though admittedly this should be pretty rare, so
>> maybe
>> not worth worrying about).
>>
>
> I'm pretty sure that the intent behind StringPiece is to never copy
> strings, but
> instead to capture const-references (or the equivalent for C-strings).
> So, I
> don't think this is going to be an issue.


So the problem is the std::string -> StringPiece -> std::string (for
Histogram member) conversion. This sequence requires a copy when the final
std::string is created. But I guess it's actually no worse than what the
existing code does (i.e. that copy will still happen in the other codepath)
- so you're right, this point is moot.

(We can't store that member variable as a StringPiece because we don't have
a guarantee that the caller isn't passing a temporary std::string to the
macros, which will make the StringPiece point to invalid data once that
temporary as gone.)


>
>  (So based on the results above, I still think the overloads are a better
>> solution. 25k is still worthwhile savings.)
>>
>
> I'd personally prefer to go with a simpler API.  Using StringPiece gives
> us 90%
> of the savings that overloading does, so it's still a huge win.  I'm open
> to
> being swayed if 25K is really a significant amount, but I currently think
> it's
> an acceptable cost to save 100 lines of (admittedly, not very complex)
> code.
>

I'd be more persuaded if it was only a few kilobytes, but 25k is still
significant. For example, the Facebook Lite app Facebook released is
entirely contained in 250k[1]!

I've heard there was evidence that saving in download size results in
greater install rates - and also have other side-benefits (e.g. better
cache locality of code). It's especially important in emerging markets for
example, where data is more limited / connections are slower.

For example, a similar change to add the C-string override to
base::CommandLine::HasSwitch() was done because it saved 76k - which is not
much different than 25k:
https://codereview.chromium.org/1007283012

[1] http://facebook-lite.en.uptodown.com/android


>
> https://codereview.chromium.org/1195473002/
>
>

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

[Ilya Sherman, 2015-06-18 22:13:39]

Fair enough, I suppose it's fine either way, and it's true that saving on binary
size is important both for emerging markets and for download success rates in
general.  LGTM.  Thanks for the optimization.

[Alexei Svitkine (slow), 2015-06-18 23:01:50]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-06-18 23:02:16]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-06-18 23:06:32]

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

[commit-bot: I haz the power, 2015-06-19 00:37:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-19 00:38:46]

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