Add support to increase a UMA histogram bucket by an aribitrary integer.

Add support to increase a UMA histogram bucket by an aribitrary integer.

The UMA code supports counting events that fall into ranges (buckets),
and this is implemented by adding 1 to the count for particular range.
The change is to add an arbitrary amount to a bucket on a single event.

To achieve this goal, we have added MultiAdd function to Histogram and
SparseHistogram classes. So the user of these classes will be able to
increase the count number of a bucket in these classes by an arbitrary
integer instead of one which is hardcoded in their Add function.

BUG=514317
Committed: https://crrev.com/6779b5c3bc0f026f503628c136f7bd02efd3e02e
Cr-Commit-Position: refs/heads/master@{#341961}

[amohammadkhan, 2015-07-27 20:06:17]

amohammadkhan@google.com changed reviewers:
+ bengr@chromium.org

[amohammadkhan, 2015-07-27 20:11:11]

On 2015/07/27 20:06:17, amohammadkhan wrote:

Do I need to add checking for negative values of second parameter of MultiAdd
or it is Okay to have negative values?

[bengr, 2015-07-28 17:18:10]

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

https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram....
base/metrics/histogram.h:184: void MultiAdd(Sample value, int repetition)
override;
Why the name 'repetition'?

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

https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram_...
base/metrics/histogram_unittest.cc:234: // Test the MultiAdd Function
Function -> function.

[bengr, 2015-07-28 17:18:27]

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

[amohammadkhan, 2015-07-28 22:27:40]

On 2015/07/28 17:18:10, bengr wrote:
> https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram.h
> File base/metrics/histogram.h (right):
> 
>
https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram....
> base/metrics/histogram.h:184: void MultiAdd(Sample value, int repetition)
> override;
> Why the name 'repetition'?

Do you have a suggestion for it? I think the best choice was "count", but it
seems they have used word count for something else, as already they have some
macros with CUSTOM_COUNT suffix. I wanted to show that it is not related to that
but I agree that repetition is not a good choice. (For now I changed it to
count) 


>
https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram_...
> File base/metrics/histogram_unittest.cc (right):
> 
>
https://codereview.chromium.org/1256363002/diff/20001/base/metrics/histogram_...
> base/metrics/histogram_unittest.cc:234: // Test the MultiAdd Function
> Function -> function.
Done.

[bengr, 2015-07-31 15:32:52]

asvitkine: PTAL.

[Alexei Svitkine (slow), 2015-07-31 15:39:28]

looks good generally, a few minor comments

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

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:287: void Histogram::MultiAdd(int value, int count) {
We should disallow negative values.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:294: value = 0;
Can you make a helper function for this clamping in the anon namespace and use
it both here and in Add().

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:298: 
Nit: Remove empty line.

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

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_base.h:123: virtual void MultiAdd(Sample value, int
repetition) = 0;
How about naming it AddCount(value, count). Also add some comments. Update your
tests to the new name too. Also, you still use "repetition" as a param name in
some of the files. Please update them.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_unittest.cc:256: 
Remove entry blank lines.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/sparse_his...
File base/metrics/sparse_histogram_unittest.cc (right):

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/sparse_his...
base/metrics/sparse_histogram_unittest.cc:84: 
Remove extra blank lines.

[amohammadkhan, 2015-08-03 17:30:43]

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

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:287: void Histogram::MultiAdd(int value, int count) {
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> We should disallow negative values.

Done. I change negative values to zero. Is it good enough or I have to make an
assertion or something else?

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:294: value = 0;
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> Can you make a helper function for this clamping in the anon namespace and use
> it both here and in Add().

Done.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram....
base/metrics/histogram.cc:298: 
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

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

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_base.h:123: virtual void MultiAdd(Sample value, int
repetition) = 0;
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> How about naming it AddCount(value, count). Also add some comments. Update
your
> tests to the new name too. Also, you still use "repetition" as a param name in
> some of the files. Please update them.

Done.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/histogram_...
base/metrics/histogram_unittest.cc:256: 
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> Remove entry blank lines.

Done.

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/sparse_his...
File base/metrics/sparse_histogram_unittest.cc (right):

https://codereview.chromium.org/1256363002/diff/40001/base/metrics/sparse_his...
base/metrics/sparse_histogram_unittest.cc:84: 
On 2015/07/31 15:39:27, Alexei Svitkine wrote:
> Remove extra blank lines.

Done.

[Alexei Svitkine (slow), 2015-08-03 18:15:43]

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:274: int ClampValue(int value) {
Put this in the anon namespace at the top of the file. Add a comment.

Alternatively, since you're making Add() call AddCount() now, maybe just keep
this inside the method like before instead of a separate helper.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:289: if (count < 0) {
If the body is a single line, no need for {}'s.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:290: count = 0;
Seems like a count of 0 would actually not change anything. Suggesting change if
to be <= 0 and then doing a return with a NOTREACHED() inside it.

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram_base.h:123: // In Add function the |value| bucket is
increased by one, but in some use
Add a blank line before.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.cc (left):

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:53: 
Nit: Keep the empty line.

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:55: if (count < 0) {
Same comment as in the other method. This if can probably be at the start of the
method, before the lock.

[amohammadkhan, 2015-08-04 18:22:52]

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:274: int ClampValue(int value) {
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> Put this in the anon namespace at the top of the file. Add a comment.
> 
> Alternatively, since you're making Add() call AddCount() now, maybe just keep
> this inside the method like before instead of a separate helper.

Done.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:289: if (count < 0) {
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> If the body is a single line, no need for {}'s.

Done.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram.cc:290: count = 0;
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> Seems like a count of 0 would actually not change anything. Suggesting change
if
> to be <= 0 and then doing a return with a NOTREACHED() inside it.

Done.

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/histogram...
base/metrics/histogram_base.h:123: // In Add function the |value| bucket is
increased by one, but in some use
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> Add a blank line before.

Done.

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.cc (left):

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:53: 
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> Nit: Keep the empty line.

Done.

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

https://codereview.chromium.org/1256363002/diff/100001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.cc:55: if (count < 0) {
On 2015/08/03 18:15:42, Alexei Svitkine wrote:
> Same comment as in the other method. This if can probably be at the start of
the
> method, before the lock.

Done.

[Alexei Svitkine (slow), 2015-08-04 18:25:12]

lgtm % comments

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

https://codereview.chromium.org/1256363002/diff/160001/base/metrics/histogram...
base/metrics/histogram_base.h:126: // function increases the |value| bucket by
|count|.
Mention that the |count| param must be > 0.

https://codereview.chromium.org/1256363002/diff/160001/base/metrics/sparse_hi...
File base/metrics/sparse_histogram.h (right):

https://codereview.chromium.org/1256363002/diff/160001/base/metrics/sparse_hi...
base/metrics/sparse_histogram.h:45: void AddCount (Sample value, int count)
override;
Remove space before (

[amohammadkhan, 2015-08-05 17:32:44]

Patchset #12 (id:220001) has been deleted

[amohammadkhan, 2015-08-05 18:44:19]

The CQ bit was checked by amohammadkhan@google.com

[amohammadkhan, 2015-08-05 18:44:20]

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/1256363002/#ps200001
(title: "After rebase-update")

[commit-bot: I haz the power, 2015-08-05 18:45:16]

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

[commit-bot: I haz the power, 2015-08-05 20:31:22]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-05 20:32:08]

Patchset 11 (id:??) landed as
https://crrev.com/6779b5c3bc0f026f503628c136f7bd02efd3e02e
Cr-Commit-Position: refs/heads/master@{#341961}

[Lei Zhang, 2015-08-05 23:47:13]

Is this causing all the memory.fyi bots to go red?

[amohammadkhan, 2015-08-06 00:19:25]

On 2015/08/05 23:47:13, Lei Zhang wrote:
> Is this causing all the memory.fyi bots to go red?

I think it is an intentional leak for SampleMap and it needs to be suppressed.
But I am not completely sure because I am a new intern, so I am waiting for
opinion of my reviewers.