Add SampleMap and use it in SparseHistogram

Add SampleMap and use it in SparseHistogram


BUG=139612
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160485

[Ilya Sherman, 2012-10-01 21:24:50]

LGTM

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
File base/metrics/sample_map.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.cc:37: map<Sample, Count>::const_iterator it;
nit: Please define this within the for loop's variable initialization clause --
this doesn't need to be scoped outside the loop.

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.cc:60: return false;  // SparseHistogram only supports
bucket with size 1.
nit: Should this be a DCHECK?

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.cc:62: (instruction ==  HistogramSamples::ADD) ? count :
-count;
nit: I don't think you need the "HistogramSamples::" prefix, since this class
inherits from that one.

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
File base/metrics/sample_map.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.h:5: // SampleVector implements HistogramSamples
interface. It is used by all
nit: SampleMap

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.h:6: // SparseHistogram to store samples.
Optional nit: For the second sentence, perhaps something like "It is used by the
SparseHistogram class to store samples." ?

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.h:40: std::map<HistogramBase::Sample,
HistogramBase::Count> sample_count_;
nit: I think either |sample_counts_| or just |counts_| would be a slightly
better name, since this is a map containing multiple counts.

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.h:43: mutable base::Lock lock_;
This is tangential to the current CL, but I wonder if it's really worth having a
lock for sparse histograms, rather than requiring them to always be called from
a single thread...  It's rather atypical for Chrome objects to provide thread
safety, and it doesn't seem obviously necessary for this class to buck the
trend.  Would be good to discuss this more (probably not as part of this
CL/review).

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map.h:52: sample_count);
nit: Same naming comment as above.

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
File base/metrics/sample_map_unittest.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/10001/base/metrics/sampl...
base/metrics/sample_map_unittest.cc:17: samples.Accumulate(1, -300);
What does it mean for a bucket to have a negative count?  I'm surprised that
this isn't something we'd want to treat as an error, i.e. trip a DCHECK for.

[kaiwang, 2012-10-01 22:38:52]

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:37: map<Sample, Count>::const_iterator it;
On 2012/10/01 21:24:50, Ilya Sherman wrote:
> nit: Please define this within the for loop's variable initialization clause
--
> this doesn't need to be scoped outside the loop.

Done.

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:60: return false;  // SparseHistogram only supports
bucket with size 1.
As in SampleVector, this function can be called in different situations,
sometimes it's better to DCHECK, sometimes it's better to return some error
(e.g. parse histogram data from renderer).

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:62: (instruction ==  HistogramSamples::ADD) ? count :
-count;
Right, but I think this is a bit easier for code reading. If the reader is
interested to know where ADD is defined, he will go to histogram_samples.h.
Without class name, ADD can be defined anywhere

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:5: // SampleVector implements HistogramSamples
interface. It is used by all
On 2012/10/01 21:24:50, Ilya Sherman wrote:
> nit: SampleMap

Done.

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:6: // SparseHistogram to store samples.
On 2012/10/01 21:24:50, Ilya Sherman wrote:
> Optional nit: For the second sentence, perhaps something like "It is used by
the
> SparseHistogram class to store samples." ?

Done.

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:40: std::map<HistogramBase::Sample,
HistogramBase::Count> sample_count_;
On 2012/10/01 21:24:50, Ilya Sherman wrote:
> nit: I think either |sample_counts_| or just |counts_| would be a slightly
> better name, since this is a map containing multiple counts.

Done.

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:43: mutable base::Lock lock_;
At least for existing histograms, it's common to be called on different threads.
Also SparseHistogram does not mean to be fast (a major goal of Histogram).
As a "locked histogram" was originally Jim's idea. Jim, what's your thought?

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:52: sample_count);
On 2012/10/01 21:24:50, Ilya Sherman wrote:
> nit: Same naming comment as above.

Done.

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map_un...
File base/metrics/sample_map_unittest.cc (right):

http://codereview.chromium.org/11022002/diff/10001/base/metrics/sample_map_un...
base/metrics/sample_map_unittest.cc:17: samples.Accumulate(1, -300);
Because HistogramSamples support Subtract, it's hard to avoid negative counts.
(and in my opinion, may not worth, because the perception is histogram can be
corrupted at anytime).

[jar (doing other things), 2012-10-01 23:10:38]

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:13: typedef HistogramBase::Count Count;
nit: would using work here?

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:86: DCHECK(!Done());
I'm surprised we're not holding a lock when we access the map.

We especially need a lock when snapshotting.

I'd suggest that the constructor should do the snapshotting.

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:40: std::map<HistogramBase::Sample,
HistogramBase::Count> sample_counts_;
nit: I'd prefer to see a typedef... as this map name is long, and makes lines
hard to read... and wrap.

It would get used 4 times in this file.

Possible names:
SampleCountsMap

[kaiwang, 2012-10-02 02:57:56]

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:13: typedef HistogramBase::Count Count;
nope... using only works for namespace

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:86: DCHECK(!Done());
As discussed locked in SnapshotSamples function

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:40: std::map<HistogramBase::Sample,
HistogramBase::Count> sample_counts_;
Seems there's no way to make it a private typedef here, because it's used in
constructor.

I think the best solution might be moving Sample and Count to base

[jar (doing other things), 2012-10-02 17:41:10]

I'm pretty scared about the use of locks in this class.  I think you need a
clear philosophy that is consistently applied.  Examples of issues are listed
below.

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/18001/base/metrics/sample_map.h#...
base/metrics/sample_map.h:40: std::map<HistogramBase::Sample,
HistogramBase::Count> sample_counts_;
Moving Sample and Count SGTM. It will surely help with this long type.

Should we really move all of histograms to a base::metrics namespace?  This
would match the (current) directory structure, and avoid polluting the base::
namespace.

On 2012/10/02 02:57:56, kaiwang wrote:
> Seems there's no way to make it a private typedef here, because it's used in
> constructor.
> 
> I think the best solution might be moving Sample and Count to base

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:27: Count SampleMap::GetCount(Sample value) const {
This is a surprising API.  I would assume that you get the list of values by
iterating over the map.... but then it is surprising to (after releasing the
lock) see to look at the associated values.  

I'm becoming suspicious that for the lifetime of the iterator, the lock should
be held by the iterator, to prevent the map from changing during the iterations.

To say it another way... I'm wondering if the pulling of the iterator class out
makes sense, or whether it should *only* be used within the map class for
introspection and copying.  Perhaps the iterator should not be long lived, and
should only exist for the duration of one acquisition and release of the lock.

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:35: Count SampleMap::TotalCount() const {
Note that this approach, of gathering the total count under a lock, produces a
result that can't be compared with the RedundantCount.

To make them comparable, you must gather all stats under one acquisition of the
lock.

I think you can live with this... but it was a little surprising to me (I
envisioned that once we took the expense of using a lock, we'd have perfect
synchronization (except for when we got overwritten by memory smashers).

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:47: scoped_ptr<SampleCountIterator>
SampleMap::Iterator() const {
This is the scary interface.  We're giving away an iterator to a data structure
that is lock protected... and we don't hold the lock.  The iterator can become
invalid at any point now :-(.

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sample_map.h#...
base/metrics/sample_map.h:66: const std::map<HistogramBase::Sample,
HistogramBase::Count>::const_iterator
This seems like an implementation detail (a shortcut in fact), that is best
placed in the implementation file, possibly in an anonymous namespace. 

Putting it here just complicates reading... but seems to add no value.

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

http://codereview.chromium.org/11022002/diff/11013/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:32: snapshot->Add(samples_);
Will this induce numerous locks?  Note that we don't really need to lock
|snapshot| since we are the only one that knows about it.

[jar (doing other things), 2012-10-05 01:27:36]

I think this L G T M, unless Isherman has any comments.

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.cc:48: IncreaseRedundantCount(- redundant_count());
nit: remove space after unary operator |-|

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.h:38: HistogramSamples::Instruction instruction)
OVERRIDE;
The variable name and type bothered me (made me think too much when reading
this).  You should consider changing the name (but I'm not coming up with great
improvements). 

Perhaps:
HistogtramSamples::Operator operator

You suggested a comment, that "|instruction| is either ADD or SUBTRACT" which
probably should go above the declaration to add clarity.

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.h:50: sample_counts);
This is a looong line :-/.  I really think typdefs would help.

At least add a typedef to this class, so that it helps with lines 49, 61, and
62.

Possible example:
SampleToCountMap

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map_unittest.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map_unittest.cc:23: EXPECT_EQ(0, samples.TotalCount());
Make this anything other than 0 (0 happens by accident too often).

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
File base/metrics/sparse_histogram.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
base/metrics/sparse_histogram.cc:36: scoped_ptr<SampleMap> snapshot(new
SampleMap());
nit: always do as much as possible outside the lock.  You should new up the
SampleMap() before acquiring the lock.

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
base/metrics/sparse_histogram.h:38: SparseHistogram(const std::string& name);
nit: make private, unless we have a subclass using it.

[kaiwang, 2012-10-05 03:16:17]

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map.cc
File base/metrics/sample_map.cc (right):

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map.cc...
base/metrics/sample_map.cc:48: IncreaseRedundantCount(- redundant_count());
On 2012/10/05 01:27:36, jar wrote:
> nit: remove space after unary operator |-|

Done.

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map.h
File base/metrics/sample_map.h (right):

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map.h#...
base/metrics/sample_map.h:38: HistogramSamples::Instruction instruction)
OVERRIDE;
Changed to "Operator op", because operator is C++ key word

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map.h#...
base/metrics/sample_map.h:50: sample_counts);
On 2012/10/05 01:27:36, jar wrote:
> This is a looong line :-/.  I really think typdefs would help.
> 
> At least add a typedef to this class, so that it helps with lines 49, 61, and
> 62.
> 
> Possible example:
> SampleToCountMap

Done.

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map_un...
File base/metrics/sample_map_unittest.cc (right):

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sample_map_un...
base/metrics/sample_map_unittest.cc:23: EXPECT_EQ(0, samples.TotalCount());
On 2012/10/05 01:27:36, jar wrote:
> Make this anything other than 0 (0 happens by accident too often).

Done.

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.cc (right):

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sparse_histog...
base/metrics/sparse_histogram.cc:36: scoped_ptr<SampleMap> snapshot(new
SampleMap());
On 2012/10/05 01:27:36, jar wrote:
> nit: always do as much as possible outside the lock.  You should new up the
> SampleMap() before acquiring the lock.

Done.

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sparse_histog...
File base/metrics/sparse_histogram.h (right):

http://codereview.chromium.org/11022002/diff/15005/base/metrics/sparse_histog...
base/metrics/sparse_histogram.h:38: SparseHistogram(const std::string& name);
On 2012/10/05 01:27:36, jar wrote:
> nit: make private, unless we have a subclass using it.

Done.

[Ilya Sherman, 2012-10-05 03:29:24]

LGTM

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.cc:50: }
This method feels a little odd as part of the API -- it sort of suggests that
this class shouldn't be managing its own redundant count at all, and that
Accumulate() should only ever be called as part of construction.  Put another
way, it suggests that the inheritance relationship with HistogramSamples isn't
quite right.

It's ok to leave as is, but it feels a little heavy handed.

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
File base/metrics/sparse_histogram_unittest.cc (left):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
base/metrics/sparse_histogram_unittest.cc:25: ASSERT_EQ(0u, sample.size());
nit: Why did you remove the testcase covering an empty map?

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
File base/metrics/sample_map.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
base/metrics/sample_map.h:49: SampleToCountMap;
nit: Do we really want a public typedef?  If we move these classes into the
base::metrics:: namespace, we'd have std::map<Sample, Count>, which isn't
terribly long.

[kaiwang, 2012-10-05 04:02:38]

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.cc:50: }
Yes, it may feel a bit odd. But it's caused by different types of usage from
Histogram and SparseHistogram.
We do need a redundant count in HistogramSamples so I think it's still better
than having another redundant_count in Histogram.

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
File base/metrics/sparse_histogram_unittest.cc (left):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/spars...
base/metrics/sparse_histogram_unittest.cc:25: ASSERT_EQ(0u, sample.size());
On 2012/10/05 03:29:24, Ilya Sherman wrote:
> nit: Why did you remove the testcase covering an empty map?

Done.

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
File base/metrics/sample_map.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
base/metrics/sample_map.h:49: SampleToCountMap;
This is exactly what I suggested to Jim

[jar (doing other things), 2012-10-05 16:55:19]

LGTM

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
File base/metrics/sample_map.cc (right):

https://chromiumcodereview.appspot.com/11022002/diff/15005/base/metrics/sampl...
base/metrics/sample_map.cc:50: }
It does seem like the right answer is to have this reset performed explicitly in
the base class, by adding this method.  I am a bit surprised that we need such a
method anywhere.

On 2012/10/05 04:02:38, kaiwang wrote:
> Yes, it may feel a bit odd. But it's caused by different types of usage from
> Histogram and SparseHistogram.
> We do need a redundant count in HistogramSamples so I think it's still better
> than having another redundant_count in Histogram.

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
File base/metrics/sample_map.h (right):

https://chromiumcodereview.appspot.com/11022002/diff/23005/base/metrics/sampl...
base/metrics/sample_map.h:49: SampleToCountMap;
I agree the "rightish" thing is to indeed create a base::metrics namespace, and
move a lot of stuff into that namespace. That is a bigger change, and should not
be mixed with this CL.  

For now, it was really bothering me to have to read as many as three lines (that
should be one) because this was not shorter... so making it a typedef even in
this class seemed quite helpful (my style perspective... which I know offends
your sensibilities a bit... and also leaves Kai wandering between two code
reviewers).

If/when we move into metrics, we can all be happy.... but for now, I really
think line wraps to 3 lines in a declaration was too much.

On 2012/10/05 03:29:24, Ilya Sherman wrote:
> nit: Do we really want a public typedef?  If we move these classes into the
> base::metrics:: namespace, we'd have std::map<Sample, Count>, which isn't
> terribly long.

[commit-bot: I haz the power, 2012-10-05 18:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/11022002/22005

[commit-bot: I haz the power, 2012-10-05 21:29:48]

Change committed as 160485