Remove std::string histogram name from HistogramBase object.

Remove std::string histogram name from HistogramBase object.

The std::string implementation is frequently around 28 or 40 bytes
(on 32/64-bit builds) in order to hold small strings without having to
allocate external memory.  Not only do histogram names typically exceed
that amount but most strings are code constants to begin with.  Also,
when persistent histograms are used, the name is copied to persistent
memory meaning that there are frequently 2 ram copies of code constant
strings and 28 bytes of space wasted per histogram object.

Instead, keep only a "const char*" for the histogram name and use
other mechanisms to ensure its permanence should it not be persistent
and not be a code constant.

The resulting savings is 24 (of 64) bytes in HistogramBase on 32-bit
builds and 32 (of 104) bytes on 64-bit.  The Histogram object is thus
reduced from 88/136 bytes to 64/104 bytes.

Basically, the size of all Histogram objects is cut in half plus a
reduction in external strings.

These measurements were done on Windows.  Other implementations of
std::string may not have the small-string optimization and thus the
reduction may be as little as one machine word.


BUG=733380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[bcwhite, 2017-07-06 20:13:30]

Description was changed from

==========
Remove std::string histogram name from HistogramBase object.

The std::string implementation is frequently around 28 or 40 bytes
(on 32/64-bit builds) in order to hold small strings without having to
allocate external memory.  Not only do histogram names typically exceed
that amount but most strings are code constants to begin with.  Also,
when persistent histograms are used, the name is copied to persistent
memory meaning that there are frequently 2 ram copies of code constant
strings and 28 bytes of space wasted per histogram object.

Instead, keep only a "const char*" for the histogram name and use
other mechanisms to ensure its permanence should it not be persistent
and not be a code constant.

The resulting savings is 24 (of 64) bytes in HistogramBase on 32-bit
builds and 32 (of 104) bytes on 64-bit.  The Histogram object is thus
reduced from 88/136 bytes to 64/104 bytes.

Basically, the size of all Histogram objects is cut in half plus a
reduction in external strings.

These measurements were done on Windows.  Other implementations of
std::string may not have the small-string optimization and thus the
reduction may be as little as one machine word.


At least, that WILL be the savings.  Right now each histogram is
keeping a std::string copy of the name as it was during construction
in order to detect those that were constructed as constant when in
fact they were not.  That copy will be removed in a future CL.

BUG=733380
==========

to

==========
Remove std::string histogram name from HistogramBase object.

The std::string implementation is frequently around 28 or 40 bytes
(on 32/64-bit builds) in order to hold small strings without having to
allocate external memory.  Not only do histogram names typically exceed
that amount but most strings are code constants to begin with.  Also,
when persistent histograms are used, the name is copied to persistent
memory meaning that there are frequently 2 ram copies of code constant
strings and 28 bytes of space wasted per histogram object.

Instead, keep only a "const char*" for the histogram name and use
other mechanisms to ensure its permanence should it not be persistent
and not be a code constant.

The resulting savings is 24 (of 64) bytes in HistogramBase on 32-bit
builds and 32 (of 104) bytes on 64-bit.  The Histogram object is thus
reduced from 88/136 bytes to 64/104 bytes.

Basically, the size of all Histogram objects is cut in half plus a
reduction in external strings.

These measurements were done on Windows.  Other implementations of
std::string may not have the small-string optimization and thus the
reduction may be as little as one machine word.


At least, that WILL be the savings.  Right now each histogram is
keeping a std::string copy of the name as it was during construction
in order to detect those that were constructed as constant when in
fact they were not.  That copy will be removed in a future CL.

BUG=733380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[bcwhite, 2017-07-06 20:29:02]

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

[bcwhite, 2017-07-06 20:29:02]

Let me know what you think of this.  It's not something I'd want to submit until
after a branch point so it would have a full 6 weeks to bake it Canary/Dev.

[bcwhite, 2017-07-06 21:01:47]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 21:02:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 21:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 21:04:54]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[bcwhite, 2017-07-07 14:19:07]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 14:19:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-07 14:27:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-07 14:27:24]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Alexei Svitkine (slow), 2017-07-12 21:43:15]

I like the idea in principle, but I worry about it being too error-prone for
callers to pass in a temporary string and the histogram having a pointer to
freed memory.

If there was some way to guard against this, then I think it would be great -
but unfortunately I don't know of how we could do that...

[bcwhite, 2017-07-12 22:44:37]

On 2017/07/12 21:43:15, Alexei Svitkine (slow) wrote:
> I like the idea in principle, but I worry about it being too error-prone for
> callers to pass in a temporary string and the histogram having a pointer to
> freed memory.
> 
> If there was some way to guard against this, then I think it would be great -
> but unfortunately I don't know of how we could do that...

Yup.  It *shouldn't* happen but code that does name.c_str() somewhere would be a
danger.

The way to be safe would be to always GetPermanentName() for all passed strings
unless the Histogram code itself knows it to be safe.

That means that all persistent histograms would be able to use the persistent
strings but anything else would make copies.  It's still much better than the
existing way where it always makes a copy.

I wanted to explore this way first to see if it works and running it myself
didn't encounter any problems with the check.  But that wouldn't prevent errors
from creeping in later and I can't think of any run-time check to verify if a
string comes from code-space or not.

[bcwhite, 2017-07-13 18:15:10]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 18:15:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-07-13 18:16:07]

Description was changed from

==========
Remove std::string histogram name from HistogramBase object.

The std::string implementation is frequently around 28 or 40 bytes
(on 32/64-bit builds) in order to hold small strings without having to
allocate external memory.  Not only do histogram names typically exceed
that amount but most strings are code constants to begin with.  Also,
when persistent histograms are used, the name is copied to persistent
memory meaning that there are frequently 2 ram copies of code constant
strings and 28 bytes of space wasted per histogram object.

Instead, keep only a "const char*" for the histogram name and use
other mechanisms to ensure its permanence should it not be persistent
and not be a code constant.

The resulting savings is 24 (of 64) bytes in HistogramBase on 32-bit
builds and 32 (of 104) bytes on 64-bit.  The Histogram object is thus
reduced from 88/136 bytes to 64/104 bytes.

Basically, the size of all Histogram objects is cut in half plus a
reduction in external strings.

These measurements were done on Windows.  Other implementations of
std::string may not have the small-string optimization and thus the
reduction may be as little as one machine word.


At least, that WILL be the savings.  Right now each histogram is
keeping a std::string copy of the name as it was during construction
in order to detect those that were constructed as constant when in
fact they were not.  That copy will be removed in a future CL.

BUG=733380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove std::string histogram name from HistogramBase object.

The std::string implementation is frequently around 28 or 40 bytes
(on 32/64-bit builds) in order to hold small strings without having to
allocate external memory.  Not only do histogram names typically exceed
that amount but most strings are code constants to begin with.  Also,
when persistent histograms are used, the name is copied to persistent
memory meaning that there are frequently 2 ram copies of code constant
strings and 28 bytes of space wasted per histogram object.

Instead, keep only a "const char*" for the histogram name and use
other mechanisms to ensure its permanence should it not be persistent
and not be a code constant.

The resulting savings is 24 (of 64) bytes in HistogramBase on 32-bit
builds and 32 (of 104) bytes on 64-bit.  The Histogram object is thus
reduced from 88/136 bytes to 64/104 bytes.

Basically, the size of all Histogram objects is cut in half plus a
reduction in external strings.

These measurements were done on Windows.  Other implementations of
std::string may not have the small-string optimization and thus the
reduction may be as little as one machine word.


BUG=733380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2017-07-13 18:28:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 18:28:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[bcwhite, 2017-07-13 20:57:54]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-13 20:58:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 21:13:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 21:13:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Alexei Svitkine (slow), 2017-07-17 21:34:44]

https://codereview.chromium.org/2973863002/diff/40001/base/metrics/histogram_...
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/2973863002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_base.cc:191: char const*
HistogramBase::GetPermanentName(const std::string& name) {
Can we avoid having an extra temporary string constructed for when the original
input is a literal?

I believe with your CL, it currently works thus:

HISTOGRAM_*() macro calls const char* version of FactoryGet(). Which then
creates a std::string and calls regular version of FactoryGet(), which then
calls Factory() which stores a reference to that. Which then calls
GetPermanentName() with that entry. Now, since name is const here, the insert()
call on line 199 will make a copy of the name and add it to the set.

So there's a temporary intermediate copy.

I think we can avoid this by plumbing const char* version through and then
having the std::string be created just here on line 199 and so no extra
temporary needed.

[bcwhite, 2017-07-18 01:07:16]

https://codereview.chromium.org/2973863002/diff/40001/base/metrics/histogram_...
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/2973863002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_base.cc:191: char const*
HistogramBase::GetPermanentName(const std::string& name) {
On 2017/07/17 21:34:43, Alexei Svitkine (slow) wrote:
> Can we avoid having an extra temporary string constructed for when the
original
> input is a literal?
> 
> I believe with your CL, it currently works thus:
> 
> HISTOGRAM_*() macro calls const char* version of FactoryGet(). Which then
> creates a std::string and calls regular version of FactoryGet(), which then
> calls Factory() which stores a reference to that. Which then calls
> GetPermanentName() with that entry. Now, since name is const here, the
insert()
> call on line 199 will make a copy of the name and add it to the set.
> 
> So there's a temporary intermediate copy.
> 
> I think we can avoid this by plumbing const char* version through and then
> having the std::string be created just here on line 199 and so no extra
> temporary needed.

We might also be able to make the name parameter be a "moveable" string to avoid
the copy.  That would probably need a lot of fixes throughout the code, though,
so probably not worth the effort.

And of course, std::string is free to do internal optimizations, too, though I
don't know if it does any.

I'd need two GetPermanentName() methods: one that takes std::string (for when
it's a constructed name) and one that takes const char*... though taking
std::string&& (i.e. movable) might be even better as it would allow the former
to call the latter with a copy rather than duplicate the logic.

Is it worth it considering that persistence is now the 100% default and so this
extra-copy path only gets called when persistence hasn't yet been enabled or
when the memory buffer is full?

[bcwhite, 2017-07-31 16:05:26]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-31 16:05:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-31 16:14:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-31 16:14:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bcwhite, 2017-08-02 16:30:41]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 16:30:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-02 16:42:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-02 16:42:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

[bcwhite, 2017-08-02 17:43:11]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-02 17:43:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-02 18:01:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-02 18:01:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-08-10 16:48:49]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-10 16:48:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-10 17:10:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-10 17:10:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bcwhite, 2017-08-11 13:41:32]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-11 13:41:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-11 13:57:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-11 13:57:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-08-11 15:09:50]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-11 15:09:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-11 15:25:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-11 15:25:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-08-31 18:25:26]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-31 18:25:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-31 18:36:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-08-31 18:36:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bcwhite, 2017-09-14 18:19:09]

Patchset #7 (id:120001) has been deleted