Support "shared" histograms between processes.

Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c7o/edit?usp=sharing

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*

Committed: https://crrev.com/5cb99ebcf2cf1450e7a68e7d7fd3d11ce0549175
Cr-Commit-Position: refs/heads/master@{#372760}

[bcwhite, 2015-11-26 18:58:49]

Patchset #8 (id:130001) has been deleted

[bcwhite, 2015-12-03 17:57:05]

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

[bcwhite, 2015-12-03 17:57:06]

Here's the last big piece of the persistent/shared histogram puzzle.

We talked this morning about duplicating a few histograms and comparing them. 
In this CL (done before our talk), it puts them all in persistent memory but is
controlled by Finch.  Is that a better way to do it and compare operation?

[Alexei Svitkine (slow), 2015-12-04 18:27:36]

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

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
base/metrics/histogram.cc:850: }
The code from 817 to 850 is repeated in several places (with some small
differences). Please find a way to refactor this to share more code, e.g. using
a helper function.

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
base/metrics/histogram_base.h:209: PersistentMemoryAllocator::Iterator* iter);
Can you put these in a separate file? HistogramBase is an abstract base class
and shouldn't be concerned with how histograms are allocated.

[bcwhite, 2015-12-07 23:27:19]

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

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
base/metrics/histogram.cc:850: }
On 2015/12/04 18:27:36, Alexei Svitkine (slow) wrote:
> The code from 817 to 850 is repeated in several places (with some small
> differences). Please find a way to refactor this to share more code, e.g.
using
> a helper function.

Done.

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1425533011/diff/170001/base/metrics/histogram...
base/metrics/histogram_base.h:209: PersistentMemoryAllocator::Iterator* iter);
On 2015/12/04 18:27:36, Alexei Svitkine (slow) wrote:
> Can you put these in a separate file? HistogramBase is an abstract base class
> and shouldn't be concerned with how histograms are allocated.

Done.

[bcwhite, 2015-12-10 20:39:44]

Patchset #11 (id:210001) has been deleted

[bcwhite, 2015-12-10 20:39:55]

Patchset #11 (id:230001) has been deleted

[bcwhite, 2015-12-11 02:04:34]

> > The code from 817 to 850 is repeated in several places (with some small
> > differences). Please find a way to refactor this to share more code, e.g.
> using
> > a helper function.
> 
> Done.

I've changed it to a base "Factory" class with derived version for the various
Histogram classes.  I tried anonymous classes but they couldn't access the
protected constructors so I changed the design to embedded classes.

[Alexei Svitkine (slow), 2015-12-15 22:48:53]

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

https://codereview.chromium.org/1425533011/diff/190001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:49:
!parsed_command_line_.HasSwitch(switches::kDisablePersistentMetrics)) {
Please use the base/feature_list.h API instead. This way, you don't need to
check both the field trial and the command line, just the feature.

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
components/metrics/metrics_service.h:333: }
I think I mentioned this before, but my suggestion was to use our own interface
similar to:
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_...

Rather than overloading operators, which is generally not done in Chromium code.

[Alexei Svitkine (slow), 2015-12-15 22:54:03]

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:243: // Only continue here if all
allocations were successful.
If only some of them were successful, what handles the freeing?

Another option would be to allocate it all in one chunk and have the different
parts be one after another in the structure. Possibly with offsets/pointers to
them from the struct head. This reduces the failure cases during allocation.

[Alexei Svitkine (slow), 2015-12-15 23:03:37]

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:117: int32 flags_;
Do these need to be members or could they be params to Build() instead of the
ctor so that they can be used directly?

I know that for e.g. BooleanHistogram, Build() doesn't need that many arguments.
But it could just call the base Build() supplying the defaults there, as is done
in its ctor.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.h:467: class Factory;
It's not clear to me why these have to be declared in the header. I don't see
anything in the header using them?

[bcwhite, 2015-12-16 13:36:34]

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:117: int32 flags_;
On 2015/12/15 23:03:37, Alexei Svitkine (slow) wrote:
> Do these need to be members or could they be params to Build() instead of the
> ctor so that they can be used directly?
> 
> I know that for e.g. BooleanHistogram, Build() doesn't need that many
arguments.
> But it could just call the base Build() supplying the defaults there, as is
done
> in its ctor.

Whether the defaults are set in the ctor or as paremeters doesn't change much
for the caller.

However, the base Build and some of the virtual methods do use them all even if
some are derived or constant based on the histogram type.  If they were
parameters, they would have to be passed to the virtual functions even if not
used, complicating things there.

I think it's also more efficient to set everything in the ctor where the
compiler can easily inline the storing of constants into memory locations as
opposed to parameters passed around which would involve multiple copies even
when they're not going to be used.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.h:467: class Factory;
On 2015/12/15 23:03:37, Alexei Svitkine (slow) wrote:
> It's not clear to me why these have to be declared in the header. I don't see
> anything in the header using them?

Everything related to a class has to be declared within the class definition. 
It's not possible to just add new things within the .cc file.  Thus, the
embedded class must at least be declared here even if it's not referenced or
defined here.

[bcwhite, 2015-12-16 13:54:29]

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
components/metrics/metrics_service.h:333: }
On 2015/12/15 22:48:52, Alexei Svitkine (slow) wrote:
> I think I mentioned this before, but my suggestion was to use our own
interface
> similar to:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_...
> 
> Rather than overloading operators, which is generally not done in Chromium
code.

The code taking this iterator accepts standard STL iterators.  That could be
changed but then it wouldn't be possible to pass in iterators to STL containers
such as ::map or ::vector.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:243: // Only continue here if all
allocations were successful.
On 2015/12/15 22:54:03, Alexei Svitkine (slow) wrote:
> If only some of them were successful, what handles the freeing?

It's not possible to free anything.  The memory would just be lost, but that's
not unreasonable since the segment is too full for any further allocations
anyway.


> Another option would be to allocate it all in one chunk and have the different
> parts be one after another in the structure. Possibly with offsets/pointers to
> them from the struct head. This reduces the failure cases during allocation.

Yes but that introduces complexities such as alignment into this code which
seemed better left to lower-level components.  I can easily allocate the name
string directly after the histogram_data structure since, as a char*, it has no
alignment requirements.

[bcwhite, 2015-12-16 16:23:46]

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

https://codereview.chromium.org/1425533011/diff/190001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:49:
!parsed_command_line_.HasSwitch(switches::kDisablePersistentMetrics)) {
On 2015/12/15 22:48:52, Alexei Svitkine (slow) wrote:
> Please use the base/feature_list.h API instead. This way, you don't need to
> check both the field trial and the command line, just the feature.

Done.

[Alexei Svitkine (slow), 2015-12-16 16:33:49]

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
components/metrics/metrics_service.h:333: }
On 2015/12/16 13:54:29, bcwhite wrote:
> On 2015/12/15 22:48:52, Alexei Svitkine (slow) wrote:
> > I think I mentioned this before, but my suggestion was to use our own
> interface
> > similar to:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_...
> > 
> > Rather than overloading operators, which is generally not done in Chromium
> code.
> 
> The code taking this iterator accepts standard STL iterators.  That could be
> changed but then it wouldn't be possible to pass in iterators to STL
containers
> such as ::map or ::vector.

Do we currently pass in other iterators? If indeed we do and we need to support
that use case, I'm ok with this, but it should be documented in a comment above
the class. If we don't, I think it would be better to simplify it.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:117: int32 flags_;
On 2015/12/16 13:36:34, bcwhite wrote:
> On 2015/12/15 23:03:37, Alexei Svitkine (slow) wrote:
> > Do these need to be members or could they be params to Build() instead of
the
> > ctor so that they can be used directly?
> > 
> > I know that for e.g. BooleanHistogram, Build() doesn't need that many
> arguments.
> > But it could just call the base Build() supplying the defaults there, as is
> done
> > in its ctor.
> 
> Whether the defaults are set in the ctor or as paremeters doesn't change much
> for the caller.
> 
> However, the base Build and some of the virtual methods do use them all even
if
> some are derived or constant based on the histogram type.  If they were
> parameters, they would have to be passed to the virtual functions even if not
> used, complicating things there.
> 
> I think it's also more efficient to set everything in the ctor where the
> compiler can easily inline the storing of constants into memory locations as
> opposed to parameters passed around which would involve multiple copies even
> when they're not going to be used.

Sorry, I didn't mention it, but one of the reason I suggested it is because
protected member variables are discouraged by the style guide. ("Note that data
members should be private.")

So to keep the current structure, you'd need to add accessors for them that will
be called from sub classes, which adds a lot of boilerplate. :\ So I was
thinking passing them as params would be more concise.

Anyway, thinking about this a bit more, I have a third alternate suggestion -
see my comment below.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:188: return NULL;
Nit: nullptr

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:193: BucketRanges* Histogram::Factory::CreateRanges()
{
So, thinking about this more.... given that these type-specific functions are
short, how about just having one factory class and having a switch on
histogram_type in the places that matter? That way, there's no issue with the
member variables being protected per the comment above (they can be private) and
the whole class can be in the anon namespace.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.h:467: class Factory;
On 2015/12/16 13:36:34, bcwhite wrote:
> On 2015/12/15 23:03:37, Alexei Svitkine (slow) wrote:
> > It's not clear to me why these have to be declared in the header. I don't
see
> > anything in the header using them?
> 
> Everything related to a class has to be declared within the class definition. 
> It's not possible to just add new things within the .cc file.  Thus, the
> embedded class must at least be declared here even if it's not referenced or
> defined here.

I guess my point is that it could just be a free-standing class in the anonymous
namespace.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:243: // Only continue here if all
allocations were successful.
On 2015/12/16 13:54:29, bcwhite wrote:
> On 2015/12/15 22:54:03, Alexei Svitkine (slow) wrote:
> > If only some of them were successful, what handles the freeing?
> 
> It's not possible to free anything.  The memory would just be lost, but that's
> not unreasonable since the segment is too full for any further allocations
> anyway.
> 

Okay, that's fair. Please add a comment about that to the code.

> 
> > Another option would be to allocate it all in one chunk and have the
different
> > parts be one after another in the structure. Possibly with offsets/pointers
to
> > them from the struct head. This reduces the failure cases during allocation.
> 
> Yes but that introduces complexities such as alignment into this code which
> seemed better left to lower-level components.  I can easily allocate the name
> string directly after the histogram_data structure since, as a char*, it has
no
> alignment requirements.
>

[bcwhite, 2015-12-17 15:55:48]

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/1425533011/diff/190001/components/metrics/met...
components/metrics/metrics_service.h:333: }
On 2015/12/16 16:33:48, Alexei Svitkine (slow) wrote:
> On 2015/12/16 13:54:29, bcwhite wrote:
> > On 2015/12/15 22:48:52, Alexei Svitkine (slow) wrote:
> > > I think I mentioned this before, but my suggestion was to use our own
> > interface
> > > similar to:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_...
> > > 
> > > Rather than overloading operators, which is generally not done in Chromium
> > code.
> > 
> > The code taking this iterator accepts standard STL iterators.  That could be
> > changed but then it wouldn't be possible to pass in iterators to STL
> containers
> > such as ::map or ::vector.
> 
> Do we currently pass in other iterators? If indeed we do and we need to
support
> that use case, I'm ok with this, but it should be documented in a comment
above
> the class. If we don't, I think it would be better to simplify it.

Yes.  The StatisticsRecorder provides an iterator, too.  It isn't an STL
iterator at the moment but I hope to reduce it to that in later optimizations.

Comment added.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:117: int32 flags_;
> > Whether the defaults are set in the ctor or as paremeters doesn't change
much
> > for the caller.
> > 
> > However, the base Build and some of the virtual methods do use them all even
> if
> > some are derived or constant based on the histogram type.  If they were
> > parameters, they would have to be passed to the virtual functions even if
not
> > used, complicating things there.
> > 
> > I think it's also more efficient to set everything in the ctor where the
> > compiler can easily inline the storing of constants into memory locations as
> > opposed to parameters passed around which would involve multiple copies even
> > when they're not going to be used.
> 
> Sorry, I didn't mention it, but one of the reason I suggested it is because
> protected member variables are discouraged by the style guide. ("Note that
data
> members should be private.")
>
> So to keep the current structure, you'd need to add accessors for them that
will
> be called from sub classes, which adds a lot of boilerplate. :\ So I was
> thinking passing them as params would be more concise.

I think given that the class is a utility class completely embedded within the
implementing .cc file, we're not worried about implementation changes.  I'm
assuming that the reason for not using protected members but rather protected
accessors is so that the implementation can change without necessarily breaking
all derived classes.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:188: return NULL;
On 2015/12/16 16:33:49, Alexei Svitkine (slow) wrote:
> Nit: nullptr

Old habits die hard.  Done.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.cc:193: BucketRanges* Histogram::Factory::CreateRanges()
{
On 2015/12/16 16:33:49, Alexei Svitkine (slow) wrote:
> So, thinking about this more.... given that these type-specific functions are
> short, how about just having one factory class and having a switch on
> histogram_type in the places that matter? That way, there's no issue with the
> member variables being protected per the comment above (they can be private)
and
> the whole class can be in the anon namespace.

I wouldn't even need a class for that.  It would just be the original
FactoryGet() function but with multiple switch statements within it.  It can't
be anonymous, though, because it would have to be a friend of every class to
access the non-public constructors of the various Histogram classes.

This would also require complete knowledge of all the various classes within
that one class/method.  Right now, the LinearHistogram::Factory class has extra
knowledge that indicates how to set descriptions and is used by the overridden
"datafill" method.  With a single class/method, every caller would have to pass
a "descriptions" pointer (null or otherwise) even though most have no knowledge
about what that is.

I think the use of virtual functions is a much cleaner (and more extensible)
method than numerous embedded switch statements.

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

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram.h:467: class Factory;
On 2015/12/16 16:33:49, Alexei Svitkine (slow) wrote:
> On 2015/12/16 13:36:34, bcwhite wrote:
> > On 2015/12/15 23:03:37, Alexei Svitkine (slow) wrote:
> > > It's not clear to me why these have to be declared in the header. I don't
> see
> > > anything in the header using them?
> > 
> > Everything related to a class has to be declared within the class
definition. 
> > It's not possible to just add new things within the .cc file.  Thus, the
> > embedded class must at least be declared here even if it's not referenced or
> > defined here.
> 
> I guess my point is that it could just be a free-standing class in the
anonymous
> namespace.

I tried that originally but such cannot access the non-public constructors of
the various Histogram classes.

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/250001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:243: // Only continue here if all
allocations were successful.
On 2015/12/16 16:33:49, Alexei Svitkine (slow) wrote:
> On 2015/12/16 13:54:29, bcwhite wrote:
> > On 2015/12/15 22:54:03, Alexei Svitkine (slow) wrote:
> > > If only some of them were successful, what handles the freeing?
> > 
> > It's not possible to free anything.  The memory would just be lost, but
that's
> > not unreasonable since the segment is too full for any further allocations
> > anyway.
> > 
> 
> Okay, that's fair. Please add a comment about that to the code.
> 
> > 
> > > Another option would be to allocate it all in one chunk and have the
> different
> > > parts be one after another in the structure. Possibly with
offsets/pointers
> to
> > > them from the struct head. This reduces the failure cases during
allocation.
> > 
> > Yes but that introduces complexities such as alignment into this code which
> > seemed better left to lower-level components.  I can easily allocate the
name
> > string directly after the histogram_data structure since, as a char*, it has
> no
> > alignment requirements.
> > 
> 

Done.  Though I think I'm going to look at embedding the histogram name in the
main data rather than do separate allocations.

[Alexei Svitkine (slow), 2015-12-21 19:32:11]

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

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:124: virtual void FillHistogram(HistogramBase*
histogram) {}
Please document these methods and the class.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:132: };
Add a private: section and DISALLOW_COPY_AND_ASSIGN().

Move any members that are not referenced by subclasses to private section.

For the other ones, add a comment about why they're in protected section.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:144: // Sometimes the bucket info is dynamically
determined in create_ranges().
Nit: create_ranges() -> CreateRanges().

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:167: if (!tentative_histogram) {
Either make this an else or mention in what legit situation can the allocator
return null and why those situation should be handled gracefully. (i.e. if it's
not expected that it should fail, it should be a DCHECK for example)

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:169: tentative_histogram =
HeapAlloc(registered_ranges);
Should this also clear histogram_ref?

Or is the expectation that if the allocator returned null, the ref is also 0? If
so, there should be a DCHECK on that since you have code that checks
histogram_ref on line 179.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:174: tentative_histogram->SetFlags(flags_);
Nit: Move this about FillHistogram() and don't separate the two by a blank line.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:188:
GetPersistentHistogramMemoryAllocator()->SetType(histogram_ref, 0);
Can this logic be contained in the allocator? i.e. just have a single function
on it that takes the histogram ref and a bool about whether it was registered or
not.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:671: protected:
Nit: private:

and add DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:842: BucketRanges* CreateRanges() {
Can this class just extend LinearHistogram and avoid this override?

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram.h
File base/metrics/histogram.h (left):

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.h:282: ~LinearHistogram() override;
Why is this removed?

Non-trivial classes should have explicit out of line destructors.

Otherwise, every file that includes the header will generate machine code for it
which has to be deduped at link time, which causes slower build times.

[bcwhite, 2015-12-22 20:50:38]

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

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:124: virtual void FillHistogram(HistogramBase*
histogram) {}
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Please document these methods and the class.

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:132: };
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Add a private: section and DISALLOW_COPY_AND_ASSIGN().
> 
> Move any members that are not referenced by subclasses to private section.
> 
> For the other ones, add a comment about why they're in protected section.

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:144: // Sometimes the bucket info is dynamically
determined in create_ranges().
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Nit: create_ranges() -> CreateRanges().

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:167: if (!tentative_histogram) {
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Either make this an else or mention in what legit situation can the allocator
> return null and why those situation should be handled gracefully. (i.e. if
it's
> not expected that it should fail, it should be a DCHECK for example)

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:169: tentative_histogram =
HeapAlloc(registered_ranges);
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Should this also clear histogram_ref?
> 
> Or is the expectation that if the allocator returned null, the ref is also 0?
If
> so, there should be a DCHECK on that since you have code that checks
> histogram_ref on line 179.

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:174: tentative_histogram->SetFlags(flags_);
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Nit: Move this about FillHistogram() and don't separate the two by a blank
line.

That will require that any overriden FillHistogram() call the "super" version. 
That should probably be the case anyway.

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:188:
GetPersistentHistogramMemoryAllocator()->SetType(histogram_ref, 0);
On 2015/12/21 19:32:11, Alexei Svitkine (slow) wrote:
> Can this logic be contained in the allocator? i.e. just have a single function
> on it that takes the histogram ref and a bool about whether it was registered
or
> not.

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:671: protected:
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Nit: private:
> 
> and add DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.cc:842: BucketRanges* CreateRanges() {
On 2015/12/21 19:32:10, Alexei Svitkine (slow) wrote:
> Can this class just extend LinearHistogram and avoid this override?

Yes but it would involve changing the LinearHistogram ctor to take a
histogram_type parameter and passing a null descriptions value.  This seemed
cleaner overall.

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram.h
File base/metrics/histogram.h (left):

https://codereview.chromium.org/1425533011/diff/310001/base/metrics/histogram...
base/metrics/histogram.h:282: ~LinearHistogram() override;
On 2015/12/21 19:32:11, Alexei Svitkine (slow) wrote:
> Why is this removed?
> 
> Non-trivial classes should have explicit out of line destructors.
> 
> Otherwise, every file that includes the header will generate machine code for
it
> which has to be deduped at link time, which causes slower build times.

I removed it because it was the only one of the three derived classes that had
one.  I missed the BucketDescriptionMap member variable.

Restored.

[Alexei Svitkine (slow), 2016-01-08 18:52:49]

Sorry, turns out I hadn't previously looked at the histogram_persistence.cc code
in enough detail, so I have a number of comments on that front.

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

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
base/metrics/histogram.cc:951: DISALLOW_COPY_AND_ASSIGN(Factory);
Nit: Add a blank line above this. Same for the others where there's a member
variable declared above them.

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
base/metrics/histogram_persistence.h:68: bool register);
Nit: codereview is highlighting "register" because it's also a C keyword. To
avoid confusion, let's use a different name. e.g. was_registered

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:50: scoped_ptr<PersistentMemoryAllocator>
allocator_;
If this won't be used outside of this file, it should be in an anon namespace.
Same for the things above.

Also, conventions for naming such things is g_allocator.

Finally, I think this would create a static initializer. You may need to use a
base::LazyInstance here, since static initializers are not allowed.

But seems like you're also declaring it in the header. That doesn't look right.

How about declaring a new class - e.g. PersistentHistogramAllocator that has all
these currently free-standing functions as members and itself holding a member
PersistentMemoryAllocator. Then, the PersistentHistogramAllocator can be held by
the StatisticsRecorder.

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:78: // be simple references to histograms
in other processes).
There shouldn't be both a comment in the .h and in the .cc file above the
function. If you want to talk more about the implementation, the comment should
be within the function.

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:170: histogram = new Histogram(
I don't like that all the classes need to be friends with this function. Can
this be avoided? e.g. by this class taking a pointer to a Factory object? The
Factory object can then be responsible for calling the specific ctors.

[bcwhite, 2016-01-12 16:34:57]

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

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
base/metrics/histogram.cc:951: DISALLOW_COPY_AND_ASSIGN(Factory);
On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> Nit: Add a blank line above this. Same for the others where there's a member
> variable declared above them.

Done.

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
base/metrics/histogram_persistence.h:68: bool register);
On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> Nit: codereview is highlighting "register" because it's also a C keyword. To
> avoid confusion, let's use a different name. e.g. was_registered

Done.

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:50: scoped_ptr<PersistentMemoryAllocator>
allocator_;
There didn't seem any difference between a class with only static methods and
global functions.  I can change it if you want but don't see any benefit in it.

The "visibility" issue is annoying.  CreatePersistentHistogram needs to be
defined publicly because it has to be a friend of the Histogram classes in order
to call their non-public constructors.  The PesistentHistogramData class is only
visible because it's passed to CreatePersistentHistogram from within
histogram_persistence.cc.

Let me see what I can do to clean that up...

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:170: histogram = new Histogram(
On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> I don't like that all the classes need to be friends with this function. Can
> this be avoided? e.g. by this class taking a pointer to a Factory object? The
> Factory object can then be responsible for calling the specific ctors.

The Factory object, like the FactoryGet methods, are what call this method (and
do other preparation).  I could create new FactoryGet methods that take all
these parameters and make them public; that would solve the visibility issue and
thus clean up other things.

Okay with that?

[bcwhite, 2016-01-12 21:12:29]

On 2016/01/12 16:34:57, bcwhite wrote:
>
https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram.cc
> File base/metrics/histogram.cc (right):
> 
>
https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
> base/metrics/histogram.cc:951: DISALLOW_COPY_AND_ASSIGN(Factory);
> On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> > Nit: Add a blank line above this. Same for the others where there's a member
> > variable declared above them.
> 
> Done.
> 
>
https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
> File base/metrics/histogram_persistence.h (right):
> 
>
https://codereview.chromium.org/1425533011/diff/330001/base/metrics/histogram...
> base/metrics/histogram_persistence.h:68: bool register);
> On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> > Nit: codereview is highlighting "register" because it's also a C keyword. To
> > avoid confusion, let's use a different name. e.g. was_registered
> 
> Done.
> 
>
https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
> File base/metrics/histogram_persistence.cc (right):
> 
>
https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
> base/metrics/histogram_persistence.cc:50:
scoped_ptr<PersistentMemoryAllocator>
> allocator_;
> There didn't seem any difference between a class with only static methods and
> global functions.  I can change it if you want but don't see any benefit in
it.
> 
> The "visibility" issue is annoying.  CreatePersistentHistogram needs to be
> defined publicly because it has to be a friend of the Histogram classes in
order
> to call their non-public constructors.  The PesistentHistogramData class is
only
> visible because it's passed to CreatePersistentHistogram from within
> histogram_persistence.cc.
> 
> Let me see what I can do to clean that up...
> 
>
https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
> base/metrics/histogram_persistence.cc:170: histogram = new Histogram(
> On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> > I don't like that all the classes need to be friends with this function. Can
> > this be avoided? e.g. by this class taking a pointer to a Factory object?
The
> > Factory object can then be responsible for calling the specific ctors.
> 
> The Factory object, like the FactoryGet methods, are what call this method
(and
> do other preparation).  I could create new FactoryGet methods that take all
> these parameters and make them public; that would solve the visibility issue
and
> thus clean up other things.
> 
> Okay with that?

New "PersistentGet()" methods (beside FactoryGet ones) fix the visibility
problem.

[Alexei Svitkine (slow), 2016-01-12 21:54:45]

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:50: scoped_ptr<PersistentMemoryAllocator>
allocator_;
On 2016/01/12 16:34:57, bcwhite wrote:
> There didn't seem any difference between a class with only static methods and
> global functions.  I can change it if you want but don't see any benefit in
it.

I guess the main benefit is it allows you to have multiple such allocators -
which provides flexibility in terms of use and likely testing. However, if
keeping instance local to the .cc file works and we can hide everything behind
the static functions, I guess I'm OK with this approach initially. We can always
refactor if we find we need the extra flexibility. Thanks.

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

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.cc:89: 
Nit: Remove the extra line.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.cc:473: if (ranges)
Nit: {}'s since body is multi-line.

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

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.h:142: static HistogramBase* PersistentGet(const char*
name,
Add a comment, please.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.h:387: LinearHistogram(const std::string& name,
Please add a comment to these extra constructors.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_base.cc:22: #include "base/synchronization/lock.h"
Doesn't seem like this header or the one on line 15 is used anywhere in this
file?

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_delta_serialization.h (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_delta_serialization.h:34: bool include_persistent);
Document the new param.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:22: kTypeIdHistogram   = 0xF1645910 + 1, 
// SHA1(Histogram) v1
SHA1 identifiers are longer than uint32_t. Do you mean
HashMetricName("Histogram")?

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:35:
base::PersistentMemoryAllocator::Reference ranges_ref;
Nit: The anon namespace can still be inside the base namespace, in which case
you can remove the base:: here.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:57: const Feature
kPersistentHistogramsFeature = {
Nit: No = needed.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:58: "PersistentMetrics",
FEATURE_DISABLED_BY_DEFAULT
Nit: PersistentHistograms

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:67: // such persistent histograms have
already been forgotten.
I think it should be a DCHECK here but we should have a separate
SerPsersistentHistogramMemoryAllocatorForTesting() function that doesn't have
the DCHECK - only the comment about why only the test version allows it.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent histogram
data was not valid; skipped.";
Nit: You can pipe the message into the NOTREACHED().

Same for the other places below.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:147: case HISTOGRAM:
Nit: indent these 2 more.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.h:26: PersistentMemoryAllocator* a);
Nit: longer param name.

[bcwhite, 2016-01-13 13:38:29]

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/350001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:78: // be simple references to histograms
in other processes).
On 2016/01/08 18:52:48, Alexei Svitkine wrote:
> There shouldn't be both a comment in the .h and in the .cc file above the
> function. If you want to talk more about the implementation, the comment
should
> be within the function.

Done.

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

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.cc:89: 
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Nit: Remove the extra line.

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.cc:473: if (ranges)
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Nit: {}'s since body is multi-line.

Done.

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

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.h:142: static HistogramBase* PersistentGet(const char*
name,
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Add a comment, please.

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram.h:387: LinearHistogram(const std::string& name,
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Please add a comment to these extra constructors.

There's one big descriptive comment for the one in Histogram.  I left it out
here (and below) because the preceding ctor also had its comment omitted.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_base.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_base.cc:22: #include "base/synchronization/lock.h"
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Doesn't seem like this header or the one on line 15 is used anywhere in this
> file?

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_delta_serialization.h (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_delta_serialization.h:34: bool include_persistent);
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Document the new param.

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:22: kTypeIdHistogram   = 0xF1645910 + 1, 
// SHA1(Histogram) v1
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> SHA1 identifiers are longer than uint32_t. Do you mean
> HashMetricName("Histogram")?

No.  The number is the first 4 bytes of a sha1 hash.  It could be any arbitrary
integer so long as it's unique.  Trying to do sequential numbering didn't seem
feasible and this seemed better than using a RNG.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:35:
base::PersistentMemoryAllocator::Reference ranges_ref;
On 2016/01/12 21:54:45, Alexei Svitkine wrote:
> Nit: The anon namespace can still be inside the base namespace, in which case
> you can remove the base:: here.

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:57: const Feature
kPersistentHistogramsFeature = {
On 2016/01/12 21:54:45, Alexei Svitkine wrote:
> Nit: No = needed.

Done.  It's actually done both ways elsewhere, sometimes within the same file:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:58: "PersistentMetrics",
FEATURE_DISABLED_BY_DEFAULT
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Nit: PersistentHistograms

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent histogram
data was not valid; skipped.";
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Nit: You can pipe the message into the NOTREACHED().
> 
> Same for the other places below.

But that would omit the warning on Release builds, would it not?  Would you
prefer the message not appear on Release builds?

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:147: case HISTOGRAM:
On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> Nit: indent these 2 more.

Done.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.h:26: PersistentMemoryAllocator* a);
On 2016/01/12 21:54:45, Alexei Svitkine wrote:
> Nit: longer param name.

Done.

[Alexei Svitkine (slow), 2016-01-14 16:43:14]

Another round of comments. Generally ok with most of the structure except would
like to see the iterator/metrics service stuff moved to a future CL that adds
something that hooks into that.

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

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:98: HistogramBase* Build();
Can this have a comment? Specifically also mentioning the ownership of the
return value.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:148: ImportPersistentHistograms();
I don't understand this call. Is it needed? If so, please add an explanation
comment.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:151: if (!histogram) {
Prefer an early return if found, so the rest of the code doesn't need to be
indented.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:157: // Sometimes the bucket info is dynamically
determined in CreateRanges().
Can this comment have more context? When exactly is this the case?

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:167: GetPersistentHistogramMemoryAllocator();
Add a comment above this that mentions that currently the presence of the
allocator is controlled by an base::Feature that's off by default. And that if
one doesn't exist, this allocates the histograms on the heap.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:215: 
Nit: Remove empty line.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:318: bool include_persistent) {
It would be better to make this an enum so that it's clear what's happening from
the callers side.

Then, you can actually have 3 values that all make logical sense:

PERSISTENT_HISTOGRAMS = 1,
NON_PERSISTENT_HISTOGRAM = 2,
ALL_HISTOGRAMS = PERSISTENT_HISTOGRAMS | NON_PERSISTENT_HISTOGRAM

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:324: return
HistogramIterator(histograms_->end(), true);
Why is it correct to pass in true here, rather than relying on a param?

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:48: if
(base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
You define the feature in base/ but only query it in chrome/browser. I don't
think this makes sense.

If we want the feature to only control the browser process' use of persistence,
let's define it in chrome/browser - e.g. in this file. We can then have a
separate feature to control renderers. We should also put Browser in the name
somewhere to make it clear what it controls.

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:58: }
Can you move this new code to a helper function?

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:790: base::PersistentMemoryAllocator*
allocator =
Add a comment about this block, as it's not clear what it's for.

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
Can the metrics service changes be in a separate CL?

Correct me if I'm wrong, but it doesn't seem like anything is being hooked up to
metrics service here anyway - even if the trial is on, we're not going to get
AddPersistentMemorySegment() called yet until we have something coming from a
different source than the browser process. In which case, I suggest including
those changes with the first CL that starts using this - for example the CL that
adds the collection to setup.exe.

(In which case, probably all the iterator changes can also be part of that CL.)

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service_accessor.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service_accessor.cc:19: // Testing Metrics -- DO NOT
COMMIT
Remove from CL.

[bcwhite, 2016-01-14 19:20:26]

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

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:98: HistogramBase* Build();
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> Can this have a comment? Specifically also mentioning the ownership of the
> return value.

Done.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:148: ImportPersistentHistograms();
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> I don't understand this call. Is it needed? If so, please add an explanation
> comment.

Done.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:151: if (!histogram) {
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> Prefer an early return if found, so the rest of the code doesn't need to be
> indented.

Found doesn't mean it's correct.  There are additional checks at the bottom of
the method to ensure that the histogram being requested matches the one that is
known.

This existed in previous code, presumably to ensure that the results are
identical should the same histogram be instantiated via macros at different
places in the code.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:157: // Sometimes the bucket info is dynamically
determined in CreateRanges().
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> Can this comment have more context? When exactly is this the case?

Done.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:167: GetPersistentHistogramMemoryAllocator();
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> Add a comment above this that mentions that currently the presence of the
> allocator is controlled by an base::Feature that's off by default. And that if
> one doesn't exist, this allocates the histograms on the heap.

Done.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:215: 
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:318: bool include_persistent) {
On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> It would be better to make this an enum so that it's clear what's happening
from
> the callers side.
> 
> Then, you can actually have 3 values that all make logical sense:
> 
> PERSISTENT_HISTOGRAMS = 1,
> NON_PERSISTENT_HISTOGRAM = 2,
> ALL_HISTOGRAMS = PERSISTENT_HISTOGRAMS | NON_PERSISTENT_HISTOGRAM

Sure, though I don't see the use-case of that.  You either want:

1) All histograms created locally (true), or
2) All histograms created locally but not reported elsewhere (false)

I would actually prefer to remove the parameter completely and just do the
"false" case but it's more efficient to find shared histograms here than by
independently iterating over a persistent memory segment.  Once reporting
directly from persistent data is complete (a future optimization), I'll likely
go that way.

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:324: return
HistogramIterator(histograms_->end(), true);
On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> Why is it correct to pass in true here, rather than relying on a param?

The second parameter defines what histograms are found within an allocator but
end() doesn't return a valid allocator so the value of the second parameter is
unimportant.

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:48: if
(base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> You define the feature in base/ but only query it in chrome/browser. I don't
> think this makes sense.
> 
> If we want the feature to only control the browser process' use of
persistence,
> let's define it in chrome/browser - e.g. in this file. We can then have a
> separate feature to control renderers. We should also put Browser in the name
> somewhere to make it clear what it controls.

I want the same parameter to control all.  I just haven't gotten to writing the
Renderer part of it.

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:58: }
On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> Can you move this new code to a helper function?

I looked at that and it would have to be a helper function defined in this same
file because this is the only place that knows what type of allocator to create.
 A renderer would create it entirely differently and there's no way to pass that
methodology via parameters.

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:790: base::PersistentMemoryAllocator*
allocator =
On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> Add a comment about this block, as it's not clear what it's for.

Done.

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> Can the metrics service changes be in a separate CL?
> 
> Correct me if I'm wrong, but it doesn't seem like anything is being hooked up
to
> metrics service here anyway - even if the trial is on, we're not going to get
> AddPersistentMemorySegment() called yet until we have something coming from a
> different source than the browser process. In which case, I suggest including
> those changes with the first CL that starts using this - for example the CL
that
> adds the collection to setup.exe.
> 
> (In which case, probably all the iterator changes can also be part of that
CL.)

It could be moved but it seems to be more "in context" here since this whole CL
is about adding persistent support.

[Alexei Svitkine (slow), 2016-01-14 21:53:41]

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

https://codereview.chromium.org/1425533011/diff/390001/base/metrics/histogram...
base/metrics/histogram.cc:151: if (!histogram) {
On 2016/01/14 19:20:25, bcwhite wrote:
> On 2016/01/14 16:43:13, Alexei Svitkine wrote:
> > Prefer an early return if found, so the rest of the code doesn't need to be
> > indented.
> 
> Found doesn't mean it's correct.  There are additional checks at the bottom of
> the method to ensure that the histogram being requested matches the one that
is
> known.
> 
> This existed in previous code, presumably to ensure that the results are
> identical should the same histogram be instantiated via macros at different
> places in the code.

Ok, that's fair. Let's keep as-is then.

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:48: if
(base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
On 2016/01/14 19:20:26, bcwhite wrote:
> On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > You define the feature in base/ but only query it in chrome/browser. I don't
> > think this makes sense.
> > 
> > If we want the feature to only control the browser process' use of
> persistence,
> > let's define it in chrome/browser - e.g. in this file. We can then have a
> > separate feature to control renderers. We should also put Browser in the
name
> > somewhere to make it clear what it controls.
> 
> I want the same parameter to control all.  I just haven't gotten to writing
the
> Renderer part of it.

I thought we weren't certain if we wanted the browser process to use the new
system for its own histograms and we only wanted this feature control to test
out that things are working as expected?

If that's still true, it sounds like we should have separate features.

OTOH, if we do want them to share it, then I suggest making the Feature local to
the base code and have a function in base that sets it up (w/ appropriate
params). I know you mentioned that the renderer use case will use a different
way of initialization the allocator - but that's OK, we can just have a separate
function for that as well. That is, assuming we want the same feature to be used
for both, which I'm not convinced of (given what I mention above).

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
On 2016/01/14 19:20:26, bcwhite wrote:
> On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > Can the metrics service changes be in a separate CL?
> > 
> > Correct me if I'm wrong, but it doesn't seem like anything is being hooked
up
> to
> > metrics service here anyway - even if the trial is on, we're not going to
get
> > AddPersistentMemorySegment() called yet until we have something coming from
a
> > different source than the browser process. In which case, I suggest
including
> > those changes with the first CL that starts using this - for example the CL
> that
> > adds the collection to setup.exe.
> > 
> > (In which case, probably all the iterator changes can also be part of that
> CL.)
> 
> It could be moved but it seems to be more "in context" here since this whole
CL
> is about adding persistent support.

I disagree. Given that this CL is not adding any external persistent histograms,
I think it's cleaner to have this set of changes with the CL that does.

[bcwhite, 2016-01-15 15:24:38]

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:48: if
(base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
> I thought we weren't certain if we wanted the browser process to use the new
> system for its own histograms and we only wanted this feature control to test
> out that things are working as expected?

There's no persistence on the Browser side but moving the histograms into a
persistent memory segment (even one not persisted) will give immediate feedback
on operation and performance without depending on all the sharing code.


> If that's still true, it sounds like we should have separate features.

I don't see that as the case.  Like any large feature, testers just get more of
it as development continues.


> OTOH, if we do want them to share it, then I suggest making the Feature local
to
> the base code and have a function in base that sets it up (w/ appropriate
> params). I know you mentioned that the renderer use case will use a different
> way of initialization the allocator - but that's OK, we can just have a
separate
> function for that as well. That is, assuming we want the same feature to be
used
> for both, which I'm not convinced of (given what I mention above).

My point is that common code simply doesn't know enough.  Which persistent
allocator is created?  Where does the underlying memory segment come from?  Or
was it an underlying file?  You'd end up with a half-dozen methods all with
about 3 lines of code with each being distinct from the other.  And then the
caller would still have do almost all the same work in order to get the pieces
together.

I'll keep it in mind, though.  If it becomes evident at a later time how to make
much of it common, I'll move it then.

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
On 2016/01/14 21:53:41, asvitkine (OOO until Jan18) wrote:
> On 2016/01/14 19:20:26, bcwhite wrote:
> > On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > > Can the metrics service changes be in a separate CL?
> > > 
> > > Correct me if I'm wrong, but it doesn't seem like anything is being hooked
> up
> > to
> > > metrics service here anyway - even if the trial is on, we're not going to
> get
> > > AddPersistentMemorySegment() called yet until we have something coming
from
> a
> > > different source than the browser process. In which case, I suggest
> including
> > > those changes with the first CL that starts using this - for example the
CL
> > that
> > > adds the collection to setup.exe.
> > > 
> > > (In which case, probably all the iterator changes can also be part of that
> > CL.)
> > 
> > It could be moved but it seems to be more "in context" here since this whole
> CL
> > is about adding persistent support.
> 
> I disagree. Given that this CL is not adding any external persistent
histograms,
> I think it's cleaner to have this set of changes with the CL that does.

I understand the point but pushing this to the setup-metrics CL is going to
complicate the CL between it and this one (the "merge histograms" CL) since it
expands on this somewhat.  It's also going to cost me a day of time to do yet
another split and fix my CL chain in git.

[Alexei Svitkine (slow), 2016-01-15 15:43:47]

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
On 2016/01/15 15:24:38, bcwhite wrote:
> On 2016/01/14 21:53:41, asvitkine (OOO until Jan18) wrote:
> > On 2016/01/14 19:20:26, bcwhite wrote:
> > > On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > > > Can the metrics service changes be in a separate CL?
> > > > 
> > > > Correct me if I'm wrong, but it doesn't seem like anything is being
hooked
> > up
> > > to
> > > > metrics service here anyway - even if the trial is on, we're not going
to
> > get
> > > > AddPersistentMemorySegment() called yet until we have something coming
> from
> > a
> > > > different source than the browser process. In which case, I suggest
> > including
> > > > those changes with the first CL that starts using this - for example the
> CL
> > > that
> > > > adds the collection to setup.exe.
> > > > 
> > > > (In which case, probably all the iterator changes can also be part of
that
> > > CL.)
> > > 
> > > It could be moved but it seems to be more "in context" here since this
whole
> > CL
> > > is about adding persistent support.
> > 
> > I disagree. Given that this CL is not adding any external persistent
> histograms,
> > I think it's cleaner to have this set of changes with the CL that does.
> 
> I understand the point but pushing this to the setup-metrics CL is going to
> complicate the CL between it and this one (the "merge histograms" CL) since it
> expands on this somewhat.  It's also going to cost me a day of time to do yet
> another split and fix my CL chain in git.

APII'm not fully sold on the iterator changes and would like to not have that
discussion block landing this CL, so would like these changes to move to a
follow-up CL - preferably the one that also adds external persistent metrics so
I can reviewer the new API in the same CL that it's used. I'm hoping we can
agree on the remaining parts on this CL and land it early next week and start
experimenting - while in parallel discussing this other part on a separate CL.

[bcwhite, 2016-01-15 19:22:15]

Patchset #20 (id:430001) has been deleted

[bcwhite, 2016-01-15 19:23:41]

Looks like the try-bots might include dependent CLs.  Will likely have compile
errors on other platforms to fix.

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/390001/components/metrics/met...
components/metrics/metrics_service.cc:1157:
histogram_snapshot_manager_.PrepareDeltas(
On 2016/01/15 15:43:47, asvitkine (OOO until Jan18) wrote:
> On 2016/01/15 15:24:38, bcwhite wrote:
> > On 2016/01/14 21:53:41, asvitkine (OOO until Jan18) wrote:
> > > On 2016/01/14 19:20:26, bcwhite wrote:
> > > > On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > > > > Can the metrics service changes be in a separate CL?
> > > > > 
> > > > > Correct me if I'm wrong, but it doesn't seem like anything is being
> hooked
> > > up
> > > > to
> > > > > metrics service here anyway - even if the trial is on, we're not going
> to
> > > get
> > > > > AddPersistentMemorySegment() called yet until we have something coming
> > from
> > > a
> > > > > different source than the browser process. In which case, I suggest
> > > including
> > > > > those changes with the first CL that starts using this - for example
the
> > CL
> > > > that
> > > > > adds the collection to setup.exe.
> > > > > 
> > > > > (In which case, probably all the iterator changes can also be part of
> that
> > > > CL.)
> > > > 
> > > > It could be moved but it seems to be more "in context" here since this
> whole
> > > CL
> > > > is about adding persistent support.
> > > 
> > > I disagree. Given that this CL is not adding any external persistent
> > histograms,
> > > I think it's cleaner to have this set of changes with the CL that does.
> > 
> > I understand the point but pushing this to the setup-metrics CL is going to
> > complicate the CL between it and this one (the "merge histograms" CL) since
it
> > expands on this somewhat.  It's also going to cost me a day of time to do
yet
> > another split and fix my CL chain in git.
> 
> APII'm not fully sold on the iterator changes and would like to not have that
> discussion block landing this CL, so would like these changes to move to a
> follow-up CL - preferably the one that also adds external persistent metrics
so
> I can reviewer the new API in the same CL that it's used. I'm hoping we can
> agree on the remaining parts on this CL and land it early next week and start
> experimenting - while in parallel discussing this other part on a separate CL.

Done.

[bcwhite, 2016-01-15 20:50:55]

Patchset #21 (id:470001) has been deleted

[Alexei Svitkine (slow), 2016-01-18 18:51:25]

You have some build errors - looks like you forgot to update some files, e.g.:

components/cronet/histogram_manager.cc

[Alexei Svitkine (slow), 2016-01-18 19:25:36]

Looks really good - just a few more nits and then I think it's good to go.

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent histogram
data was not valid; skipped.";
On 2016/01/13 13:38:29, bcwhite wrote:
> On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> > Nit: You can pipe the message into the NOTREACHED().
> > 
> > Same for the other places below.
> 
> But that would omit the warning on Release builds, would it not?  Would you
> prefer the message not appear on Release builds?

I think it's best practice to have release builds not output messages as it
bloats the binary - and these things are not useful to end users. If you want
monitoring from the wild, then it's better to add an enum histogram with an enum
for each error case.

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:58: }
On 2016/01/14 19:20:26, bcwhite wrote:
> On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > Can you move this new code to a helper function?
> 
> I looked at that and it would have to be a helper function defined in this
same
> file because this is the only place that knows what type of allocator to
create.
>  A renderer would create it entirely differently and there's no way to pass
that
> methodology via parameters.

Okay, please at least make it an anon function at the top of the file, so that
InstantiateDynamicTrials() doesn't have this logic and just calls it. (Otherwise
if each trial will clutter this function with its logic, it gets out of hand.)

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_delta_serialization.h (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_delta_serialization.h:31: // If |serialized_deltas| is
NULL, no data is serialized, though the next call
Nit: "is NULL" -> "is null" per new conventions since you're updating the
comment.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:102: std::string
name(histogram_data_ptr->name);
Nit: move this closer to where it's used - i.e. above the switch. Or just inline
histogram_data_ptr->name into the calls in the switch.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:126: if (bad_ranges || ranges->checksum()
!= histogram_data.ranges_checksum) {
Nit: Can you make a helper CreateRanges() in the anon namespace that returns
null if the ranges are bad. Then, the code in this function will be simplified.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:249: if (allocator) {
Do an early return instead, since in this case, there's nothing to be done if
its null.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:251: CHECK(bucket_count <=
std::numeric_limits<int32_t>::max() /
DCHECK_LE?

Or if there's a reason it should be a CHECK, add a comment. If it's temporary
during development, add a TODO to convert to a DCHECK once the code is stable.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:253: size_t counts_memory = bucket_count *
sizeof(HistogramBase::AtomicCount);
Nit: counts_size

ranges_size

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:260:
allocator->Allocate(offsetof(PersistentHistogramData, name) +
Can you add a static_assert somewhere that checks sizeof and offsetof to ensure
the name is the last member?

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:321: HistogramBase* h =
GetNextPersistentHistogram(g_allocator, &iter);
Nit: histogram

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:74: };
Nit: DISALLOW_COPY_AND_ASSIGN() on the test.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/statistic...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/statistic...
base/metrics/statistics_recorder.h:35: typedef std::map<uint64_t,
HistogramBase*> HistogramMap;
Nit: Move this below the public: block.

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
components/metrics/metrics_service.cc:1118:
base::StatisticsRecorder::begin(true), base::StatisticsRecorder::end(),
For all the calls to begin(), can you add a comment above explaining reasoning
for the true/false param, so it's obvious from the call site.

Particularly also the one in histogram_delta_serialization.cc.

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
File components/metrics/metrics_service_accessor.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
components/metrics/metrics_service_accessor.cc:19: // Testing Metrics -- DO NOT
COMMIT
Remove. This is also responsible for a few red bots.

[bcwhite, 2016-01-20 15:26:15]

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent histogram
data was not valid; skipped.";
On 2016/01/18 19:25:35, Alexei Svitkine (slow) wrote:
> On 2016/01/13 13:38:29, bcwhite wrote:
> > On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> > > Nit: You can pipe the message into the NOTREACHED().
> > > 
> > > Same for the other places below.
> > 
> > But that would omit the warning on Release builds, would it not?  Would you
> > prefer the message not appear on Release builds?
> 
> I think it's best practice to have release builds not output messages as it
> bloats the binary - and these things are not useful to end users. If you want
> monitoring from the wild, then it's better to add an enum histogram with an
enum
> for each error case.

Should that histogram have only the errors or should it also include a "success"
entry?

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

https://codereview.chromium.org/1425533011/diff/390001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:58: }
On 2016/01/18 19:25:35, Alexei Svitkine (slow) wrote:
> On 2016/01/14 19:20:26, bcwhite wrote:
> > On 2016/01/14 16:43:14, Alexei Svitkine wrote:
> > > Can you move this new code to a helper function?
> > 
> > I looked at that and it would have to be a helper function defined in this
> same
> > file because this is the only place that knows what type of allocator to
> create.
> >  A renderer would create it entirely differently and there's no way to pass
> that
> > methodology via parameters.
> 
> Okay, please at least make it an anon function at the top of the file, so that
> InstantiateDynamicTrials() doesn't have this logic and just calls it.
(Otherwise
> if each trial will clutter this function with its logic, it gets out of hand.)

Done.

[Alexei Svitkine (slow), 2016-01-20 16:26:41]

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent histogram
data was not valid; skipped.";
On 2016/01/20 15:26:14, bcwhite wrote:
> On 2016/01/18 19:25:35, Alexei Svitkine (slow) wrote:
> > On 2016/01/13 13:38:29, bcwhite wrote:
> > > On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> > > > Nit: You can pipe the message into the NOTREACHED().
> > > > 
> > > > Same for the other places below.
> > > 
> > > But that would omit the warning on Release builds, would it not?  Would
you
> > > prefer the message not appear on Release builds?
> > 
> > I think it's best practice to have release builds not output messages as it
> > bloats the binary - and these things are not useful to end users. If you
want
> > monitoring from the wild, then it's better to add an enum histogram with an
> enum
> > for each error case.
> 
> Should that histogram have only the errors or should it also include a
"success"
> entry?

Yep, that's usually the best practice.

[bcwhite, 2016-01-21 16:56:13]

On 2016/01/20 16:26:41, Alexei Svitkine (very slow) wrote:
>
https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
> File base/metrics/histogram_persistence.cc (right):
> 
>
https://codereview.chromium.org/1425533011/diff/370001/base/metrics/histogram...
> base/metrics/histogram_persistence.cc:90: LOG(WARNING) << "Persistent
histogram
> data was not valid; skipped.";
> On 2016/01/20 15:26:14, bcwhite wrote:
> > On 2016/01/18 19:25:35, Alexei Svitkine (slow) wrote:
> > > On 2016/01/13 13:38:29, bcwhite wrote:
> > > > On 2016/01/12 21:54:44, Alexei Svitkine wrote:
> > > > > Nit: You can pipe the message into the NOTREACHED().
> > > > > 
> > > > > Same for the other places below.
> > > > 
> > > > But that would omit the warning on Release builds, would it not?  Would
> you
> > > > prefer the message not appear on Release builds?
> > > 
> > > I think it's best practice to have release builds not output messages as
it
> > > bloats the binary - and these things are not useful to end users. If you
> want
> > > monitoring from the wild, then it's better to add an enum histogram with
an
> > enum
> > > for each error case.
> > 
> > Should that histogram have only the errors or should it also include a
> "success"
> > entry?
> 
> Yep, that's usually the best practice.

Done.  It's complicated to create histograms within histogram code.  Please
double-check.

[Alexei Svitkine (slow), 2016-01-21 17:31:05]

Thanks - I still need to look at the histogram code a bit more closely, but I
noticed there were a few comments from my last review that you've not addressed
yet.

[bcwhite, 2016-01-22 15:24:53]

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_delta_serialization.h (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_delta_serialization.h:31: // If |serialized_deltas| is
NULL, no data is serialized, though the next call
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: "is NULL" -> "is null" per new conventions since you're updating the
> comment.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:102: std::string
name(histogram_data_ptr->name);
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: move this closer to where it's used - i.e. above the switch. Or just
inline
> histogram_data_ptr->name into the calls in the switch.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:126: if (bad_ranges || ranges->checksum()
!= histogram_data.ranges_checksum) {
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: Can you make a helper CreateRanges() in the anon namespace that returns
> null if the ranges are bad. Then, the code in this function will be
simplified.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:249: if (allocator) {
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Do an early return instead, since in this case, there's nothing to be done if
> its null.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:251: CHECK(bucket_count <=
std::numeric_limits<int32_t>::max() /
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> DCHECK_LE?
> 
> Or if there's a reason it should be a CHECK, add a comment. If it's temporary
> during development, add a TODO to convert to a DCHECK once the code is stable.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:253: size_t counts_memory = bucket_count *
sizeof(HistogramBase::AtomicCount);
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: counts_size
> 
> ranges_size

Did _bytes instead.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:260:
allocator->Allocate(offsetof(PersistentHistogramData, name) +
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Can you add a static_assert somewhere that checks sizeof and offsetof to
ensure
> the name is the last member?

Not accurately because the compiler will add padding to the structure, the exact
size of which depends on all the other fields of the structure.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:321: HistogramBase* h =
GetNextPersistentHistogram(g_allocator, &iter);
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: histogram

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:74: };
On 2016/01/18 19:25:35, Alexei Svitkine (very slow) wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN() on the test.

Done.

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/statistic...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1425533011/diff/490001/base/metrics/statistic...
base/metrics/statistics_recorder.h:35: typedef std::map<uint64_t,
HistogramBase*> HistogramMap;
On 2016/01/18 19:25:36, Alexei Svitkine (very slow) wrote:
> Nit: Move this below the public: block.

Done.

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
components/metrics/metrics_service.cc:1118:
base::StatisticsRecorder::begin(true), base::StatisticsRecorder::end(),
On 2016/01/18 19:25:36, Alexei Svitkine (very slow) wrote:
> For all the calls to begin(), can you add a comment above explaining reasoning
> for the true/false param, so it's obvious from the call site.
> 
> Particularly also the one in histogram_delta_serialization.cc.

Done.

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
File components/metrics/metrics_service_accessor.cc (right):

https://codereview.chromium.org/1425533011/diff/490001/components/metrics/met...
components/metrics/metrics_service_accessor.cc:19: // Testing Metrics -- DO NOT
COMMIT
On 2016/01/18 19:25:36, Alexei Svitkine (very slow) wrote:
> Remove. This is also responsible for a few red bots.

Done.

[Alexei Svitkine (slow), 2016-01-22 16:15:16]

Please update histograms.xml with the new histogram you're adding.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:88: static base::subtle::AtomicWord
atomic_histogram_pointer = 0;         \
Remove \

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:94: // they'll always return the same
result; there is a mutex in the Get.
Nit: "same result; there is" -> "same result as there is"

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:102: "CreatePersistentHistogram.Result",
Change this to UMA.CreatePersistanteHistogram.Result

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:104: HistogramBase::kNoFlags);
kUmaTargetedHistogramFlag

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:260:
result_histogram->Add(CREATE_HISTOGRAM_INVALID_METADATA);
Nit: Instead of having this pattern with the if statement, how about making a
helper function that just takes the enum. The function then calls the getter for
the histogram, checks if its not null, and calls Add().

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.h:17: enum CreateHistogramResultType {
Move this to the .cc file in the anon namespace.

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

https://codereview.chromium.org/1425533011/diff/550001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:29: void
InstantiatePersistentHistograms() {
Nit: Add a comment.

https://codereview.chromium.org/1425533011/diff/550001/chrome/service/service...
File chrome/service/service_ipc_server.cc (right):

https://codereview.chromium.org/1425533011/diff/550001/chrome/service/service...
chrome/service/service_ipc_server.cc:126: // "false" to
SPerpareAndSerializeDeltas() indicates to *not* include
Nit: SPerpareAndSerializeDeltas() -> PerpareAndSerializeDeltas()

[bcwhite, 2016-01-22 17:17:36]

Patchset #25 (id:570001) has been deleted

[bcwhite, 2016-01-22 17:17:48]

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:88: static base::subtle::AtomicWord
atomic_histogram_pointer = 0;         \
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Remove \

Oops.  Thought I got all those.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:94: // they'll always return the same
result; there is a mutex in the Get.
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Nit: "same result; there is" -> "same result as there is"

Done.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:102: "CreatePersistentHistogram.Result",
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Change this to UMA.CreatePersistanteHistogram.Result

Done.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:104: HistogramBase::kNoFlags);
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> kUmaTargetedHistogramFlag

Done.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:260:
result_histogram->Add(CREATE_HISTOGRAM_INVALID_METADATA);
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Nit: Instead of having this pattern with the if statement, how about making a
> helper function that just takes the enum. The function then calls the getter
for
> the histogram, checks if its not null, and calls Add().

Done.

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.h:17: enum CreateHistogramResultType {
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Move this to the .cc file in the anon namespace.

I had it there but given that GetCreateHistogramResultHistogram() is public, it
seemed reasonable that it's contents be public, too.  You sure?

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

https://codereview.chromium.org/1425533011/diff/550001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:29: void
InstantiatePersistentHistograms() {
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/1425533011/diff/550001/chrome/service/service...
File chrome/service/service_ipc_server.cc (right):

https://codereview.chromium.org/1425533011/diff/550001/chrome/service/service...
chrome/service/service_ipc_server.cc:126: // "false" to
SPerpareAndSerializeDeltas() indicates to *not* include
On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> Nit: SPerpareAndSerializeDeltas() -> PerpareAndSerializeDeltas()

Done.

[Alexei Svitkine (slow), 2016-01-22 17:23:40]

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
base/metrics/histogram_persistence.h:17: enum CreateHistogramResultType {
On 2016/01/22 17:17:48, bcwhite wrote:
> On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> > Move this to the .cc file in the anon namespace.
> 
> I had it there but given that GetCreateHistogramResultHistogram() is public,
it
> seemed reasonable that it's contents be public, too.  You sure?

Hmm, but it's only public for tests, right? In that case, I actually suggest
making GetCreateHistogramResultHistogram() private and having a
InitializeCreateHistogramResultHistogramForTesting() function that's public that
is called just from the test.

[bcwhite, 2016-01-22 17:39:48]

On 2016/01/22 17:23:40, Alexei Svitkine (very slow) wrote:
>
https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
> File base/metrics/histogram_persistence.h (right):
> 
>
https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
> base/metrics/histogram_persistence.h:17: enum CreateHistogramResultType {
> On 2016/01/22 17:17:48, bcwhite wrote:
> > On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> > > Move this to the .cc file in the anon namespace.
> > 
> > I had it there but given that GetCreateHistogramResultHistogram() is public,
> it
> > seemed reasonable that it's contents be public, too.  You sure?
> 
> Hmm, but it's only public for tests, right? In that case, I actually suggest
> making GetCreateHistogramResultHistogram() private and having a
> InitializeCreateHistogramResultHistogramForTesting() function that's public
that
> is called just from the test.

I had figured that some tests might want to access the specific buckets in order
to check the operation but as of yet no tests do so.

[bcwhite, 2016-01-22 20:25:30]

Patchset #26 (id:610001) has been deleted

[Alexei Svitkine (slow), 2016-02-01 16:28:09]

On 2016/01/22 17:39:48, bcwhite wrote:
> On 2016/01/22 17:23:40, Alexei Svitkine (very slow) wrote:
> >
>
https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
> > File base/metrics/histogram_persistence.h (right):
> > 
> >
>
https://codereview.chromium.org/1425533011/diff/550001/base/metrics/histogram...
> > base/metrics/histogram_persistence.h:17: enum CreateHistogramResultType {
> > On 2016/01/22 17:17:48, bcwhite wrote:
> > > On 2016/01/22 16:15:16, Alexei Svitkine (very slow) wrote:
> > > > Move this to the .cc file in the anon namespace.
> > > 
> > > I had it there but given that GetCreateHistogramResultHistogram() is
public,
> > it
> > > seemed reasonable that it's contents be public, too.  You sure?
> > 
> > Hmm, but it's only public for tests, right? In that case, I actually suggest
> > making GetCreateHistogramResultHistogram() private and having a
> > InitializeCreateHistogramResultHistogramForTesting() function that's public
> that
> > is called just from the test.
> 
> I had figured that some tests might want to access the specific buckets in
order
> to check the operation but as of yet no tests do so.

Well, given you're not using it in tests right now, I'd make it internal.

Even if you do want to expose it for tests, but only for tests, it should be
inside an "internal" inner namespace, so it's clear that it's not part of the
API regular clients are expected to use.

[Alexei Svitkine (slow), 2016-02-01 16:28:33]

Also, please add the new histogram to histograms.xml per my previous comment.

[bcwhite, 2016-02-01 19:04:15]

On 2016/02/01 16:28:33, Alexei Svitkine (very slow) wrote:
> Also, please add the new histogram to histograms.xml per my previous comment.

Done.

[Alexei Svitkine (slow), 2016-02-01 19:08:40]

lgtm

[bcwhite, 2016-02-01 19:42:11]

Description was changed from

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

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

to

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*, components/cronet/*, and content/child/*
==========

[bcwhite, 2016-02-01 19:43:25]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-02-01 19:45:43]

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

[bcwhite, 2016-02-01 20:42:15]

Description was changed from

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*, components/cronet/*, and content/child/*
==========

to

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*
==========

[commit-bot: I haz the power, 2016-02-01 21:08:06]

Description was changed from

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*
==========

to

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*
==========

[commit-bot: I haz the power, 2016-02-01 21:08:07]

Committed patchset #28 (id:670001)

[commit-bot: I haz the power, 2016-02-01 21:10:28]

Description was changed from

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*
==========

to

==========
Support "shared" histograms between processes.

Histograms have tradionally been held on the local heap and
passed between processes using RPCs or simply lost when that
process stops.  Shared histograms are read directly by another
process.
https://docs.google.com/document/d/1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c...

BUG=546019
TBR=mmenke,nasko,thakis
for base/*.{gn,gypi}, components/cronet/*, and content/child/*

Committed: https://crrev.com/5cb99ebcf2cf1450e7a68e7d7fd3d11ce0549175
Cr-Commit-Position: refs/heads/master@{#372760}
==========

[commit-bot: I haz the power, 2016-02-01 21:10:29]

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