Add support for persistent sparse histograms.

Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)

Committed: https://crrev.com/3dd85c4f5f230f7c1fa1055cb035c72196a46237
Cr-Commit-Position: refs/heads/master@{#381699}

[bcwhite, 2016-02-25 20:56:57]

Patchset #2 (id:20001) has been deleted

[bcwhite, 2016-02-25 21:30:04]

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

[grt (UTC plus 2), 2016-03-02 20:00:43]

overall looks good. i don't know the guts of histograms or the allocator to
provide much commentary beyond style. let me know if you need another reviewer
with a deep understanding. is asvitkine the only one at the moment?

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:175: // 2016-02-25, the availability of such is
controlled by a base::Feature
updating this date repeatedly is cumbersome. wdyt about changing this to
"..allocator, the availability of which is..."

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:125: const unsigned kBytesPerBucket = 2 *
sizeof(HistogramBase::AtomicCount);
sizeof() returns a size_t. please change the type of this to any of: size_t,
int, or a fixed-size int type.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:130: if (bucket_count >
std::numeric_limits<uint32_t>::max() / kBytesPerBucket)
why is this comparing with uint32_t's max when the return type is size_t? this
is inconsistent with the comment, which says that the check is to avoid
overflowing the return type.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:131: return 0;
if this should never happen, and always means that something has gone horribly
wrong (or is being tampered with), then CHECK(false); seems like the right thing
to do. this will crash the process immediately, and send us a dump telling us
why.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:450: // If CalculateRequiredCountsBytes()
returns zero then the bucket_count
nit: move this comment into the "!counts_bytes" block as something like:
      // |bucket_count| is out of range.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:478: strcpy(histogram_data->name,
name.c_str());
nit: use memcpy since you know the size

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:486: histogram_data->bucket_count =
static_cast<uint32_t>(bucket_count);
i think this is safe on account of CalculateRequiredCountsBytes, but it would be
nice to have an explicit range check for bucket_count up on line 452. why does
BucketRanges use size_t?

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:144: if (!count_pointer)
nit: return count_pointer ? *count_pointer : 0;

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:155: for (const auto& entry : sample_counts_) {
nit: omit braces

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:166: return scoped_ptr<SampleCountIterator>(
does this work:
  return make_scoped_ptr(new PersistentSampleMapIterator(sample_counts_));
i don't remember if the implicit upcast is handled in that case or not

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:171: HistogramSamples::Operator op) {
HistogramSamples:: not needed

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:181: (op == HistogramSamples::ADD) ? count : -count;
HistogramSamples:: not needed. unless ADD is too overloaded. huh. maybe leave it
there for the heck of it

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:186: Count*
PersistentSampleMap::GetSampleCountPointer(HistogramBase::Sample value,
omit HistogramBase:: thanks to typedef at top of file (line 235, too)

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:268: if (sample_counts_.find(record->value) ==
sample_counts_.end()) {
#include "base/stl_util.h"
if (!ContainsKey(sample_counts_, record->value))

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:309: if (min != NULL)
if (min)
...

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:318: while (!Done() && *iter_->second == 0) {
nit: omit braces

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:90: HistogramSamples::Operator op) override;
nit: HistogramSamples:: not needed

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:94: HistogramBase::Count*
GetSampleCountPointer(HistogramBase::Sample value,
nit: what do you think of GetSampleCountStorage?

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:98: // Imports samples from persistent memory,
stopping if a particular value
is it correct to say "stopping after importing |until_value|."? "is found"
doesn't make it clear if the value imported or not.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:99: // is found. Pass -1 to import everything. Returns
a pointer to the
nit: add a private kAllSamples constant like this:
    enum : HistogramBase::Sample { kAllSamples = -1 };
and then use that symbol in the doc comment here and in the code.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:100: // count for |until_value| if it is found, NULL
otherwise.
in our post-nullptr world (where we shouldn't use NULL in code), i prefer the
lower-case "null" to the other alternatives ("nullptr" or "NULL"). there was a
chromium-dev thread somewhere about this that landed on it being a personal
style thing, so i won't press you to change it, just letting you know my
preference.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
base/metrics/sparse_histogram.cc:82: const std::string& name,
oh, how lovely it would be if this could be a base::StringPiece. looks like
you'd need to make a string right away when calling the base ctor, so it
probably isn't worth doing it in this one spot. i wonder how many strings would
be saved if base/metrics comprehensively used StringPiece...

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
base/metrics/sparse_histogram.cc:85: return new SparseHistogram(allocator, name,
meta, logged_meta);
since this method unconditionally creates a new instance, i think
CreatePersistent is a more appropriate name.

[bcwhite, 2016-03-02 22:32:42]

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

[bcwhite, 2016-03-02 22:32:44]

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram....
base/metrics/histogram.cc:175: // 2016-02-25, the availability of such is
controlled by a base::Feature
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> updating this date repeatedly is cumbersome. wdyt about changing this to
> "..allocator, the availability of which is..."

I don't plan to update it repeatedly.  I just updated it because I was putting
the same comment elsewhere and it seemed odd to have different dates in both.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
File base/metrics/histogram_persistence.cc (right):

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:125: const unsigned kBytesPerBucket = 2 *
sizeof(HistogramBase::AtomicCount);
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> sizeof() returns a size_t. please change the type of this to any of: size_t,
> int, or a fixed-size int type.

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:130: if (bucket_count >
std::numeric_limits<uint32_t>::max() / kBytesPerBucket)
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> why is this comparing with uint32_t's max when the return type is size_t? this
> is inconsistent with the comment, which says that the check is to avoid
> overflowing the return type.

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:131: return 0;
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> if this should never happen, and always means that something has gone horribly
> wrong (or is being tampered with), then CHECK(false); seems like the right
thing
> to do. this will crash the process immediately, and send us a dump telling us
> why.

This is a tough balance.  On one hand, it should never happen in a
well-functioning system so a crash seems reasonable.  On the other hand, it's
dependent on data outside of the control of this module and thus end up crashing
for things it should not.

For example, a corrupted file from setup.exe could cause a debug build to crash
after one minute of operation as the file gets read.

I used to have more such DCHECK() for things that simply shouldn't happen but
I've been moving away from them on anything that could be "external" input.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:450: // If CalculateRequiredCountsBytes()
returns zero then the bucket_count
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: move this comment into the "!counts_bytes" block as something like:
>       // |bucket_count| is out of range.

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:478: strcpy(histogram_data->name,
name.c_str());
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: use memcpy since you know the size

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/histogram_...
base/metrics/histogram_persistence.cc:486: histogram_data->bucket_count =
static_cast<uint32_t>(bucket_count);
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> i think this is safe on account of CalculateRequiredCountsBytes, but it would
be
> nice to have an explicit range check for bucket_count up on line 452. why does
> BucketRanges use size_t?

It's actually limited because any allocation that exceeds 32-bits will fail and
thus this smaller value must be less than 32-bits in size.  I'll add a comment.

I don't know why BucketRanges uses size_t.  Probably seemed the right thing at
the time.

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:144: if (!count_pointer)
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: return count_pointer ? *count_pointer : 0;

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:155: for (const auto& entry : sample_counts_) {
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: omit braces

Done.  Above, too.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:166: return scoped_ptr<SampleCountIterator>(
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> does this work:
>   return make_scoped_ptr(new PersistentSampleMapIterator(sample_counts_));
> i don't remember if the implicit upcast is handled in that case or not

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:171: HistogramSamples::Operator op) {
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> HistogramSamples:: not needed

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:181: (op == HistogramSamples::ADD) ? count : -count;
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> HistogramSamples:: not needed. unless ADD is too overloaded. huh. maybe leave
it
> there for the heck of it

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:186: Count*
PersistentSampleMap::GetSampleCountPointer(HistogramBase::Sample value,
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> omit HistogramBase:: thanks to typedef at top of file (line 235, too)

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:268: if (sample_counts_.find(record->value) ==
sample_counts_.end()) {
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> #include "base/stl_util.h"
> if (!ContainsKey(sample_counts_, record->value))

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:309: if (min != NULL)
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> if (min)
> ...

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.cc:318: while (!Done() && *iter_->second == 0) {
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: omit braces

Done.

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

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:90: HistogramSamples::Operator op) override;
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: HistogramSamples:: not needed

Done.  Above, too.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:94: HistogramBase::Count*
GetSampleCountPointer(HistogramBase::Sample value,
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: what do you think of GetSampleCountStorage?

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:98: // Imports samples from persistent memory,
stopping if a particular value
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> is it correct to say "stopping after importing |until_value|."? "is found"
> doesn't make it clear if the value imported or not.

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:99: // is found. Pass -1 to import everything. Returns
a pointer to the
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> nit: add a private kAllSamples constant like this:
>     enum : HistogramBase::Sample { kAllSamples = -1 };
> and then use that symbol in the doc comment here and in the code.

Done.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sample_map...
base/metrics/sample_map.h:100: // count for |until_value| if it is found, NULL
otherwise.
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> in our post-nullptr world (where we shouldn't use NULL in code), i prefer the
> lower-case "null" to the other alternatives ("nullptr" or "NULL"). there was a
> chromium-dev thread somewhere about this that landed on it being a personal
> style thing, so i won't press you to change it, just letting you know my
> preference.

Acknowledged.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
base/metrics/sparse_histogram.cc:82: const std::string& name,
On 2016/03/02 20:00:43, grt (no reviews Mar 2-4) wrote:
> oh, how lovely it would be if this could be a base::StringPiece. looks like
> you'd need to make a string right away when calling the base ctor, so it
> probably isn't worth doing it in this one spot. i wonder how many strings
would
> be saved if base/metrics comprehensively used StringPiece...

HistogramBase internally keeps a std::string of the name.  Is it more efficient
to pass StringPiece around and create a new std::string at that point?  If so,
I'll address that in a future CL.

https://codereview.chromium.org/1734033003/diff/80001/base/metrics/sparse_his...
base/metrics/sparse_histogram.cc:85: return new SparseHistogram(allocator, name,
meta, logged_meta);
On 2016/03/02 20:00:42, grt (no reviews Mar 2-4) wrote:
> since this method unconditionally creates a new instance, i think
> CreatePersistent is a more appropriate name.

I've renamed it to PersistentCreate in
https://codereview.chromium.org/1738063002/

[Alexei Svitkine (slow), 2016-03-03 17:43:38]

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

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:448: }
Everything in this block seems to be a subset of what's done in the "else"
below. If so, I suggest moving this out of the if and having it be done commonly
for both cases. The non-sparse code would need to do more work, but I think it's
fine to do that later. Sure, it does mean that if there's an error we would have
done a bit more work before detecting, but I think it would be fine?

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
File base/metrics/sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.cc:193: // Stop here if no creation of new samples is
desired.
I prefer these to be separate methods. You can have the GetOrCreate version call
the Get* function first which does the above.

Having them be separate functions avoids you needing to document the second
param in calls.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:72: class BASE_EXPORT PersistentSampleMap : public
HistogramSamples {
I would make a separate file for this.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:79: // HistogramSamples implementation:
Nit: // HistogramSamples:

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:114: class BASE_EXPORT PersistentSampleMapIterator :
public SampleCountIterator {
How is this class different than SampleMapIterator?

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

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

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:448: }
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> Everything in this block seems to be a subset of what's done in the "else"
> below. If so, I suggest moving this out of the if and having it be done
commonly
> for both cases. The non-sparse code would need to do more work, but I think
it's
> fine to do that later. Sure, it does mean that if there's an error we would
have
> done a bit more work before detecting, but I think it would be fine?

Sorry, the above meant to say "if there's an error, the non-sparse could would
have to do more work but that should be exceptional case".

[bcwhite, 2016-03-07 15:27:04]

Patchset #7 (id:140001) has been deleted

[bcwhite, 2016-03-07 15:30:39]

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

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:448: }
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> Everything in this block seems to be a subset of what's done in the "else"
> below. If so, I suggest moving this out of the if and having it be done
commonly
> for both cases. The non-sparse code would need to do more work, but I think
it's
> fine to do that later. Sure, it does mean that if there's an error we would
have
> done a bit more work before detecting, but I think it would be fine?

I looked at that but it's not clean.  There would have to be, as I recall, four
separate !=SPARSE conditions within the lower block.  It made the code-flow
not-obvious, prone to error, and difficult to maintain.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
File base/metrics/sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.cc:193: // Stop here if no creation of new samples is
desired.
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> I prefer these to be separate methods. You can have the GetOrCreate version
call
> the Get* function first which does the above.
> 
> Having them be separate functions avoids you needing to document the second
> param in calls.

Done.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:72: class BASE_EXPORT PersistentSampleMap : public
HistogramSamples {
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> I would make a separate file for this.

Done.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:79: // HistogramSamples implementation:
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> Nit: // HistogramSamples:

Done.  Above, too.

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/sample_ma...
base/metrics/sample_map.h:114: class BASE_EXPORT PersistentSampleMapIterator :
public SampleCountIterator {
On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> How is this class different than SampleMapIterator?

It operates on a PersistentSampleMap instead of a SampleMap.  It boils down to a
map to Count* instead of Count and associated code changes.

Deriving it from SampeMapIterator instead of SampleCountIterator wouldn't change
anything; the same set of methods would have to be defined and there would be
duplicated private variables of the same names but slightly different types.

[Alexei Svitkine (slow), 2016-03-07 21:32:31]

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

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:448: }
On 2016/03/07 15:30:39, bcwhite wrote:
> On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> > Everything in this block seems to be a subset of what's done in the "else"
> > below. If so, I suggest moving this out of the if and having it be done
> commonly
> > for both cases. The non-sparse code would need to do more work, but I think
> it's
> > fine to do that later. Sure, it does mean that if there's an error we would
> have
> > done a bit more work before detecting, but I think it would be fine?
> 
> I looked at that but it's not clean.  There would have to be, as I recall,
four
> separate !=SPARSE conditions within the lower block.  It made the code-flow
> not-obvious, prone to error, and difficult to maintain.

Are you sure you can't arrange the code to make those 4 conditions 1 condition?

For example, you just remove the if on line 438. Then, you make line 449 the !=
SPARSE if and then remove things like lines 466-470 and others that have already
been done above. What am I missing?

[bcwhite, 2016-03-07 22:22:07]

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

https://codereview.chromium.org/1734033003/diff/100001/base/metrics/histogram...
base/metrics/histogram_persistence.cc:448: }
On 2016/03/07 21:32:31, Alexei Svitkine (slow) wrote:
> On 2016/03/07 15:30:39, bcwhite wrote:
> > On 2016/03/03 17:43:38, Alexei Svitkine (slow) wrote:
> > > Everything in this block seems to be a subset of what's done in the "else"
> > > below. If so, I suggest moving this out of the if and having it be done
> > commonly
> > > for both cases. The non-sparse code would need to do more work, but I
think
> > it's
> > > fine to do that later. Sure, it does mean that if there's an error we
would
> > have
> > > done a bit more work before detecting, but I think it would be fine?
> > 
> > I looked at that but it's not clean.  There would have to be, as I recall,
> four
> > separate !=SPARSE conditions within the lower block.  It made the code-flow
> > not-obvious, prone to error, and difficult to maintain.
> 
> Are you sure you can't arrange the code to make those 4 conditions 1
condition?
> 
> For example, you just remove the if on line 438. Then, you make line 449 the
!=
> SPARSE if and then remove things like lines 466-470 and others that have
already
> been done above. What am I missing?

I see what you're getting at.  Done.

[Alexei Svitkine (slow), 2016-03-08 19:46:25]

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:128: count_pointer = new Count(0);
Who owns this? Doesn't seem like the map does.

Is this an intentional leak? If so, it should have a comment.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:137: // A race condition could cause two
of the above records to be created. The
If there could be a race - how is this code in general safe? i.e. it uses a
std::map that is not thread safe. Either this code will not work under a race
(i.e. crash w/ std::map from multiple threads) - or there's something else to
prevent the race (i.e. a lock held by the caller?) - if so, is this comment
about a race really true?

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:168: while ((ref =
allocator_->GetNextIterable(&sample_iter_, &type_id)) != 0) {
I think it would be useful to add GetNextIterableOfType() function that takes
the type_id you're looking for.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:186: DCHECK_EQ(0, record->count);  //
Duplicate record should never be used.
Expand comment to mention "see the code in GetOrCreateSampleCountStorage()."
Move it above the DCHECK.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:230: while (!Done() && *iter_->second ==
0)
Nit: For loops, prefer {}'s even for one liners.

Also, restore the ones you removed in the other file.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:55: // Imports samples from persistent
memory, stopping after importing
I don't understand what Import means in this context - without reading the code.
Can you expand the comment to explain this? i.e. that it iterates over all
persistent memory and fills |sample_counts_| map as it does.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:56: // |until_value| or when there is
nothing left to import. Pass kAllSamples
What does "stopping after importing |until_value|" mean? Do you mean
"|until_value| samples" - as in that many samples - or until we get a sample
with that value? If it's the former, can it be named |max_samples|? Also, I'd
just document that passing -1 here explicitly results in all being imported - no
need for the extra constant.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:58: // if it is found, NULL otherwise.
Nit: NULL -> null

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:61: // All created/loaded sample values and
their associated counts.
Expand on ownership.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:71: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
This class shouldn't be used directly by clients, right? Iterator() above
returns the interface.

So, can this be moved entirely to the .cc?

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map_unittest.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map_unittest.cc:214: #if (!defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON)) && GTEST_HAS_DEATH_TEST
Add a comment about this above it.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map_unittest.cc:215: 
No empty line.

[bcwhite, 2016-03-09 01:16:53]

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:128: count_pointer = new Count(0);
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Who owns this? Doesn't seem like the map does.
> 
> Is this an intentional leak? If so, it should have a comment.

It does, just above.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:137: // A race condition could cause two
of the above records to be created. The
> If there could be a race - how is this code in general safe? i.e. it uses a
> std::map that is not thread safe. Either this code will not work under a race
> (i.e. crash w/ std::map from multiple threads) - or there's something else to
> prevent the race (i.e. a lock held by the caller?) - if so, is this comment
> about a race really true?

The race is between processes that share the histogram data, not within a single
process with multiple threads accessing the same histogram object.  I'll update
the comment.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:168: while ((ref =
allocator_->GetNextIterable(&sample_iter_, &type_id)) != 0) {
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> I think it would be useful to add GetNextIterableOfType() function that takes
> the type_id you're looking for.

Perhaps.  I have some ideas for changing the iterator to be a proper object
rather than just a passed-in data structure.  I'll look at adding such then.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:186: DCHECK_EQ(0, record->count);  //
Duplicate record should never be used.
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Expand comment to mention "see the code in GetOrCreateSampleCountStorage()."
> Move it above the DCHECK.

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:230: while (!Done() && *iter_->second ==
0)
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Nit: For loops, prefer {}'s even for one liners.
> 
> Also, restore the ones you removed in the other file.

Greg specifically asked me to remove them.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:55: // Imports samples from persistent
memory, stopping after importing
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> I don't understand what Import means in this context - without reading the
code.
> Can you expand the comment to explain this? i.e. that it iterates over all
> persistent memory and fills |sample_counts_| map as it does.

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:56: // |until_value| or when there is
nothing left to import. Pass kAllSamples
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> What does "stopping after importing |until_value|" mean? Do you mean
> "|until_value| samples" - as in that many samples - or until we get a sample
> with that value? If it's the former, can it be named |max_samples|? Also, I'd
> just document that passing -1 here explicitly results in all being imported -
no
> need for the extra constant.

It's the latter.

I had -1 originally documented in the comment.  Greg specifically asked for the
constant.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:58: // if it is found, NULL otherwise.
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Nit: NULL -> null

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:61: // All created/loaded sample values and
their associated counts.
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Expand on ownership.

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:71: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> This class shouldn't be used directly by clients, right? Iterator() above
> returns the interface.
> 
> So, can this be moved entirely to the .cc?

That works.  Would you like me to do the same to SampleMapIterator while I'm at
it?

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map_unittest.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map_unittest.cc:214: #if (!defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON)) && GTEST_HAS_DEATH_TEST
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> Add a comment about this above it.

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map_unittest.cc:215: 
On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> No empty line.

Done.

[Alexei Svitkine (slow), 2016-03-09 20:35:19]

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:230: while (!Done() && *iter_->second ==
0)
On 2016/03/09 01:16:52, bcwhite wrote:
> On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> > Nit: For loops, prefer {}'s even for one liners.
> > 
> > Also, restore the ones you removed in the other file.
> 
> Greg specifically asked me to remove them.

I think for metrics/ code we actually prefer to keep the {}'s for 1-line loops.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:56: // |until_value| or when there is
nothing left to import. Pass kAllSamples
On 2016/03/09 01:16:52, bcwhite wrote:
> On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> > What does "stopping after importing |until_value|" mean? Do you mean
> > "|until_value| samples" - as in that many samples - or until we get a sample
> > with that value? If it's the former, can it be named |max_samples|? Also,
I'd
> > just document that passing -1 here explicitly results in all being imported
-
> no
> > need for the extra constant.
> 
> It's the latter.
> 
> I had -1 originally documented in the comment.  Greg specifically asked for
the
> constant.

Okay. I guess it doesn't matter too much either way.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:71: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
On 2016/03/09 01:16:52, bcwhite wrote:
> On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> > This class shouldn't be used directly by clients, right? Iterator() above
> > returns the interface.
> > 
> > So, can this be moved entirely to the .cc?
> 
> That works.  Would you like me to do the same to SampleMapIterator while I'm
at
> it?

Yes, please. :)

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

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:18: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
Not sure if you need a BASE_EXPORT here.

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:44: flags |= HistogramBase::kIsPersistent;
Can we move this to inside AllocatePersistentHistogram() so that callers don't
need to set it explicitly?

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:178: // The ID must be different so as to not
confuse sample records.
Why is this the case here but not for other histogram types? Please expand the
comment.

[bcwhite, 2016-03-09 22:57:33]

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:230: while (!Done() && *iter_->second ==
0)
On 2016/03/09 20:35:19, Alexei Svitkine (very slow) wrote:
> On 2016/03/09 01:16:52, bcwhite wrote:
> > On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> > > Nit: For loops, prefer {}'s even for one liners.
> > > 
> > > Also, restore the ones you removed in the other file.
> > 
> > Greg specifically asked me to remove them.
> 
> I think for metrics/ code we actually prefer to keep the {}'s for 1-line
loops.

Done.

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/180001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:71: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
On 2016/03/09 20:35:19, Alexei Svitkine (very slow) wrote:
> On 2016/03/09 01:16:52, bcwhite wrote:
> > On 2016/03/08 19:46:25, Alexei Svitkine (slow) wrote:
> > > This class shouldn't be used directly by clients, right? Iterator() above
> > > returns the interface.
> > > 
> > > So, can this be moved entirely to the .cc?
> > 
> > That works.  Would you like me to do the same to SampleMapIterator while I'm
> at
> > it?
> 
> Yes, please. :)

Done.

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

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:18: class BASE_EXPORT
PersistentSampleMapIterator : public SampleCountIterator {
On 2016/03/09 20:35:19, Alexei Svitkine (very slow) wrote:
> Not sure if you need a BASE_EXPORT here.

I wasn't sure either.  It seems to work without it...  Let's go with "not",
then.

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.cc (right):

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:44: flags |= HistogramBase::kIsPersistent;
On 2016/03/09 20:35:19, Alexei Svitkine (very slow) wrote:
> Can we move this to inside AllocatePersistentHistogram() so that callers don't
> need to set it explicitly?

It would get overwritten below on line 65...  but that's probably wrong because
the flags need to be known by Allocate so they can be properly stored in
persistent memory.

Fixed, here and in histogram.cc.

https://codereview.chromium.org/1734033003/diff/200001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:178: // The ID must be different so as to not
confuse sample records.
On 2016/03/09 20:35:19, Alexei Svitkine (very slow) wrote:
> Why is this the case here but not for other histogram types? Please expand the
> comment.

Done.

[Alexei Svitkine (slow), 2016-03-14 18:49:00]

lgtm

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

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:130: // overridden, be sure to call the "super"
version.
Nit: Remove second sentence, since super version is a no-op.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:200: tentative_histogram->SetFlags(flags_);
Nit: Put this inside HeapAlloc()

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
Nit: No (c)

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:17: // An iterator for going through a
PersistentSampleMap.
Mention that this shares logic with SampleMapIterator and if updating, consider
updating both. Add a similar comment to the other class.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
Nit: No (c)

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:25: class BASE_EXPORT PersistentSampleMap :
public HistogramSamples {
Add a comment. Mention it shares logic for SampleMap.

[bcwhite, 2016-03-15 00:00:21]

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

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:130: // overridden, be sure to call the "super"
version.
On 2016/03/14 18:48:59, Alexei Svitkine (very slow) wrote:
> Nit: Remove second sentence, since super version is a no-op.

It is, but there is no guarantee it will always remain that way, or be true if
it were to be derived from a derived class.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:200: tentative_histogram->SetFlags(flags_);
On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> Nit: Put this inside HeapAlloc()

It would have to be done inside every HeapAlloc and the purpose of those many 
methods is only to allocate and return; no modifications.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
File base/metrics/persistent_sample_map.cc (right):

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> Nit: No (c)

Done.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.cc:17: // An iterator for going through a
PersistentSampleMap.
On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> Mention that this shares logic with SampleMapIterator and if updating,
consider
> updating both. Add a similar comment to the other class.

Done.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
File base/metrics/persistent_sample_map.h (right):

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> Nit: No (c)

Done.

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/persisten...
base/metrics/persistent_sample_map.h:25: class BASE_EXPORT PersistentSampleMap :
public HistogramSamples {
On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> Add a comment. Mention it shares logic for SampleMap.

Done.

[Alexei Svitkine (slow), 2016-03-16 15:25:43]

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

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:200: tentative_histogram->SetFlags(flags_);
On 2016/03/15 00:00:21, bcwhite wrote:
> On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> > Nit: Put this inside HeapAlloc()
> 
> It would have to be done inside every HeapAlloc and the purpose of those many 
> methods is only to allocate and return; no modifications.

To me, it seem a small implementation detail that flags_ is set via a separate
call vs. passed to the ctor. As such, it would be better to abstract it into
HeapAlloc() - since I see that function as allocating the object with the state
specified in member fields (i.e. flags_ is no different than name_ or maximum_).
So I think it's better to have SetFlags() be in HeapAlloc(). And maybe in a
future CL to event make it a ctor param.

[bcwhite, 2016-03-16 15:42:48]

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

https://codereview.chromium.org/1734033003/diff/220001/base/metrics/histogram...
base/metrics/histogram.cc:200: tentative_histogram->SetFlags(flags_);
On 2016/03/16 15:25:43, Alexei Svitkine (very slow) wrote:
> On 2016/03/15 00:00:21, bcwhite wrote:
> > On 2016/03/14 18:49:00, Alexei Svitkine (very slow) wrote:
> > > Nit: Put this inside HeapAlloc()
> > 
> > It would have to be done inside every HeapAlloc and the purpose of those
many 
> > methods is only to allocate and return; no modifications.
> 
> To me, it seem a small implementation detail that flags_ is set via a separate
> call vs. passed to the ctor. As such, it would be better to abstract it into
> HeapAlloc() - since I see that function as allocating the object with the
state
> specified in member fields (i.e. flags_ is no different than name_ or
maximum_).
> So I think it's better to have SetFlags() be in HeapAlloc(). And maybe in a
> future CL to event make it a ctor param.

I was thinking the same: It would be best as a ctor param.  Will do.

[bcwhite, 2016-03-16 21:21:46]

Patchset #12 (id:260001) has been deleted

[bcwhite, 2016-03-16 21:23:00]

Description was changed from

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

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

to

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)
==========

[bcwhite, 2016-03-17 12:47:59]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-03-17 12:48:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1734033003/#ps280001
(title: "rebased")

[commit-bot: I haz the power, 2016-03-17 12:48:08]

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

[commit-bot: I haz the power, 2016-03-17 13:22:04]

Description was changed from

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)
==========

to

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)
==========

[commit-bot: I haz the power, 2016-03-17 13:22:05]

Committed patchset #12 (id:280001)

[commit-bot: I haz the power, 2016-03-17 13:23:14]

Description was changed from

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)
==========

to

==========
Add support for persistent sparse histograms.

Sparse histograms don't have a single "values vector" that
can be created during construction.  Rather, they create a
list of value "records" that can be easily added to as needed.

BUG=546019
TBR=mark
mark: gn and gyp (new files)

Committed: https://crrev.com/3dd85c4f5f230f7c1fa1055cb035c72196a46237
Cr-Commit-Position: refs/heads/master@{#381699}
==========

[commit-bot: I haz the power, 2016-03-17 13:23:15]

Patchset 12 (id:??) landed as
https://crrev.com/3dd85c4f5f230f7c1fa1055cb035c72196a46237
Cr-Commit-Position: refs/heads/master@{#381699}