Refactor histogram_persistence to be a class.

Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files

Committed: https://crrev.com/33d95806addac4c84a88eb8561ea424ef2097b6d
Cr-Commit-Position: refs/heads/master@{#381386}

[bcwhite, 2016-02-26 19:19:40]

Description was changed from

==========
Refactor histogram_persistence to be a class.

Okay, you were right...  This would be better as a class
with static members.  Specifically, having an iterator
specific to histograms will allow for some other improvements.

BUG=546019
==========

to

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful class and reordered to
match the public/private sections of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
==========

[bcwhite, 2016-02-26 19:20:03]

Description was changed from

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful class and reordered to
match the public/private sections of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
==========

to

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
==========

[bcwhite, 2016-02-26 19:28:56]

Patchset #3 (id:40001) has been deleted

[bcwhite, 2016-02-26 19:49:34]

Patchset #3 (id:60001) has been deleted

[bcwhite, 2016-02-29 01:49:49]

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram....
base/metrics/histogram.h:107: ~Histogram() override;
This has to be made public (like it's base class) in order to make scoped_ptr to
it which is part of the ownership-transfer change.

[bcwhite, 2016-02-29 01:51:11]

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

[Alexei Svitkine (slow), 2016-03-03 18:11:28]

Any chance you could make this easier for me to review by keeping the same order
of functions in the file (with a TODO to change this later), so that the diff is
much easier to view?

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:209: const void* my_histogram =
tentative_histogram.get();
Why cont void*? Use the actual type.

Also, rename to |tentative_histogram_ptr|.

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:104:
PersistentHistogramAllocator* PersistentHistogramAllocator::g_allocator_;
Can this be local to the file?

(If not, the comment should be on its declaration.)

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:311: // Each call resumes from
where it last left off so need persistant
Nit: Grammar.

[bcwhite, 2016-03-04 21:17:16]

> Any chance you could make this easier for me to
> review by keeping the same order of functions in
> the file (with a TODO to change this later), so
> that the diff is much easier to view?

:-D  Done.

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:209: const void* my_histogram =
tentative_histogram.get();
On 2016/03/03 18:11:28, Alexei Svitkine (slow) wrote:
> Why cont void*? Use the actual type.

I don't want it to have a type because it could point to invalid memory after
the next line.  "void" seems the proper type for such.


> Also, rename to |tentative_histogram_ptr|.

Done.

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:104:
PersistentHistogramAllocator* PersistentHistogramAllocator::g_allocator_;
> Can this be local to the file?

Done.  I wasn't sure which was the best style for a class.

https://codereview.chromium.org/1738063002/diff/80001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:311: // Each call resumes from
where it last left off so need persistant
On 2016/03/03 18:11:28, Alexei Svitkine (slow) wrote:
> Nit: Grammar.

Done.

[Alexei Svitkine (slow), 2016-03-07 16:40:43]

Wow, so much easier to review with the same order of methods. Looks great - just
a few comments.

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:252: (histogram_data.bucket_count
+ 1) * sizeof(HistogramBase::Sample)) {
Eek, this if is very verbose.

Suggest adding some local vars above it to make it more readable, e.g. for the
following:

std::numeric_limits<uint32_t>::max() / std::numeric_limits<uint32_t>::max()
and:
(histogram_data.bucket_count + 1) * sizeof(HistogramBase::Sample)

Then, within the if you can refer to the shorter var names making it more
readable.

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:456:
subtle::NoBarrier_Store(&last_created_, histogram_ref);
Can you add a comment explaining this?

Is this change orthogonal to the refactor to the class? If so, suggest making it
separately in a different CL.

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:38: make_scoped_ptr(new
base::LocalPersistentMemoryAllocator(
How about making a helper static method that takes the three params below and
hides the extra complexity of allocating the objects inside it?

Then, callers can be much cleaner, e.g.:

base::PersistentHistogramAllocator::InitGlobalAllocator(
    3 << 20,     // 3 MiB
    0x935DDD43,  // SHA1(BrowserMetrics)
    kAllocatorName);

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
File chrome/installer/setup/installer_metrics.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
chrome/installer/setup/installer_metrics.cc:28:
base::PersistentHistogramAllocator::GetGlobalAllocator()
Nit: Save a pointer to base::PersistentHistogramAllocator::GetGlobalAllocator()
in a local since you call it also right below.

[bcwhite, 2016-03-08 00:24:04]

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:252: (histogram_data.bucket_count
+ 1) * sizeof(HistogramBase::Sample)) {
On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> Eek, this if is very verbose.
> 
> Suggest adding some local vars above it to make it more readable, e.g. for the
> following:
> 
> std::numeric_limits<uint32_t>::max() / std::numeric_limits<uint32_t>::max()
> and:
> (histogram_data.bucket_count + 1) * sizeof(HistogramBase::Sample)
> 
> Then, within the if you can refer to the shorter var names making it more
> readable.

Done.

https://codereview.chromium.org/1738063002/diff/100001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:456:
subtle::NoBarrier_Store(&last_created_, histogram_ref);
On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> Can you add a comment explaining this?
> 
> Is this change orthogonal to the refactor to the class? If so, suggest making
it
> separately in a different CL.

It's part of one of the two CLs mentioned in the overall description.

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:38: make_scoped_ptr(new
base::LocalPersistentMemoryAllocator(
On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> How about making a helper static method that takes the three params below and
> hides the extra complexity of allocating the objects inside it?
> 
> Then, callers can be much cleaner, e.g.:
> 
> base::PersistentHistogramAllocator::InitGlobalAllocator(
>     3 << 20,     // 3 MiB
>     0x935DDD43,  // SHA1(BrowserMetrics)
>     kAllocatorName);

This whole method _is_ a helper method.  :-)

I can't reduce it to parameters to a more general helper method, one that would
be useful outside of this file, because the type (e.g.
LocalPersistentMemoryAllocator) would also need to be passed... and a template
won't work because the parameters to different types may be different.

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
File chrome/installer/setup/installer_metrics.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
chrome/installer/setup/installer_metrics.cc:28:
base::PersistentHistogramAllocator::GetGlobalAllocator()
On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> Nit: Save a pointer to
base::PersistentHistogramAllocator::GetGlobalAllocator()
> in a local since you call it also right below.

Done.

Hmmm...  Perhaps it's better to export the data() and used() methods directly
from the histogram-allocator and call those.

[Alexei Svitkine (slow), 2016-03-08 19:58:29]

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:38: make_scoped_ptr(new
base::LocalPersistentMemoryAllocator(
On 2016/03/08 00:24:04, bcwhite wrote:
> On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> > How about making a helper static method that takes the three params below
and
> > hides the extra complexity of allocating the objects inside it?
> > 
> > Then, callers can be much cleaner, e.g.:
> > 
> > base::PersistentHistogramAllocator::InitGlobalAllocator(
> >     3 << 20,     // 3 MiB
> >     0x935DDD43,  // SHA1(BrowserMetrics)
> >     kAllocatorName);
> 
> This whole method _is_ a helper method.  :-)
> 
> I can't reduce it to parameters to a more general helper method, one that
would
> be useful outside of this file, because the type (e.g.
> LocalPersistentMemoryAllocator) would also need to be passed... and a template
> won't work because the parameters to different types may be different.

There's like half a dozen places where you use this pattern, though I now see
that there's two patterns - one with a LocalPMA and other with just a PMA.
Still, I think it would be cleaner to hide some of the complexity inside the API
rather than on the call site.

So I suggest making two helper functions:

base::PersistentHistogramAllocator::InitGlobalAllocator() (for LocalPMA)
and:
base::PersistentHistogramAllocator::InitGlobalAllocatorOverMemory() (for PMA
over memory w/ extra params)

And simplifying the params as I suggest above - which would make the call sites
much cleaner.

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
File chrome/installer/setup/installer_metrics.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/installer/setup...
chrome/installer/setup/installer_metrics.cc:28:
base::PersistentHistogramAllocator::GetGlobalAllocator()
On 2016/03/08 00:24:04, bcwhite wrote:
> On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> > Nit: Save a pointer to
> base::PersistentHistogramAllocator::GetGlobalAllocator()
> > in a local since you call it also right below.
> 
> Done.
> 
> Hmmm...  Perhaps it's better to export the data() and used() methods directly
> from the histogram-allocator and call those.

Exporting them looks good.

[bcwhite, 2016-03-09 03:49:17]

Patchset #6 (id:140001) has been deleted

[bcwhite, 2016-03-09 04:00:20]

Patchset #6 (id:160001) has been deleted

[bcwhite, 2016-03-09 04:01:51]

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1738063002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:38: make_scoped_ptr(new
base::LocalPersistentMemoryAllocator(
On 2016/03/08 19:58:28, Alexei Svitkine (slow) wrote:
> On 2016/03/08 00:24:04, bcwhite wrote:
> > On 2016/03/07 16:40:43, Alexei Svitkine (slow) wrote:
> > > How about making a helper static method that takes the three params below
> and
> > > hides the extra complexity of allocating the objects inside it?
> > > 
> > > Then, callers can be much cleaner, e.g.:
> > > 
> > > base::PersistentHistogramAllocator::InitGlobalAllocator(
> > >     3 << 20,     // 3 MiB
> > >     0x935DDD43,  // SHA1(BrowserMetrics)
> > >     kAllocatorName);
> > 
> > This whole method _is_ a helper method.  :-)
> > 
> > I can't reduce it to parameters to a more general helper method, one that
> would
> > be useful outside of this file, because the type (e.g.
> > LocalPersistentMemoryAllocator) would also need to be passed... and a
template
> > won't work because the parameters to different types may be different.
> 
> There's like half a dozen places where you use this pattern, though I now see
> that there's two patterns - one with a LocalPMA and other with just a PMA.
> Still, I think it would be cleaner to hide some of the complexity inside the
API
> rather than on the call site.
> 
> So I suggest making two helper functions:
> 
> base::PersistentHistogramAllocator::InitGlobalAllocator() (for LocalPMA)
> and:
> base::PersistentHistogramAllocator::InitGlobalAllocatorOverMemory() (for PMA
> over memory w/ extra params)
> 
> And simplifying the params as I suggest above - which would make the call
sites
> much cleaner.

Done.  Though personally I like the explicit creation of it.  I like to see what
is going on.

[bcwhite, 2016-03-09 04:06:26]

Patchset #6 (id:180001) has been deleted

[Alexei Svitkine (slow), 2016-03-09 20:23:15]

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:153: // memory segment is not shared between
processes, this call does nothing.
Can you remind me in what case this is relevant again? I thought we were not
going with the approach that multiple processes could write to the same segment?

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:415: Reference ref,
Nit: Params fit on previous line?

Maybe same for the function above?

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:497:
subtle::NoBarrier_Store(&last_created_, histogram_ref);
Add a comment please.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:517: void
PersistentHistogramAllocator::ImportGlobalHistograms() {
Can this be a member method too now?

Then, you can have lock and iter be members.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:60: // This method will return
NULL if any problem is detected with the data.
Nit: NULL -> null

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:127: // Every call to this method
resumes from the last entry it saw so it costs
Nit: add a , after "saw"

[bcwhite, 2016-03-09 21:14:59]

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:153: // memory segment is not shared between
processes, this call does nothing.
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Can you remind me in what case this is relevant again? I thought we were not
> going with the approach that multiple processes could write to the same
segment?

We're not for the general case, but some cases may.  For example, the current
plans for SyzyASAN are to use file-backed memory that is fixed.  Next start will
use the same file/memory, effectively resuming exactly where it left off.

This code doesn't know exactly how it will be used so has to be accommodating to
the various possibilities.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:415: Reference ref,
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Nit: Params fit on previous line?
> 
> Maybe same for the function above?

86 characters with parameters.  For some others, the parameters do fit but the
brace will not.  I used "git cl format" for these but will double-check.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:497:
subtle::NoBarrier_Store(&last_created_, histogram_ref);
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Add a comment please.

Done.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:517: void
PersistentHistogramAllocator::ImportGlobalHistograms() {
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Can this be a member method too now?
> 
> Then, you can have lock and iter be members.

It's one of the "global" methods, which are all static.  There's no need to have
a lock or iterator for the non-global allocators.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:60: // This method will return
NULL if any problem is detected with the data.
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Nit: NULL -> null

Done.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:127: // Every call to this method
resumes from the last entry it saw so it costs
On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> Nit: add a , after "saw"

That wouldn't be correct English.  There is no non-essential information or
reversal of clauses.

But I can replace ", so" with a semicolon which may be more clear.

[Alexei Svitkine (slow), 2016-03-09 21:33:25]

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:153: // memory segment is not shared between
processes, this call does nothing.
On 2016/03/09 21:14:58, bcwhite wrote:
> On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > Can you remind me in what case this is relevant again? I thought we were not
> > going with the approach that multiple processes could write to the same
> segment?
> 
> We're not for the general case, but some cases may.  For example, the current
> plans for SyzyASAN are to use file-backed memory that is fixed.  Next start
will
> use the same file/memory, effectively resuming exactly where it left off.
> 
> This code doesn't know exactly how it will be used so has to be accommodating
to
> the various possibilities.

Hmm, could the code be structured so that this would a hook that the
PersistentHistogramAllocator would register on its own when created in a mode
that fits that use case? I feel the hook should be registered with the
StatisticsRecorder() itself and should be called from there from
FindHistogram(), so that other call sites of FindHistogram() also work with it.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:415: Reference ref,
On 2016/03/09 21:14:58, bcwhite wrote:
> On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > Nit: Params fit on previous line?
> > 
> > Maybe same for the function above?
> 
> 86 characters with parameters.  For some others, the parameters do fit but the
> brace will not.  I used "git cl format" for these but will double-check.

This fits:

void PersistentHistogramAllocator::FinalizeHistogram(Reference ref,
                                                     bool registered) {

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:517: void
PersistentHistogramAllocator::ImportGlobalHistograms() {
On 2016/03/09 21:14:58, bcwhite wrote:
> On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > Can this be a member method too now?
> > 
> > Then, you can have lock and iter be members.
> 
> It's one of the "global" methods, which are all static.  There's no need to
have
> a lock or iterator for the non-global allocators.

I understand it's global/static now, but it doesn't seem to need to be. i.e. the
action is actually exporting histograms from the persistent allocator to the
global histograms recorder. So I can see this being a non-static member function
of the persistent allocator - maybe called
ExportHistogramsToStatisticsRecorder().

And all the places that call it already are (later) checking for the existence
of the allocator object, so they can do it earlier and call it on the object if
it's there.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:127: // Every call to this method
resumes from the last entry it saw so it costs
On 2016/03/09 21:14:59, bcwhite wrote:
> On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > Nit: add a , after "saw"
> 
> That wouldn't be correct English.  There is no non-essential information or
> reversal of clauses.
> 
> But I can replace ", so" with a semicolon which may be more clear.

From:

https://owl.english.purdue.edu/owl/resource/607/02/

"1. Use commas to separate independent clauses when they are joined by any of
these seven coordinating conjunctions: and, but, for, or, nor, so, yet."

e.g. "Yesterday was her brother's birthday, so she took him out to dinner."

Anyway, I'm OK with a ; as well - just the current wording is a bit hard to read
due to saw and so being together.

[bcwhite, 2016-03-10 00:45:26]

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:153: // memory segment is not shared between
processes, this call does nothing.
On 2016/03/09 21:33:25, Alexei Svitkine (very slow) wrote:
> On 2016/03/09 21:14:58, bcwhite wrote:
> > On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > > Can you remind me in what case this is relevant again? I thought we were
not
> > > going with the approach that multiple processes could write to the same
> > segment?
> > 
> > We're not for the general case, but some cases may.  For example, the
current
> > plans for SyzyASAN are to use file-backed memory that is fixed.  Next start
> will
> > use the same file/memory, effectively resuming exactly where it left off.
> > 
> > This code doesn't know exactly how it will be used so has to be
accommodating
> to
> > the various possibilities.
> 
> Hmm, could the code be structured so that this would a hook that the
> PersistentHistogramAllocator would register on its own when created in a mode
> that fits that use case? I feel the hook should be registered with the
> StatisticsRecorder() itself and should be called from there from
> FindHistogram(), so that other call sites of FindHistogram() also work with
it.

Good idea.  Let me think about it and address it in a different CL.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:415: Reference ref,
On 2016/03/09 21:33:25, Alexei Svitkine (very slow) wrote:
> On 2016/03/09 21:14:58, bcwhite wrote:
> > On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > > Nit: Params fit on previous line?
> > > 
> > > Maybe same for the function above?
> > 
> > 86 characters with parameters.  For some others, the parameters do fit but
the
> > brace will not.  I used "git cl format" for these but will double-check.
> 
> This fits:
> 
> void PersistentHistogramAllocator::FinalizeHistogram(Reference ref,
>                                                      bool registered) {

Done.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:517: void
PersistentHistogramAllocator::ImportGlobalHistograms() {
> I understand it's global/static now, but it doesn't seem to need to be. i.e.
the
> action is actually exporting histograms from the persistent allocator to the
> global histograms recorder. So I can see this being a non-static member
function
> of the persistent allocator - maybe called
> ExportHistogramsToStatisticsRecorder().

It could be but I don't think it's wise.  Calling that on any object but the
global allocator would be potentially fatal because only the global allocator
has protections against going away and leaving histograms pointing to nothing.

It could be private with explicit friends allowed to call it... but I think it's
better to put all the safety logic here.  That way we don't have to protect
against others doing the wrong thing because only this code can do it.

I'll look at improving it in that "other CL" I said earlier that I'd do.

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:127: // Every call to this method
resumes from the last entry it saw so it costs
On 2016/03/09 21:33:25, Alexei Svitkine (very slow) wrote:
> On 2016/03/09 21:14:59, bcwhite wrote:
> > On 2016/03/09 20:23:15, Alexei Svitkine (very slow) wrote:
> > > Nit: add a , after "saw"
> > 
> > That wouldn't be correct English.  There is no non-essential information or
> > reversal of clauses.
> > 
> > But I can replace ", so" with a semicolon which may be more clear.
> 
> From:
> 
> https://owl.english.purdue.edu/owl/resource/607/02/
> 
> "1. Use commas to separate independent clauses when they are joined by any of
> these seven coordinating conjunctions: and, but, for, or, nor, so, yet."
> 
> e.g. "Yesterday was her brother's birthday, so she took him out to dinner."
> 
> Anyway, I'm OK with a ; as well - just the current wording is a bit hard to
read
> due to saw and so being together.

Ah...  My mistake.  There wouldn't be a comma if it was "... so costs nothing
..." (no "it" subject); then they wouldn't be "independent".

[Alexei Svitkine (slow), 2016-03-14 19:17:20]

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:51: size_t used() const { return
memory_allocator_->used(); }
Nit: Move these below the memory_allocator() getter.

Also, is necessary to expose all of them? If we just need a couple functions,
like data() and length(), I'd prefer exposing just those.

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:52: PersistentMemoryAllocator*
memory_allocator() {
Nit: Add a newline before this.

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:69: void CreateIterator(Iterator*
iter);
Why not return by value?

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:86: // Create and update any
internal histograms.
Document what "name" is and how it relates to histograms.xml.

[bcwhite, 2016-03-14 23:41:50]

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:51: size_t used() const { return
memory_allocator_->used(); }
On 2016/03/14 19:17:20, Alexei Svitkine (very slow) wrote:
> Nit: Move these below the memory_allocator() getter.
> 
> Also, is necessary to expose all of them? If we just need a couple functions,
> like data() and length(), I'd prefer exposing just those.

Done.

I implemented the others so that the "basic" API was the same for both
persistent allocators.  used() is essential.  Id() and Name() are useful.

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:52: PersistentMemoryAllocator*
memory_allocator() {
On 2016/03/14 19:17:20, Alexei Svitkine (very slow) wrote:
> Nit: Add a newline before this.

Done.

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:69: void CreateIterator(Iterator*
iter);
On 2016/03/14 19:17:20, Alexei Svitkine (very slow) wrote:
> Why not return by value?

This mimics the way iterators are done in the PersistentMemoryAllocator.  I have
ideas on a better way to do them that is more OO and self-contained.

https://codereview.chromium.org/1738063002/diff/240001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:86: // Create and update any
internal histograms.
On 2016/03/14 19:17:20, Alexei Svitkine (very slow) wrote:
> Document what "name" is and how it relates to histograms.xml.

Done.

[Alexei Svitkine (slow), 2016-03-15 22:00:10]

lgtm

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:71: // Create an iterator for
going through all histograms in an allocator.
You were planning to refactor the iterators in an upcoming CL? Add a TODO.

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:87: void
FinalizeHistogram(Reference ref, bool register);
Nit: "register" can be a keyword in C++ and gets syntax highlighted here. Use a
different name to avoid confusion - e.g. bool should_register

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator_unittest.cc (right):

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:23: void TearDown()
override { DestroyPersistentHistogramAllocator(); }
Instead of needing to override SetUp() or TearDown(), you can just make the two
calls in the constructor/dtor. Even if there are multiple test cases, the
ctor/dtor will still be run once. each.

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:27:
allocator_memory_.reset(new char[kAllocatorMemorySize]);
This is only run once at the start of the test - so just making it unconditional
with memset following it. If this ever changes, it can be updated - but no
reason to have the code be more complicated if it's not needed.

[bcwhite, 2016-03-16 00:31:49]

Patchset #10 (id:280001) has been deleted

[bcwhite, 2016-03-16 00:32:39]

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> No (c)

Done.

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:71: // Create an iterator for
going through all histograms in an allocator.
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> You were planning to refactor the iterators in an upcoming CL? Add a TODO.

Already done:
https://codereview.chromium.org/1803253002

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:87: void
FinalizeHistogram(Reference ref, bool register);
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> Nit: "register" can be a keyword in C++ and gets syntax highlighted here. Use
a
> different name to avoid confusion - e.g. bool should_register

Oops.  I'd already fixed it in the .cc file.  Odd that it's not a compiler
error.

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator_unittest.cc (right):

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> No (c)

Done.

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:23: void TearDown()
override { DestroyPersistentHistogramAllocator(); }
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> Instead of needing to override SetUp() or TearDown(), you can just make the
two
> calls in the constructor/dtor. Even if there are multiple test cases, the
> ctor/dtor will still be run once. each.

Done.  Though what's the point of setup/teardown, then, if they're pretty much
the same as the ctor/dtor?

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:27:
allocator_memory_.reset(new char[kAllocatorMemorySize]);
On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> This is only run once at the start of the test - so just making it
unconditional
> with memset following it. If this ever changes, it can be updated - but no
> reason to have the code be more complicated if it's not needed.

Done.

[bcwhite, 2016-03-16 00:35:58]

Description was changed from

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
==========

to

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files
==========

[bcwhite, 2016-03-16 00:40:40]

Patchset #10 (id:300001) has been deleted

[bcwhite, 2016-03-16 01:16:59]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-03-16 01:17:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1738063002/#ps320001
(title: "addressed final review comments by Alexei")

[commit-bot: I haz the power, 2016-03-16 01:17:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738063002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738063002/320001

[commit-bot: I haz the power, 2016-03-16 02:37:53]

Description was changed from

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files
==========

to

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files
==========

[commit-bot: I haz the power, 2016-03-16 02:37:54]

Committed patchset #10 (id:320001)

[commit-bot: I haz the power, 2016-03-16 02:38:55]

Description was changed from

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files
==========

to

==========
Refactor histogram_persistence to be a class.

The number of top-level functions was getting too large
and impeding development of other features due to the
previous lack of OO design.

The code is largely unchanged, just moved into a stateful
class and reordered to match the public/private sections
of the class.

Two other CLs are included here because they fit well with the refactoring:

https://codereview.chromium.org/1689833002/
Add ownership-transfer to histogram management calls.

  This CL changes the interface to use scoped_ptr to
  explicitly document, with std::move, when the transfer
  of ownership is taking place.

https://codereview.chromium.org/1731453002/
Reduce histogram creation time by avoiding import of those just created.

  Attempting to import histograms in the persistent memory
  segment is necessary because it could be shared and thus
  have other processes creating objects within it. However,
  there's no need to import those objects that this process
  created.

  The simple method remembering the "reference" of the last
  histogram created in the allocator catches almost all
  cases and reduces histogram creation time by 40%.

BUG=546019
TBR=grt,thakis
grt: setup/installer_metrics.cc (no logic changes)
thakis: gn & gyp changes for new files

Committed: https://crrev.com/33d95806addac4c84a88eb8561ea424ef2097b6d
Cr-Commit-Position: refs/heads/master@{#381386}
==========

[commit-bot: I haz the power, 2016-03-16 02:38:56]

Patchset 10 (id:??) landed as
https://crrev.com/33d95806addac4c84a88eb8561ea424ef2097b6d
Cr-Commit-Position: refs/heads/master@{#381386}

[Alexei Svitkine (slow), 2016-03-16 15:14:08]

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator_unittest.cc (right):

https://codereview.chromium.org/1738063002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:23: void TearDown()
override { DestroyPersistentHistogramAllocator(); }
On 2016/03/16 00:32:39, bcwhite wrote:
> On 2016/03/15 22:00:10, Alexei Svitkine (very slow) wrote:
> > Instead of needing to override SetUp() or TearDown(), you can just make the
> two
> > calls in the constructor/dtor. Even if there are multiple test cases, the
> > ctor/dtor will still be run once. each.
> 
> Done.  Though what's the point of setup/teardown, then, if they're pretty much
> the same as the ctor/dtor?

Don't know - I agree they seem superfulous.