Reorganize histograms for persistence.

Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)

Committed: https://crrev.com/b036e43221eac3ca8a22d7e96258ca056444cedd
Cr-Commit-Position: refs/heads/master@{#364408}

[bcwhite, 2015-11-24 19:27:01]

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

[bcwhite, 2015-11-24 19:27:02]

Here's the first part.  Needs to be rebased; about_flags has a conflict in the
#include lines.

[bcwhite, 2015-11-24 22:59:34]

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

[bcwhite, 2015-11-24 22:59:34]

Dirk, can you help me out with regard to the .gn files?  There's obviously
something I don't understand.

[Dirk Pranke, 2015-11-24 23:56:37]

On 2015/11/24 22:59:34, bcwhite wrote:
> Dirk, can you help me out with regard to the .gn files?  There's obviously
> something I don't understand.

Which errors specifically are you wondering about?

[Dirk Pranke, 2015-11-25 00:10:18]

On 2015/11/24 23:56:37, Dirk Pranke wrote:
> On 2015/11/24 22:59:34, bcwhite wrote:
> > Dirk, can you help me out with regard to the .gn files?  There's obviously
> > something I don't understand.
> 
> Which errors specifically are you wondering about?

It looks like in moving metrics_hashes.cc to base you've caused the nacl
toolchain
to start building it (since the nacl toolchain depends on base), but the nacl
toolchain
doesn't have arpa/inet.h.

It also looks like sys_byteorder.h doesn't actually need to include either
arpa/inet.h
or winsock.h, so I would just remove those includes.

oddly, nothing else in base itself actually seems to include sys_byteorder.h ,
so 
that's probably why you're just seeing it now.

[bcwhite, 2015-11-25 01:23:36]

> It looks like in moving metrics_hashes.cc to base you've caused the nacl
> toolchain
> to start building it (since the nacl toolchain depends on base), but the nacl
> toolchain
> doesn't have arpa/inet.h.
> 
> It also looks like sys_byteorder.h doesn't actually need to include either
> arpa/inet.h
> or winsock.h, so I would just remove those includes.

No, it doesn't, but it seems that spdy includes that file in order to get access
to htonl() from those headers.

I'm thinking that base shouldn't be referencing those system headers if they're
not universally available and thus it would be okay to remove them and have the
spdy files include the header files directly.  What do you think?

[Dirk Pranke, 2015-11-25 01:26:20]

On 2015/11/25 01:23:36, bcwhite wrote:
> > It looks like in moving metrics_hashes.cc to base you've caused the nacl
> > toolchain
> > to start building it (since the nacl toolchain depends on base), but the
nacl
> > toolchain
> > doesn't have arpa/inet.h.
> > 
> > It also looks like sys_byteorder.h doesn't actually need to include either
> > arpa/inet.h
> > or winsock.h, so I would just remove those includes.
> 
> No, it doesn't, but it seems that spdy includes that file in order to get
access
> to htonl() from those headers.
> 
> I'm thinking that base shouldn't be referencing those system headers if
they're
> not universally available and thus it would be okay to remove them and have
the
> spdy files include the header files directly.  What do you think?

That seems fine to me; if spdy needs arpa/inet.h it should include it directly.

(of course, I'm not a //base owner).

[Alexei Svitkine (slow), 2015-11-25 16:53:54]

Agree with dpranke@ about letting spdy include their own arpa/inet headers
if they need it.

I still feel this patch is a bit too large for my liking. Here's my
suggestion on how it can be split to make the parts easier to review:

First CL:
- Moves the hashing code to base (with all the associated build changes,
include fixes, etc)
- Adds a member to Histogram that's the hash with an accessor
- Makes components metrics code use that accessor to get the histogram hash

Later CLs:
- Changing the base classes to use hashes in their internal maps, etc
- All the other stuff

On Tue, Nov 24, 2015 at 8:26 PM, <dpranke@chromium.org> wrote:

> On 2015/11/25 01:23:36, bcwhite wrote:
>
>> > It looks like in moving metrics_hashes.cc to base you've caused the nacl
>> > toolchain
>> > to start building it (since the nacl toolchain depends on base), but the
>>
> nacl
>
>> > toolchain
>> > doesn't have arpa/inet.h.
>> >
>> > It also looks like sys_byteorder.h doesn't actually need to include
>> either
>> > arpa/inet.h
>> > or winsock.h, so I would just remove those includes.
>>
>
> No, it doesn't, but it seems that spdy includes that file in order to get
>>
> access
>
>> to htonl() from those headers.
>>
>
> I'm thinking that base shouldn't be referencing those system headers if
>>
> they're
>
>> not universally available and thus it would be okay to remove them and
>> have
>>
> the
>
>> spdy files include the header files directly.  What do you think?
>>
>
> That seems fine to me; if spdy needs arpa/inet.h it should include it
> directly.
>
> (of course, I'm not a //base owner).
>
> https://codereview.chromium.org/1471073007/
>

-- 
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.

[mmenke, 2015-11-25 16:54:47]

Description was changed from

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

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

to

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

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

[Alexei Svitkine (slow), 2015-11-25 16:55:30]

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/metrics_ha...
File base/metrics/metrics_hashes.cc (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/metrics_ha...
base/metrics/metrics_hashes.cc:11: namespace metrics {
This needs to move to base namespace.

[bcwhite, 2015-11-25 17:44:19]

> I still feel this patch is a bit too large for my liking. Here's my
> suggestion on how it can be split to make the parts easier to review:

I can but... please, no.  I've got branches nested 4 deep already.

[Alexei Svitkine (slow), 2015-11-25 17:49:49]

It will be much faster to land shorter CLs.

The time to review a CL is not linear to the CL size. If you want to keep the
CLs larger, it will take me disproportionately longer to review them.

[Alexei Svitkine (slow), 2015-11-25 18:05:27]

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_base.h:108: virtual uint64_t id() const = 0;
I think it's better to be explicit about what this is and name it name_hash().
Adding new terminology "id" just obfuscates things.

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.cc:174: #ifdef ARCH_CPU_64_BITS
Can we hide these details in the implementation of Metadata?

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:52: explicit HistogramSamples(uint64 id);
Why does HistogramSamples need an id, as opposed to the Histogram object?

The Histogram object is providing backed memory for the samples right? Can it
simply allocate the backed memory, put an id in there and pass in the remainder
of the buffer?

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:91: // wishes of the derived class.
HistogramSamples is meant to just be an interface. Can this logic live in
subclasses?

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/statistics...
base/metrics/statistics_recorder.h:31: private:
Why is there a private section at the top?

[bcwhite, 2015-11-25 21:36:01]

split out cl/1476633003

[bcwhite, 2015-11-25 21:54:16]

bcwhite@chromium.org changed reviewers:
- dpranke@chromium.org

[bcwhite, 2015-11-25 21:54:17]

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_base.h:108: virtual uint64_t id() const = 0;
On 2015/11/25 18:05:27, Alexei Svitkine (slow) wrote:
> I think it's better to be explicit about what this is and name it name_hash().
> Adding new terminology "id" just obfuscates things.

Done.

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.cc:174: #ifdef ARCH_CPU_64_BITS
On 2015/11/25 18:05:27, Alexei Svitkine (slow) wrote:
> Can we hide these details in the implementation of Metadata?

I'll look at moving the metadata to std::atomic which allows for 64-bit atomics
even on 32-bit systems.  They won't be inter-process safe (i.e. they're not
lock-free) but that's no worse than the simple add we're doing here.

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:52: explicit HistogramSamples(uint64 id);
On 2015/11/25 18:05:27, Alexei Svitkine (slow) wrote:
> Why does HistogramSamples need an id, as opposed to the Histogram object?

The id is shared.  Histograms use shared sample-sets but are not themselves
shared.  All sharing happens within the sample-set.


> The Histogram object is providing backed memory for the samples right? Can it
> simply allocate the backed memory, put an id in there and pass in the
remainder
> of the buffer?

It could, but that would mean the Histogram would have to know about the
internal metadata of the sample set which seems a bad idea.  Better to
explicitly pass in the required value during construction.

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/histogram_...
base/metrics/histogram_samples.h:91: // wishes of the derived class.
On 2015/11/25 18:05:27, Alexei Svitkine (slow) wrote:
> HistogramSamples is meant to just be an interface. Can this logic live in
> subclasses?

Yes, but then it has to be repeated.  It's never been just an interface because
metadata like "redundant_count" and "sum" are present here and partially managed
here.

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1471073007/diff/60001/base/metrics/statistics...
base/metrics/statistics_recorder.h:31: private:
On 2015/11/25 18:05:27, Alexei Svitkine (slow) wrote:
> Why is there a private section at the top?

Paste-o.  Fixed.

[bcwhite, 2015-11-26 20:20:07]

Patchset #7 (id:120001) has been deleted

[Alexei Svitkine (slow), 2015-12-01 16:32:15]

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:305: DCHECK(samples.id() == 0 || samples.id() ==
samples_->id());
This seems to be adding an implicit requirement to the API, which is leaking
knowledge of how this class is used to its implementation. Can this check be
moved to the caller?

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:402: new
SampleVector(HashMetricName(histogram_name()), bucket_ranges()));
Can this use the id from samples_ instead of re-computing it?

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.cc:76: *const_cast<uint64*>(&meta_->id) = id;
This is pretty icky. Can this be restructured to avoid needing a const cast?

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.h:22: struct Metadata {
Per offline discussion, I mentioned that I was going to give some thought about
whether this can be simplified. I finally got around to looking to this part of
the CL, only to find that this isn't being used yet - in the sense that no one
is calling the ctors that take Metadata.

So it's really hard for me to suggest an alternative if I'm not seeing the full
story. How about removing this part of the CL and adding it in the same CL that
actually will call the ctors that need it? Then, we can discuss the approach on
that CL, where the full story is visible.

For example, it's not obvious to me why this can't just copy the values on
construction - rather than keeping pointers around.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.h:94: };
Nit: Add DISALLOW_COPY_AND_ASSIGN() please.

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sample_ma...
base/metrics/sample_map.h:23: explicit SampleMap(uint64 id);
Please switch to uint64_t for the id field throughout.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sample_ve...
File base/metrics/sample_vector.h (right):

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sample_ve...
base/metrics/sample_vector.h:91: const HistogramBase::AtomicCount* counts_;
Can this be "const HistogramBase::AtomicCount* const" counts ? i.e. the pointer
doesn't change after ctor call. Same for bucket_ranges.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sample_ve...
base/metrics/sample_vector.h:92: size_t counts_size_;
Make this const

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:74: new
SampleMap(HashMetricName(histogram_name())));
name_hash()?

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:62: uint64_t id = histogram->name_hash();
Nit: id -> name_hash

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/statistic...
base/metrics/statistics_recorder.h:31: // We keep all registered histograms in a
map, from name to histogram.
Update comment.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/statistic...
base/metrics/statistics_recorder.h:32: typedef std::map<uint64_t,
HistogramBase*> HistogramMap;
Move this typedef to the start of the private: section where the previous
typedef was.

[bcwhite, 2015-12-01 18:12:16]

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:305: DCHECK(samples.id() == 0 || samples.id() ==
samples_->id());
On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> This seems to be adding an implicit requirement to the API, which is leaking
> knowledge of how this class is used to its implementation. Can this check be
> moved to the caller?

I always imagined that "id" (if present, that is non-zero) would be unique or at
least associated.  The code should never try to merge samples from different
samples.

I could move it but I think it should be part of this supporting class.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:402: new
SampleVector(HashMetricName(histogram_name()), bucket_ranges()));
On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> Can this use the id from samples_ instead of re-computing it?

Done.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.cc:76: *const_cast<uint64*>(&meta_->id) = id;
On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> This is pretty icky. Can this be restructured to avoid needing a const cast?

This is a pretty standard way of initializing "const" members.

I can remove the "const" attribute or I can create a constructor for the
metadata and call it explicitly for initialization if you prefer.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.h:22: struct Metadata {
> Per offline discussion, I mentioned that I was going to give some thought
about
> whether this can be simplified. I finally got around to looking to this part
of
> the CL, only to find that this isn't being used yet - in the sense that no one
> is calling the ctors that take Metadata.

No.  I added it for completeness here even though it's won't be used in the next
CL.  I considered leaving it for the other CL but it seemed to fit better in
this one.


> So it's really hard for me to suggest an alternative if I'm not seeing the
full
> story. How about removing this part of the CL and adding it in the same CL
that
> actually will call the ctors that need it? Then, we can discuss the approach
on
> that CL, where the full story is visible.

I think that would make it worse.  If there's a change to be made because of
that then it would mean changes to what is done here.  The only difference
between the two ctors is that one attaches to local storage and the other to
external storage.


> For example, it's not obvious to me why this can't just copy the values on
> construction - rather than keeping pointers around.

Good question.  It's necessary to keep pointers because the external data may
continue to be modified externally should that data be shared between processes.
 We don't currently have distinct plans to do things that way but this code
doesn't know that and our plans may change.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram_samples.h:94: };
On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> Nit: Add DISALLOW_COPY_AND_ASSIGN() please.

Done.

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/sample_ma...
base/metrics/sample_map.h:23: explicit SampleMap(uint64 id);
On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> Please switch to uint64_t for the id field throughout.

Done.

[Alexei Svitkine (slow), 2015-12-01 18:27:14]

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:305: DCHECK(samples.id() == 0 || samples.id() ==
samples_->id());
On 2015/12/01 18:12:15, bcwhite wrote:
> On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> > This seems to be adding an implicit requirement to the API, which is leaking
> > knowledge of how this class is used to its implementation. Can this check be
> > moved to the caller?
> 
> I always imagined that "id" (if present, that is non-zero) would be unique or
at
> least associated.  The code should never try to merge samples from different
> samples.
> 
> I could move it but I think it should be part of this supporting class.

Well, my mental model is this method is just "add some values, where values are
coming from a container |samples|". So, it doesn't pre-suppose that this is used
for merging, for example. In that mental model, it seems the check doesn't make
sense.

If we want to make the method more opinionated, then it probably needs a name to
match and documentation to this factor. But I have a slight preference for
keeping this agnostic of the use case.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:402: new
SampleVector(HashMetricName(histogram_name()), bucket_ranges()));
On 2015/12/01 18:12:15, bcwhite wrote:
> On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> > Can this use the id from samples_ instead of re-computing it?
> 
> Done.

Doesn't seem like you uploaded a new patch set?

[bcwhite, 2015-12-01 19:33:44]

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

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:305: DCHECK(samples.id() == 0 || samples.id() ==
samples_->id());
On 2015/12/01 18:27:14, Alexei Svitkine (slow) wrote:
> On 2015/12/01 18:12:15, bcwhite wrote:
> > On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> > > This seems to be adding an implicit requirement to the API, which is
leaking
> > > knowledge of how this class is used to its implementation. Can this check
be
> > > moved to the caller?
> > 
> > I always imagined that "id" (if present, that is non-zero) would be unique
or
> at
> > least associated.  The code should never try to merge samples from different
> > samples.
> > 
> > I could move it but I think it should be part of this supporting class.
> 
> Well, my mental model is this method is just "add some values, where values
are
> coming from a container |samples|". So, it doesn't pre-suppose that this is
used
> for merging, for example. In that mental model, it seems the check doesn't
make
> sense.
> 
> If we want to make the method more opinionated, then it probably needs a name
to
> match and documentation to this factor. But I have a slight preference for
> keeping this agnostic of the use case.

Okay, I can move the DCHECK no problem.

https://codereview.chromium.org/1471073007/diff/140001/base/metrics/histogram...
base/metrics/histogram.cc:402: new
SampleVector(HashMetricName(histogram_name()), bucket_ranges()));
On 2015/12/01 18:27:14, Alexei Svitkine (slow) wrote:
> On 2015/12/01 18:12:15, bcwhite wrote:
> > On 2015/12/01 16:32:14, Alexei Svitkine (slow) wrote:
> > > Can this use the id from samples_ instead of re-computing it?
> > 
> > Done.
> 
> Doesn't seem like you uploaded a new patch set?

Not yet.  So far all the changes have been cosmetic.

[Alexei Svitkine (slow), 2015-12-04 18:21:00]

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.cc:137: #endif
As we discussed offline some time back, let's keep these non-atomic for now and
you can add a TODO to address later.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.h:93: Metadata* meta_;
Per offline discussion, can we just make this object unconditionally take a ptr
to Metadata? Then, this object doesn't need to set meta->id at all, it can all
be managed by the owner.

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:44: const std::string& histogram_name
= histogram.histogram_name();
Inline this into the DLOG(ERROR) since that's the only thing that uses it now.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:45: const uint64_t histogram_id =
histogram.name_hash();
Nit: Move this closer to where it's first used - i.e. right above the if
(corruption)

Also, rename to histogram_hash.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:80:
logged_samples_.find(histogram_id);
nit: auto it =

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:53: // List of histograms found to be
corrupt, and their problems.
Please update the comments for these two maps to make it clear what the map key
is.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/sample_ve...
File base/metrics/sample_vector.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/sample_ve...
base/metrics/sample_vector.h:26: SampleVector(uint64 id, const BucketRanges*
bucket_ranges);
uint64_t, fix throughout

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:62: uint64_t id = histogram->name_hash();
Nit: name_hash

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/statistic...
base/metrics/statistics_recorder.h:32: typedef std::map<uint64_t,
HistogramBase*> HistogramMap;
This should be in the private: section below. Also, update the comment please.

[bcwhite, 2015-12-04 19:34:47]

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.cc:137: #endif
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> As we discussed offline some time back, let's keep these non-atomic for now
and
> you can add a TODO to address later.

Oops.  I did it, but did it on the wrong CL.  <sigh>

Anyway, done.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.h:93: Metadata* meta_;
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> Per offline discussion, can we just make this object unconditionally take a
ptr
> to Metadata? Then, this object doesn't need to set meta->id at all, it can all
> be managed by the owner.

Feels to me like the wrong thing to do but I can.  You sure?

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:44: const std::string& histogram_name
= histogram.histogram_name();
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> Inline this into the DLOG(ERROR) since that's the only thing that uses it now.

Done.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:45: const uint64_t histogram_id =
histogram.name_hash();
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> Nit: Move this closer to where it's first used - i.e. right above the if
> (corruption)
> 
> Also, rename to histogram_hash.

Done.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:80:
logged_samples_.find(histogram_id);
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> nit: auto it =

Done.

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:53: // List of histograms found to be
corrupt, and their problems.
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> Please update the comments for these two maps to make it clear what the map
key
> is.

Done.

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/sample_ve...
File base/metrics/sample_vector.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/sample_ve...
base/metrics/sample_vector.h:26: SampleVector(uint64 id, const BucketRanges*
bucket_ranges);
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> uint64_t, fix throughout

Done.

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/statistic...
base/metrics/statistics_recorder.cc:62: uint64_t id = histogram->name_hash();
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> Nit: name_hash

Done.

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

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/statistic...
base/metrics/statistics_recorder.h:32: typedef std::map<uint64_t,
HistogramBase*> HistogramMap;
On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> This should be in the private: section below. Also, update the comment please.

Done.  It actually has to be up top so as to be previously defined when it is
used by something in the "public" section... but that "something" got moved into
the other CL so this will have to migrate over there.

[bcwhite, 2015-12-05 18:12:37]

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.h:93: Metadata* meta_;
On 2015/12/04 19:34:46, bcwhite wrote:
> On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> > Per offline discussion, can we just make this object unconditionally take a
> ptr
> > to Metadata? Then, this object doesn't need to set meta->id at all, it can
all
> > be managed by the owner.
> 
> Feels to me like the wrong thing to do but I can.  You sure?

Okay, here's the thing of it.  The concrete class of this type that supports
both internal and external storage (currently only sample_vector but eventually
sample_map) have two constructors:
- one that is all internal storage
- one that is all external storage

I don't think it would make sense to have that first one change to a mix of
internal/external (and we certainly don't want only all-external) so if I remove
the local_meta_ from here, it would need to be added to both sample_vector and
sample_map.  I'm okay with that but it seems a bit redundant.

[Alexei Svitkine (slow), 2015-12-09 23:03:40]

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
File base/metrics/histogram_samples.h (right):

https://codereview.chromium.org/1471073007/diff/160001/base/metrics/histogram...
base/metrics/histogram_samples.h:93: Metadata* meta_;
On 2015/12/05 18:12:37, bcwhite wrote:
> On 2015/12/04 19:34:46, bcwhite wrote:
> > On 2015/12/04 18:21:00, Alexei Svitkine (slow) wrote:
> > > Per offline discussion, can we just make this object unconditionally take
a
> > ptr
> > > to Metadata? Then, this object doesn't need to set meta->id at all, it can
> all
> > > be managed by the owner.
> > 
> > Feels to me like the wrong thing to do but I can.  You sure?
> 
> Okay, here's the thing of it.  The concrete class of this type that supports
> both internal and external storage (currently only sample_vector but
eventually
> sample_map) have two constructors:
> - one that is all internal storage
> - one that is all external storage
> 
> I don't think it would make sense to have that first one change to a mix of
> internal/external (and we certainly don't want only all-external) so if I
remove
> the local_meta_ from here, it would need to be added to both sample_vector and
> sample_map.  I'm okay with that but it seems a bit redundant.

Okay, you've convinced me. Let's keep as-is.

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
base/metrics/histogram_samples.cc:75: CHECK(meta_->id == 0 || meta_->id == id);
Make this a DCHECK(). CHECKs() are generally reserved for security issues or
debugging production crashes.

Alternatively, if you still would like the safety of a CHECK during development,
you can keep it as-is but add a TODO to convert to a DCHECK that you can address
once the implementation is finalized and there are no issues.

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
base/metrics/histogram_samples.cc:76: *const_cast<uint64_t*>(&meta_->id) = id;
Nit: Remove the const qualifier on the field and make this a regular assignment.

Alternatively, you can keep the field const and initialize local_meta_ with id
in its constructor. Then, in the case where a Metadata* is passed, you just do
the CHECK and not the assignment, since at that point it would already have been
assigned.

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sample_ve...
File base/metrics/sample_vector.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sample_ve...
base/metrics/sample_vector.cc:20: bucket_ranges_(bucket_ranges) {
Can you use delegated constructor syntax instead of repeating a lot of code
here?

http://stackoverflow.com/questions/308276/call-constructor-from-constructor-in-c

Same thing for other places where you have multiple constructors.

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

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:74: new
SampleMap(HashMetricName(histogram_name())));
Pass in name_hash() instead.

https://codereview.chromium.org/1471073007/diff/180001/base/test/histogram_te...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/test/histogram_te...
base/test/histogram_tester.cc:125: if (!histogram)
Nit: {}

[bcwhite, 2015-12-10 14:51:33]

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
File base/metrics/histogram_samples.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
base/metrics/histogram_samples.cc:75: CHECK(meta_->id == 0 || meta_->id == id);
On 2015/12/09 23:03:40, Alexei Svitkine (slow) wrote:
> Make this a DCHECK(). CHECKs() are generally reserved for security issues or
> debugging production crashes.
> 
> Alternatively, if you still would like the safety of a CHECK during
development,
> you can keep it as-is but add a TODO to convert to a DCHECK that you can
address
> once the implementation is finalized and there are no issues.

Done.

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/histogram...
base/metrics/histogram_samples.cc:76: *const_cast<uint64_t*>(&meta_->id) = id;
On 2015/12/09 23:03:39, Alexei Svitkine (slow) wrote:
> Nit: Remove the const qualifier on the field and make this a regular
assignment.
> 
> Alternatively, you can keep the field const and initialize local_meta_ with id
> in its constructor. Then, in the case where a Metadata* is passed, you just do
> the CHECK and not the assignment, since at that point it would already have
been
> assigned.

Given that the assignment constraints have been shuffled externally, there's
really no necessity in making the id unchangeable.  I'll remove the const.

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sample_ve...
File base/metrics/sample_vector.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sample_ve...
base/metrics/sample_vector.cc:20: bucket_ranges_(bucket_ranges) {
On 2015/12/09 23:03:40, Alexei Svitkine (slow) wrote:
> Can you use delegated constructor syntax instead of repeating a lot of code
> here?
> 
>
http://stackoverflow.com/questions/308276/call-constructor-from-constructor-in-c
> 
> Same thing for other places where you have multiple constructors.

Done, though it's not possible to delegate the 2nd to the 3rd or vise-versa
because local_counts_ is only initialized for one of them and has to be
initialized first when it is.

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

https://codereview.chromium.org/1471073007/diff/180001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:74: new
SampleMap(HashMetricName(histogram_name())));
On 2015/12/09 23:03:40, Alexei Svitkine (slow) wrote:
> Pass in name_hash() instead.

Done.

https://codereview.chromium.org/1471073007/diff/180001/base/test/histogram_te...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/1471073007/diff/180001/base/test/histogram_te...
base/test/histogram_tester.cc:125: if (!histogram)
On 2015/12/09 23:03:40, Alexei Svitkine (slow) wrote:
> Nit: {}

Done.

[Alexei Svitkine (slow), 2015-12-10 16:02:37]

lgtm

[bcwhite, 2015-12-10 16:57:44]

Description was changed from

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

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

to

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
==========

[bcwhite, 2015-12-10 17:01:01]

Description was changed from

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
==========

to

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)
==========

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

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2015-12-10 17:05:15]

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

[commit-bot: I haz the power, 2015-12-10 18:37:27]

Description was changed from

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)
==========

to

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)
==========

[commit-bot: I haz the power, 2015-12-10 18:37:28]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-12-10 18:39:07]

Description was changed from

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)
==========

to

==========
Reorganize histograms for persistence.

This is split from cl/1425533011.  It does all the base work
to support holding histograms in persistent memory including
moving some files so they'll be accessible and separating
metadata for external storage.

BUG=546019
TBR=danakj
(for trivial change to base/test/histogram_tester.cc)

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

[commit-bot: I haz the power, 2015-12-10 18:39:07]

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

[Nico, 2015-12-11 22:38:25]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-12-11 22:38:26]

Could this have caused these UBSan reports:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
?

All the tests on
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
were passing earlier this week, now they all die with something about
histograms...

[Alexei Svitkine (slow), 2015-12-11 22:41:25]

Seems possible, those constructors were changed by the CL.

I am pretty confused by the crashes though. It seems the ubsan analyzer is
actually crashing - so sounds like an actual bug in ubsan?

On Fri, Dec 11, 2015 at 5:38 PM, <thakis@chromium.org> wrote:

> Could this have caused these UBSan reports:
>
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
> ?
>
> All the tests on
>
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
> were passing earlier this week, now they all die with something about
> histograms...
>
> https://codereview.chromium.org/1471073007/
>

-- 
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.

[Alexei Svitkine (slow), 2015-12-11 22:45:40]

In checkDynamicType(), there's a comment "A crash anywhere within this
function probably means the vptr is corrupted.". But this check seems to be
being done in the ctor, so perhaps the vptr isn't initialized yet and ubsan
has some assumption that's not valid?

On Fri, Dec 11, 2015 at 5:40 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Seems possible, those constructors were changed by the CL.
>
> I am pretty confused by the crashes though. It seems the ubsan analyzer is
> actually crashing - so sounds like an actual bug in ubsan?
>
> On Fri, Dec 11, 2015 at 5:38 PM, <thakis@chromium.org> wrote:
>
>> Could this have caused these UBSan reports:
>>
>>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
>> ?
>>
>> All the tests on
>>
>>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
>> were passing earlier this week, now they all die with something about
>> histograms...
>>
>> https://codereview.chromium.org/1471073007/
>>
>
>

-- 
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.

[hans, 2015-12-11 23:01:00]

Filed crbug.com/569187 for this.

On 2015/12/11 22:45:40, Alexei Svitkine (slow) wrote:
> In checkDynamicType(), there's a comment "A crash anywhere within this
> function probably means the vptr is corrupted.". But this check seems to be
> being done in the ctor, so perhaps the vptr isn't initialized yet and ubsan
> has some assumption that's not valid?
> 
> On Fri, Dec 11, 2015 at 5:40 PM, Alexei Svitkine
<mailto:asvitkine@chromium.org>
> wrote:
> 
> > Seems possible, those constructors were changed by the CL.
> >
> > I am pretty confused by the crashes though. It seems the ubsan analyzer is
> > actually crashing - so sounds like an actual bug in ubsan?
> >
> > On Fri, Dec 11, 2015 at 5:38 PM, <mailto:thakis@chromium.org> wrote:
> >
> >> Could this have caused these UBSan reports:
> >>
> >>
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
> >> ?
> >>
> >> All the tests on
> >>
> >>
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
> >> were passing earlier this week, now they all die with something about
> >> histograms...
> >>
> >> https://codereview.chromium.org/1471073007/

[bcwhite, 2015-12-14 14:09:28]

> In checkDynamicType(), there's a comment "A crash anywhere within this
> function probably means the vptr is corrupted.". But this check seems to be
> being done in the ctor, so perhaps the vptr isn't initialized yet and ubsan
> has some assumption that's not valid?

C++ is supposed to set the vptr before the ctor runs so that virtual functions
will operate (they call only local class and base, not any overridden by a
derived class whose ctor called this one).

I'll have to figure out how to reproduce this locally and then I'll see what I
can learn.

[bcwhite, 2015-12-16 13:17:02]

Fixed by https://codereview.chromium.org/1524853002/