Skeleton code of SparseHistogram

Skeleton code of SparseHistogram

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

[kaiwang, 2012-08-03 22:26:16]

On 2012/08/03 01:24:58, kaiwang wrote:

Hi isherman, could you help to take a look at this CL. I think it's best more
people (other than Jim) can review the histogram code change

[Ilya Sherman, 2012-08-04 01:18:35]

Generally looks ok; lots of minor comments inline.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
This doesn't seem right -- kSampleType_MAX fails the if-stmt's conditional...

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram_base.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram_base.cc:7: #include <climits>
nit: This is already included in the header.  Did you mean to remove that
include?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram_base.h:34: // Only for Histogram and its sub classes:
fancy bucket-naming support.
nit: It doesn't seem appropriate for HistogramBase to know about the Histogram
class.  I don't have any great design suggestions to improve this, though :(

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram_base.h:46: void ClearFlags(int32 flags) { flags_ &=
~flags; }
nit: Please either write the function names in hacker_case(), or move the
implementations to the implementation file (depending on whether it's important
for performance reasons for these functions to be inlined; I'm guessing it's
not...)

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:10: using std::string;
nit: This sort of using statement is not permitted by the Chromium style guide.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:16: Flags flags) {
nit: Should this be |int32 flags|, as it is everywhere else in this CL?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:17: HistogramBase* histogram = new
SparseHistogram(name);
nit: I believe this is an intentional leak, right?  If so, please add a comment
to that effect -- unprotected |new|s are surprising in Chromium code, and should
be documented.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:20: }
Is this method eventually going to look up the histogram in a global lookup
table or something like that?  If so, please add documentation to indicate that
this is meant to be updated.  Otherwise, can you help me understand the
advantage of the factory method over the constructor?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:35: void
SparseHistogram::WriteHTMLGraph(string* output) const {
nit: Please add a TODO, linked to a crbug, to implement this.  (Though I wish
this weren't part of base; it's really not core functionality :/)

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:20: class BASE_EXPORT_PRIVATE SparseHistogram :
public HistogramBase {
Can you help satisfy my curiosity and explain how BASE_EXPORT_PRIVATE differs
from BASE_EXPORT?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:22: typedef std::map<Sample, Count>
SampleCounts;
nit: I strongly prefer not to typedef this; you're saving very few characters,
at the cost of clarity.  However, if you do keep this typedef, please name it
something more like "SampleToCountMap"; by convention, "SampleCounts" should be
a list of SampleCount, which this is not.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:32: virtual void SnapshotSample(SampleCounts*
sample) const;
nit: Who will be calling this method?  It seems odd to have any non-overridden
public methods defined for this class, given that there's no way for a client to
construct an object of this type.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:32: virtual void SnapshotSample(SampleCounts*
sample) const;
nit: This method's name is confusing, since elsewhere "Sample" refers to a
single int32, whereas here is seems to refer to a map.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:46: scoped_ptr<base::Lock> lock_;
nit: Why is this heap-allocated?  It seems more natural to declare it as an
inline member.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:47: };
nit: DISALLOW_COPY_AND_ASSIGN?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram_unittest.cc:7: #include "base/logging.h"
nit: What is this include needed for?

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram_unittest.cc:11: namespace base {
nit: Please leave a blank line after this line.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram_unittest.cc:15: return new SparseHistogram(name);
Why is the test code using the constructor, rather than FactoryGet?  If you keep
this design, please don't leak the histogram from the test code -- there's no
need.  Also, please add a test that does use FactoryGet, and verify that the
flags are set correctly.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram_unittest.cc:37: }
Since the SparseHistogram code uses locks, I would expect to see the test code
exercise those locks.

[kaiwang, 2012-08-08 03:59:33]

Thanks for your careful review. Impressive.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram.cc#n...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
Good catch! It's fixed in another CL

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram_base.cc
File base/metrics/histogram_base.cc (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram_base...
base/metrics/histogram_base.cc:7: #include <climits>
Done, removed the one in .h

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram_base.h
File base/metrics/histogram_base.h (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram_base...
base/metrics/histogram_base.h:34: // Only for Histogram and its sub classes:
fancy bucket-naming support.
agree... Let's keep this for code compatibility for now. Maybe we can improve
later

http://codereview.chromium.org/10830156/diff/8002/base/metrics/histogram_base...
base/metrics/histogram_base.h:46: void ClearFlags(int32 flags) { flags_ &=
~flags; }
Will move to .cc file, so it will not affect other files calling these
functions.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
File base/metrics/sparse_histogram.cc (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.cc:10: using std::string;
are you sure about this?
This is allowed in google style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces

I didn't see it forbidden in chrome style guide..

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.cc:16: Flags flags) {
Done. Good catch

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.cc:20: }
You are right. FactoryGet will eventually register and get SparseHistogram from
StatisticsRecorder.

Now StatisticsRecorder only supports Histogram. Need more refactor CLs

Added TODO.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.cc:35: void
SparseHistogram::WriteHTMLGraph(string* output) const {
Added todo.
Most code is in histogram.cc. Seems not easy to move it out of base...

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
File base/metrics/sparse_histogram.h (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.h:20: class BASE_EXPORT_PRIVATE SparseHistogram :
public HistogramBase {
Definitions are exactly the same... But I have to export the class or I can not
test.
Anyone writing code outside base knows he should not depend on this
EXPORT_PRIVATE, but there's no hard restrictions.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.h:22: typedef std::map<Sample, Count>
SampleCounts;
ok
I don't like this kind of typedefs either. Just want to be consistency with
other code of histogram

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.h:32: virtual void SnapshotSample(SampleCounts*
sample) const;
This is a temporary method.. Keep it for testing before I can abstract the data
structure from Histogram::SampleSet

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.h:46: scoped_ptr<base::Lock> lock_;
Tried to limit header file dependencies. But I think you are right, it does not
worth

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram.h:47: };
On 2012/08/04 01:18:35, Ilya Sherman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
File base/metrics/sparse_histogram_unittest.cc (right):

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram_unittest.cc:7: #include "base/logging.h"
On 2012/08/04 01:18:35, Ilya Sherman wrote:
> nit: What is this include needed for?

Done.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram_unittest.cc:11: namespace base {
On 2012/08/04 01:18:35, Ilya Sherman wrote:
> nit: Please leave a blank line after this line.

Done.

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram_unittest.cc:15: return new SparseHistogram(name);
Deleted the newed histogram.

Will test FactoryGet when it's fully implemented

http://codereview.chromium.org/10830156/diff/8002/base/metrics/sparse_histogr...
base/metrics/sparse_histogram_unittest.cc:37: }
What's the common way in chromium code to test locks?

[Ilya Sherman, 2012-08-08 05:00:29]

LGTM

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
On 2012/08/08 03:59:33, kaiwang wrote:
> Good catch! It's fixed in another CL

Ok.  In that case, can you move this change into that CL as well?  It doesn't
seem relevant to refactoring code for the SparseHistogram implementation.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.cc:10: using std::string;
On 2012/08/08 03:59:33, kaiwang wrote:
> are you sure about this?
> This is allowed in google style guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
> 
> I didn't see it forbidden in chrome style guide..

Sorry, you're right, this sort of using statement is allowed -- I read the "std"
part and mentally translated it into the more usual "using namespace std". 
However, I think it's generally discouraged to use the using directive for
members of the std:: namespace.  At least, I haven't seen it done elsewhere.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram.h:32: virtual void SnapshotSample(SampleCounts*
sample) const;
On 2012/08/08 03:59:33, kaiwang wrote:
> This is a temporary method.. Keep it for testing before I can abstract the
data
> structure from Histogram::SampleSet

Ok.  In that case, please add a comment indicating that this is a temporary
method, intended only for testing.  Maybe also name the method
SnapshotSampleForTesting(), so that it's hard for non-test dependencies on it to
accidentally creep in.

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/sparse...
base/metrics/sparse_histogram_unittest.cc:37: }
On 2012/08/08 03:59:33, kaiwang wrote:
> What's the common way in chromium code to test locks?

I'm not sure if there's an especially common way; emailing chromium-dev might
give you a good answer there.  Offhand, though, I would try interacting with the
SparseHistogram from multiple threads, and ensure that the test passes under
TSan.

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/histogra...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/histogra...
base/metrics/histogram_base.h:57: };
nit: DISALLOW_COPY_AND_ASSIGN()?

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram.h:19: class Lock;
nit: No need for this anymore.

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram.h:42: std::map<Sample, Count> sample_;
nit: Perhaps "samples_"?

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram_unittest.cc:15: SparseHistogram*
NewSparseHistogram(const std::string& name) {
nit: I think you can return a scoped_ptr<SparseHistogram> here.  The below
return statement would then look like:

return scoped_ptr<SparseHistogram>(new SparseHistogram(name));

(If you weren't constructing a temporary object, you'd use scoped_ptr<>'s
.Pass() method.)

[kaiwang, 2012-08-08 22:17:08]

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
That's already submitted

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/histogra...
File base/metrics/histogram_base.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/histogra...
base/metrics/histogram_base.h:57: };
This is an abstract class. So it should be fine not to declare this. Added for
consistency

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram.h (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram.h:19: class Lock;
On 2012/08/08 05:00:30, Ilya Sherman wrote:
> nit: No need for this anymore.

Done.

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram.h:42: std::map<Sample, Count> sample_;
On 2012/08/08 05:00:30, Ilya Sherman wrote:
> nit: Perhaps "samples_"?

Done.

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram_unittest.cc:15: SparseHistogram*
NewSparseHistogram(const std::string& name) {
IMHO, the name of the function indicates allocating object from heap, and caller
should handle the life cycle.
So maybe just returning a raw pointer is more clear to use?

[commit-bot: I haz the power, 2012-08-08 22:46:47]

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

[commit-bot: I haz the power, 2012-08-08 22:46:52]

Presubmit check for 10830156-13 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 1.1s to calculate.

[Ilya Sherman, 2012-08-08 22:49:40]

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
On 2012/08/08 22:17:08, kaiwang wrote:
> That's already submitted

Could you rebase this CL, then?  It's still showing up as a diff, which is why
I'm still asking about it ;)

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram_unittest.cc:15: SparseHistogram*
NewSparseHistogram(const std::string& name) {
On 2012/08/08 22:17:08, kaiwang wrote:
> IMHO, the name of the function indicates allocating object from heap, and
caller
> should handle the life cycle.
> So maybe just returning a raw pointer is more clear to use?

If at all possible, for Chromium code we prefer to implement code that is
guaranteed to be safe.  So, rather than relying on the caller to manage the life
cycle, which the caller could potentially get wrong, we use scoped_ptr<>'s,
which are much much harder to get wrong.

[kaiwang, 2012-08-08 23:03:36]

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
File base/metrics/histogram.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/8002/base/metrics/histog...
base/metrics/histogram.cc:465: *maximum = kSampleType_MAX;
See patch set 4

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
File base/metrics/sparse_histogram_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830156/diff/13/base/metrics/sparse_h...
base/metrics/sparse_histogram_unittest.cc:15: SparseHistogram*
NewSparseHistogram(const std::string& name) {
On 2012/08/08 22:49:40, Ilya Sherman wrote:
> On 2012/08/08 22:17:08, kaiwang wrote:
> > IMHO, the name of the function indicates allocating object from heap, and
> caller
> > should handle the life cycle.
> > So maybe just returning a raw pointer is more clear to use?
> 
> If at all possible, for Chromium code we prefer to implement code that is
> guaranteed to be safe.  So, rather than relying on the caller to manage the
life
> cycle, which the caller could potentially get wrong, we use scoped_ptr<>'s,
> which are much much harder to get wrong.

Done.

[jar (doing other things), 2012-08-08 23:10:40]

base/base.gyp* LGTM

[commit-bot: I haz the power, 2012-08-08 23:14:06]

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

[commit-bot: I haz the power, 2012-08-08 23:54:52]

Try job failure for 10830156-16006 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-09 00:30:26]

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

[commit-bot: I haz the power, 2012-08-09 01:47:47]

Try job failure for 10830156-16006 (retry) on linux_rel for step
"nacl_integration".
It's a second try, previously, step "nacl_integration" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-09 03:01:38]

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

[commit-bot: I haz the power, 2012-08-09 05:14:17]

Change committed as 150736