Merge multiple histogram snapshots into single one for reporting.

Merge multiple histogram snapshots into single one for reporting.

With multiple processes each now capable of reporting seperately,
the values need to be merged before being flattened and sent
to the server in order to conserve server space.

BUG=546019
Committed: https://crrev.com/c85a1f82ce2ab8c4ff9c19989f17234ea8abb865
Cr-Commit-Position: refs/heads/master@{#376264}

[bcwhite, 2015-11-30 17:03:36]

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

[bcwhite, 2015-11-30 17:03:36]

Yet another CL.  ;-)
I'm still working on the tests but wanted to see if you had any quick comments
before I get too far along.

[Alexei Svitkine (slow), 2015-12-04 18:30:11]

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.cc:48: SampleInfo& sample_info =
known_histograms_[histogram->name_hash()];
Nit: Make this a pointer, since non-const refs are discouraged.

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.cc:99: ++iter) {
Nit: Can this use C++11 for syntax?

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:44: int inconsistencies;
Nit: inconsistencies_count or num_inconsistencies

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/sample_map...
base/metrics/sample_map.cc:42: for (auto& entry : sample_counts_)
Nit: const auto&

https://codereview.chromium.org/1485763002/diff/20001/components/metrics/metr...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/components/metrics/metr...
components/metrics/metrics_service_unittest.cc:432: STLDeleteElements(&found);
Can HistogramSet have scoped_ptrs as members, so this wouldn't be necessary?
Otherwise, it's too easy to leak using the API.

[bcwhite, 2015-12-08 17:32:18]

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.cc:48: SampleInfo& sample_info =
known_histograms_[histogram->name_hash()];
On 2015/12/04 18:30:11, Alexei Svitkine (slow) wrote:
> Nit: Make this a pointer, since non-const refs are discouraged.

Done.

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.cc:99: ++iter) {
On 2015/12/04 18:30:11, Alexei Svitkine (slow) wrote:
> Nit: Can this use C++11 for syntax?

Done.

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:44: int inconsistencies;
On 2015/12/04 18:30:11, Alexei Svitkine (slow) wrote:
> Nit: inconsistencies_count or num_inconsistencies

It's not a count but a set of flags.  I'll add to the comment.

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/base/metrics/sample_map...
base/metrics/sample_map.cc:42: for (auto& entry : sample_counts_)
On 2015/12/04 18:30:11, Alexei Svitkine (slow) wrote:
> Nit: const auto&

Can't be "const" because it's being changed.

https://codereview.chromium.org/1485763002/diff/20001/components/metrics/metr...
File components/metrics/metrics_service_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/20001/components/metrics/metr...
components/metrics/metrics_service_unittest.cc:432: STLDeleteElements(&found);
On 2015/12/04 18:30:11, Alexei Svitkine (slow) wrote:
> Can HistogramSet have scoped_ptrs as members, so this wouldn't be necessary?
> Otherwise, it's too easy to leak using the API.

Yes, though it makes it impossible do do some things due to the fact that it's
the key.  For example, you can't find() to see if an entry already exists.  This
breaks the existing DCHECK to ensure we don't double-insert a histogram.

HistogramSet is private (well, will be shortly) to MetricsService so I'm not
sure which approach is best here.

[bcwhite, 2016-01-26 21:52:44]

New design that keeps "logged" inside the actual histograms.  This makes it much
easier to keep them associated without error and automatically discard the
logged values when the histogram itself goes away. 

It also simplifies delta accumulation and paves the way for eventual logging
persistent histograms without actually loading them into full objects.

[bcwhite, 2016-01-27 16:46:29]

Patchset #6 (id:100001) has been deleted

[bcwhite, 2016-01-28 22:16:16]

Patchset #7 (id:140001) has been deleted

[bcwhite, 2016-02-09 13:23:26]

On 2016/01/26 21:52:44, bcwhite wrote:
> New design that keeps "logged" inside the actual histograms.  This makes it
much
> easier to keep them associated without error and automatically discard the
> logged values when the histogram itself goes away. 
> 
> It also simplifies delta accumulation and paves the way for eventual logging
> persistent histograms without actually loading them into full objects.

Ping?

[Alexei Svitkine (slow), 2016-02-09 17:28:51]

Will look today - sorry for the delays.

[Alexei Svitkine (slow), 2016-02-09 19:46:52]

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:437: // If nothing has been previously logged, save
this one as logged and
Nit: save this one as |logged_counts_| and

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram.h:233: HistogramBase::AtomicCount* logged,
Nit: logged_counts

Change everywhere.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_base.h:163: // Calculate delta from the previous call to
this method.
I don't think the comment is sufficiently clear. Please expand to make it clear
what it's doing.

"delta from previous call to this method" doesn't tell me much - i.e. delta of
what?

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:226: sizeof(HistogramBase::AtomicCount)) {
Nit: Can you make a variable to store the size and a comment above for the
calculation? Then, the if can be a but more concise.

Or make a helper for it, since you use it on line 364 too.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:231: HistogramBase::AtomicCount*
logged_data =
Add a comment mentioning the second half of the counts data is the logged data.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:14: const int kNewInconsistency =
(int)0x80000000;
Needs a comment.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:35: for (auto iter :
known_histograms) {
Nit: (const auto& entry : known_histogram)

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:90: sample_info->inconsistencies |=
corruption | kNewInconsistency;
Can kNewInconsistency be added to the existing enum? i.e. enum Inconsistency {}
in histogram_base.h?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:104: void
HistogramSnapshotManager::FinishDeltas() {
Please order the function in the .cc file in the same order they're in the .h
file.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:114:
histogram_flattener_->UniqueInconsistencyDetected(
I don't understand why you moved it here rather than using the original logic.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:30: struct SampleInfo {
Add a comment about this struct overall - what's it for / what's it storing?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:38: // for this to be a scoped_ptr<>
because it is only used temporarily
I don't think it's any better to avoid the scoped_ptr<> just to have to explain
it in a comment and have a delete instead of .release(). Just use a scoped_ptr.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:40: HistogramSamples* accumulated;
Nit: accumulated_samples

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:42: // The set of inconsistencies
(flags) already seen for the histogram.
Mention what flags? Where are they defined?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:50: typedef std::map<uint64_t,
SampleInfo> HashInfoMap;
You only use this typedef once, so I'd just not have the typedef and have the
struct be above the map declaration.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:51: 
You keep placing this above the public: section - that's not the right place for
them per the style guide. If they need to be referenced from the public section,
put them in the public section. Otherwise, in the private: section.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:83: // until FinishDeltas() completes.
PrepareOnce() works the same
Nit: I find PrepareOnce() not very clear - how about PrepareWithoutDeltas()?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:88: void PrepareOnce(const
HistogramBase* histogram);
Nit: Make the param a const ref, which is best practice for const params unless
they're allowed to be null.

https://codereview.chromium.org/1485763002/diff/200001/components/metrics/met...
File components/metrics/metrics_service.cc (left):

https://codereview.chromium.org/1485763002/diff/200001/components/metrics/met...
components/metrics/metrics_service.cc:1110: // "true" indicates that
StatisticsRecorder should include histograms in
Why is this being removed?

[bcwhite, 2016-02-11 16:42:39]

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram.cc:437: // If nothing has been previously logged, save
this one as logged and
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: save this one as |logged_counts_| and

Done.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram.h:233: HistogramBase::AtomicCount* logged,
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: logged_counts
> 
> Change everywhere.

Done.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_base.h:163: // Calculate delta from the previous call to
this method.
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> I don't think the comment is sufficiently clear. Please expand to make it
clear
> what it's doing.
> 
> "delta from previous call to this method" doesn't tell me much - i.e. delta of
> what?

Done.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:226: sizeof(HistogramBase::AtomicCount)) {
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: Can you make a variable to store the size and a comment above for the
> calculation? Then, the if can be a but more concise.
> 
> Or make a helper for it, since you use it on line 364 too.

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:231: HistogramBase::AtomicCount*
logged_data =
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Add a comment mentioning the second half of the counts data is the logged
data.

Done.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:14: const int kNewInconsistency =
(int)0x80000000;
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Needs a comment. 

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:35: for (auto iter :
known_histograms) {
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: (const auto& entry : known_histogram)

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:90: sample_info->inconsistencies |=
corruption | kNewInconsistency;
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Can kNewInconsistency be added to the existing enum? i.e. enum Inconsistency
{}
> in histogram_base.h?

I can but it's used only internally here.  It would have no meaning to anything
else using those values.  Is that your preference?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:104: void
HistogramSnapshotManager::FinishDeltas() {
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Please order the function in the .cc file in the same order they're in the .h
> file.

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:114:
histogram_flattener_->UniqueInconsistencyDetected(
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> I don't understand why you moved it here rather than using the original logic.

If done during aggregation, multiple reports could be generated for the same
histogram name if multiple versions exist and multiple inconsistencies exist. 
Done here it will generate only one report for any given histogram name.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:30: struct SampleInfo {
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Add a comment about this struct overall - what's it for / what's it storing?

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:38: // for this to be a scoped_ptr<>
because it is only used temporarily
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> I don't think it's any better to avoid the scoped_ptr<> just to have to
explain
> it in a comment and have a delete instead of .release(). Just use a
scoped_ptr.

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:40: HistogramSamples* accumulated;
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: accumulated_samples

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:42: // The set of inconsistencies
(flags) already seen for the histogram.
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Mention what flags? Where are they defined?

Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:50: typedef std::map<uint64_t,
SampleInfo> HashInfoMap;
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> You only use this typedef once, so I'd just not have the typedef and have the
> struct be above the map declaration.

I guess all other uses became "auto".  Handy, that.  Done.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:51: 
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> You keep placing this above the public: section - that's not the right place
for
> them per the style guide. If they need to be referenced from the public
section,
> put them in the public section. Otherwise, in the private: section.

There is no longer any access to it from public so I've moved it down below.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:83: // until FinishDeltas() completes.
PrepareOnce() works the same
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: I find PrepareOnce() not very clear - how about PrepareWithoutDeltas()?

PrepareAbsolute?

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:88: void PrepareOnce(const
HistogramBase* histogram);
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Nit: Make the param a const ref, which is best practice for const params
unless
> they're allowed to be null.

Since it's an either/or call to these two methods, wouldn't it be better to keep
the parameters the same style?

https://codereview.chromium.org/1485763002/diff/200001/components/metrics/met...
File components/metrics/metrics_service.cc (left):

https://codereview.chromium.org/1485763002/diff/200001/components/metrics/met...
components/metrics/metrics_service.cc:1110: // "true" indicates that
StatisticsRecorder should include histograms in
On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> Why is this being removed?

Done.

[bcwhite, 2016-02-12 02:03:07]

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:38: // for this to be a scoped_ptr<>
because it is only used temporarily
On 2016/02/11 16:42:39, bcwhite wrote:
> On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> > I don't think it's any better to avoid the scoped_ptr<> just to have to
> explain
> > it in a comment and have a delete instead of .release(). Just use a
> scoped_ptr.
> 
> Done.

It turns out that using a scoped_ptr here is a real PITB due to the struct being
used as the value of a std::map.  Some architectures apparently don't yet use
std::move of the value, instead trying to call the copy-ctor which breaks
because you can't do that with a scoped_ptr.  <sigh>

Though I haven't tried it, I assume using a map value of scoped_ptr<SampleInfo>
would fail the same way (in addition to being simply wasteful).

So...  While I agree with you that the scoped_ptr is actually the better way,
I'd like to switch this back to being a raw pointer managed in code with a TODO
to fix it once all architectures support it.

Thoughts?

[bcwhite, 2016-02-12 02:03:19]

Patchset #11 (id:240001) has been deleted

[bcwhite, 2016-02-12 02:03:32]

Patchset #11 (id:260001) has been deleted

[bcwhite, 2016-02-12 14:50:02]

Patchset #12 (id:300001) has been deleted

[bcwhite, 2016-02-13 02:21:12]

Patchset #14 (id:350001) has been deleted

[bcwhite, 2016-02-15 12:53:24]

Nice to see everything green again now that other mini-CLs have been committed. 
PTAL.

I'm going to split the TEST_P change into a different CL and expand upon it.

[Alexei Svitkine (slow), 2016-02-16 16:46:05]

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:90: sample_info->inconsistencies |=
corruption | kNewInconsistency;
On 2016/02/11 16:42:39, bcwhite wrote:
> On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> > Can kNewInconsistency be added to the existing enum? i.e. enum Inconsistency
> {}
> > in histogram_base.h?
> 
> I can but it's used only internally here.  It would have no meaning to
anything
> else using those values.  Is that your preference?

Yeah, I think it's still worth adding to the enum - else someone looking at the
enum will not have the full story and may accidentally add the same value.

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:88: void PrepareOnce(const
HistogramBase* histogram);
On 2016/02/11 16:42:39, bcwhite wrote:
> On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> > Nit: Make the param a const ref, which is best practice for const params
> unless
> > they're allowed to be null.
> 
> Since it's an either/or call to these two methods, wouldn't it be better to
keep
> the parameters the same style?

But it would be different callers to them, right? Not like an if (foo) callA();
else callB(). If it was like that then I agree it would be bad to be
inconsistent in this case - but if it's completely separate callers, I don't see
it being an issue.

[bcwhite, 2016-02-16 18:06:10]

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:88: void PrepareOnce(const
HistogramBase* histogram);
On 2016/02/16 16:46:05, Alexei Svitkine wrote:
> On 2016/02/11 16:42:39, bcwhite wrote:
> > On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> > > Nit: Make the param a const ref, which is best practice for const params
> > unless
> > > they're allowed to be null.
> > 
> > Since it's an either/or call to these two methods, wouldn't it be better to
> keep
> > the parameters the same style?
> 
> But it would be different callers to them, right? Not like an if (foo)
callA();
> else callB(). If it was like that then I agree it would be bad to be
> inconsistent in this case - but if it's completely separate callers, I don't
see
> it being an issue.

Same caller.  FileMetricsProvider calls PrepareDelta() if the file is read/write
and PrepareOnce (PrepareAbsolute?) if it's read-only.

[bcwhite, 2016-02-16 19:56:00]

Change of "inconsistencies" from int to unsigned also done.

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

https://codereview.chromium.org/1485763002/diff/200001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:90: sample_info->inconsistencies |=
corruption | kNewInconsistency;
On 2016/02/16 16:46:05, Alexei Svitkine wrote:
> On 2016/02/11 16:42:39, bcwhite wrote:
> > On 2016/02/09 19:46:51, Alexei Svitkine wrote:
> > > Can kNewInconsistency be added to the existing enum? i.e. enum
Inconsistency
> > {}
> > > in histogram_base.h?
> > 
> > I can but it's used only internally here.  It would have no meaning to
> anything
> > else using those values.  Is that your preference?
> 
> Yeah, I think it's still worth adding to the enum - else someone looking at
the
> enum will not have the full story and may accidentally add the same value.

Done.

[Alexei Svitkine (slow), 2016-02-17 16:21:12]

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram.h:170: // have memory over-writes, or DRAM failures).
Nit: While here, can you point to the enum of flags this is returning in the
comment?

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:113: // "counts".
Add a sentence about why this returns 0. Maybe just move the comment from the
body to here.

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:114: uint32_t RequiredCountsBytes(size_t
bucket_count) {
Nit: Get* or Calculate*

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:120: // perhaps as the result of a
milicious actor, then return zero to
Nit: malicious

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:378: size_t counts_bytes =
RequiredCountsBytes(bucket_count);
Make the function return size_t instead of uint32_t.

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:395: void
StatisticsRecorder::ResetForTesting() {
Please order this same as in the .h file - i.e. above the ctor. Also, can you
move DumpHistogramsToVlog() to below the dtor?

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:410: callbacks_ = nullptr;
A lot of this code is similar to what's done in the dtor.

Can you make a private helper function? I know one difference is whether the
container values are deleted - but you can make a boolean param in the helper
for this.

I would also keep the structure of the dtor and have separate if (histograms_)
STLDelete* stmts for when the boolean specify to delete the contents.

(The reason to consolidate them is to make it less likely for someone update one
but forget the other.)

[bcwhite, 2016-02-17 17:58:21]

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram.h:170: // have memory over-writes, or DRAM failures).
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Nit: While here, can you point to the enum of flags this is returning in the
> comment?

Done.

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:113: // "counts".
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Add a sentence about why this returns 0. Maybe just move the comment from the
> body to here.

Done.

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:114: uint32_t RequiredCountsBytes(size_t
bucket_count) {
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Nit: Get* or Calculate*

Done.

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:120: // perhaps as the result of a
milicious actor, then return zero to
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Nit: malicious

Done.

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:378: size_t counts_bytes =
RequiredCountsBytes(bucket_count);
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Make the function return size_t instead of uint32_t.

Done.

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:395: void
StatisticsRecorder::ResetForTesting() {
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> Please order this same as in the .h file - i.e. above the ctor. Also, can you
> move DumpHistogramsToVlog() to below the dtor?

This is being moved into another CL.

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:410: callbacks_ = nullptr;
On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> A lot of this code is similar to what's done in the dtor.
> 
> Can you make a private helper function? I know one difference is whether the
> container values are deleted - but you can make a boolean param in the helper
> for this.
> 
> I would also keep the structure of the dtor and have separate if (histograms_)
> STLDelete* stmts for when the boolean specify to delete the contents.
> 
> (The reason to consolidate them is to make it less likely for someone update
one
> but forget the other.)

This is being moved into another CL.  It turns out that trying to clear the
histograms from the map can actually cause problems during testing because of
static pointers to histograms created in other code.

[Alexei Svitkine (slow), 2016-02-17 18:15:17]

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:395: void
StatisticsRecorder::ResetForTesting() {
On 2016/02/17 17:58:20, bcwhite wrote:
> On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> > Please order this same as in the .h file - i.e. above the ctor. Also, can
you
> > move DumpHistogramsToVlog() to below the dtor?
> 
> This is being moved into another CL.

So should it be deleted from this CL? Or are you planning to land the other one
first and rebase this one on top?

[bcwhite, 2016-02-17 19:39:50]

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

https://codereview.chromium.org/1485763002/diff/410001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:395: void
StatisticsRecorder::ResetForTesting() {
On 2016/02/17 18:15:16, Alexei Svitkine wrote:
> On 2016/02/17 17:58:20, bcwhite wrote:
> > On 2016/02/17 16:21:12, Alexei Svitkine wrote:
> > > Please order this same as in the .h file - i.e. above the ctor. Also, can
> you
> > > move DumpHistogramsToVlog() to below the dtor?
> > 
> > This is being moved into another CL.
> 
> So should it be deleted from this CL? Or are you planning to land the other
one
> first and rebase this one on top?

The latter.  I think the other one is ready to land now.

[Alexei Svitkine (slow), 2016-02-18 15:47:41]

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:57: const std::string& name)
{
Nit: Fits on previous line?

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:150: const
std::vector<std::string>& histograms =
Nit: No &

The object is returned by value.

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

https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
components/metrics/metrics_service.cc:1114: base::Histogram::kNoFlags,
base::Histogram::kUmaStabilityHistogramFlag);
What's this change for?

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

https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
components/metrics/metrics_service.h:77: typedef std::set<base::HistogramBase*>
HistogramSet;
Doesn't seem like this is used anywhere in this CL?

[bcwhite, 2016-02-18 17:07:05]

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:57: const std::string& name)
{
On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> Nit: Fits on previous line?

Done.

https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:150: const
std::vector<std::string>& histograms =
On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> Nit: No &
> 
> The object is returned by value.

Done.

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

https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
components/metrics/metrics_service.cc:1114: base::Histogram::kNoFlags,
base::Histogram::kUmaStabilityHistogramFlag);
On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> What's this change for?

Restored.

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

https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
components/metrics/metrics_service.h:77: typedef std::set<base::HistogramBase*>
HistogramSet;
On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> Doesn't seem like this is used anywhere in this CL?

Removed.

[Alexei Svitkine (slow), 2016-02-18 17:43:46]

Greg has a comment about not using unsigned on the other CL. Let's make
that change here too - i.e. change the flags type to uint32_t.

On Thu, Feb 18, 2016 at 12:07 PM, <bcwhite@chromium.org> wrote:

>
>
https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
> File base/metrics/histogram_snapshot_manager_unittest.cc (right):
>
https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
> base/metrics/histogram_snapshot_manager_unittest.cc:57
<https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...>:
const
> std::string& name) {
> On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> > Nit: Fits on previous line?
>
> Done.
>
https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...
> base/metrics/histogram_snapshot_manager_unittest.cc:150
<https://codereview.chromium.org/1485763002/diff/450001/base/metrics/histogram...>:
const
> std::vector<std::string>& histograms =
> On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> > Nit: No &
> >
> > The object is returned by value.
>
> Done.
>
https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
> File components/metrics/metrics_service.cc (right):
>
https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
> components/metrics/metrics_service.cc:1114
<https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...>:
base::Histogram::kNoFlags,
> base::Histogram::kUmaStabilityHistogramFlag);
> On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> > What's this change for?
>
> Restored.
>
https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
> File components/metrics/metrics_service.h (right):
>
https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...
> components/metrics/metrics_service.h:77
<https://codereview.chromium.org/1485763002/diff/450001/components/metrics/met...>:
typedef
> std::set<base::HistogramBase*> HistogramSet;
> On 2016/02/18 15:47:40, Alexei Svitkine wrote:
> > Doesn't seem like this is used anywhere in this CL?
>
> Removed.
> https://codereview.chromium.org/1485763002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2016-02-18 17:59:11]

> Greg has a comment about not using unsigned on the other CL. Let's make
> that change here too - i.e. change the flags type to uint32_t.

Done.

[Alexei Svitkine (slow), 2016-02-18 19:06:36]

lgtm % remaining comments

https://codereview.chromium.org/1485763002/diff/470001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/470001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:28:
histogram.histogram_name()));
Nit: Maybe:

ASSERT_FALSE(ContainsKey(recorded_delta_histogram_sum_,
histogram.histogram_name());

Same for line 57.

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

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:101: unsigned corruption =
histogram->FindCorruption(*samples);
uint32_t

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:119: const unsigned old_corruption =
sample_info->inconsistencies;
Ditto.

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

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:95: unsigned inconsistencies;
uint32_t

[bcwhite, 2016-02-18 19:33:55]

https://codereview.chromium.org/1485763002/diff/470001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/1485763002/diff/470001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:28:
histogram.histogram_name()));
On 2016/02/18 19:06:36, Alexei Svitkine wrote:
> Nit: Maybe:
> 
> ASSERT_FALSE(ContainsKey(recorded_delta_histogram_sum_,
> histogram.histogram_name());
> 
> Same for line 57.

Done.

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

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:101: unsigned corruption =
histogram->FindCorruption(*samples);
On 2016/02/18 19:06:36, Alexei Svitkine wrote:
> uint32_t

Done.

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:119: const unsigned old_corruption =
sample_info->inconsistencies;
On 2016/02/18 19:06:36, Alexei Svitkine wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/1485763002/diff/490001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:95: unsigned inconsistencies;
On 2016/02/18 19:06:36, Alexei Svitkine wrote:
> uint32_t

Done.

[bcwhite, 2016-02-18 19:56:54]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-02-18 19:56:54]

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

[commit-bot: I haz the power, 2016-02-18 19:57:58]

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

[commit-bot: I haz the power, 2016-02-18 21:22:23]

Committed patchset #21 (id:510001)

[commit-bot: I haz the power, 2016-02-18 21:23:26]

Description was changed from

==========
Merge multiple histogram snapshots into single one for reporting.

With multiple processes each now capable of reporting seperately,
the values need to be merged before being flattened and sent
to the server in order to conserve server space.

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

to

==========
Merge multiple histogram snapshots into single one for reporting.

With multiple processes each now capable of reporting seperately,
the values need to be merged before being flattened and sent
to the server in order to conserve server space.

BUG=546019

Committed: https://crrev.com/c85a1f82ce2ab8c4ff9c19989f17234ea8abb865
Cr-Commit-Position: refs/heads/master@{#376264}
==========

[commit-bot: I haz the power, 2016-02-18 21:23:27]

Patchset 21 (id:??) landed as
https://crrev.com/c85a1f82ce2ab8c4ff9c19989f17234ea8abb865
Cr-Commit-Position: refs/heads/master@{#376264}