Expose macros from base/metrics/histogram_macros.h as functions

Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375
Committed: https://crrev.com/09d945012fa2c299ace0b044277d67901d6158a5
Cr-Commit-Position: refs/heads/master@{#431917}

[nikunjb, 2016-11-12 00:44:35]

nikunjb@google.com changed reviewers:
+ asvitkine@google.com, nikunjb@google.com

[Alexei Svitkine (slow), 2016-11-12 00:51:13]

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

[Alexei Svitkine (slow), 2016-11-12 00:51:14]

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2016 and remove the (c)

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:15: //  
base::UmaHistogramBoolean("My.Boolean", true)
Remove the function comments from the .cc - they should only be in the .h

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
2016

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.h:22: // not possible to pass that to macro
expansion.
I would rephrase this and the previous sentence to mention that dynamic name
part first (i.e. since it's their purpose) and then mention that these are
slower.

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2016, no (c)

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions_unittest.cc:5: #include
"base/metrics/histogram_functions.h"
Nit: Add an empty line after this.

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.cc:82: const std::string& name, const
base::TimeDelta& sample,
Nit: 1 param per line.

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:60: const base::TimeDelta& sample,
Nit: I think base::TimeDelta should be passed by value. See its header.

[nikunjb, 2016-11-12 01:35:16]

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> 2016 and remove the (c)

Done.

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:15: //  
base::UmaHistogramBoolean("My.Boolean", true)
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> Remove the function comments from the .cc - they should only be in the .h

Done.

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> 2016

Done.

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions.h:22: // not possible to pass that to macro
expansion.
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> I would rephrase this and the previous sentence to mention that dynamic name
> part first (i.e. since it's their purpose) and then mention that these are
> slower.

Done.

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

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> 2016, no (c)

Done.

https://codereview.chromium.org/2393493002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_functions_unittest.cc:5: #include
"base/metrics/histogram_functions.h"
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> Nit: Add an empty line after this.

Done.

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.cc:82: const std::string& name, const
base::TimeDelta& sample,
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> Nit: 1 param per line.

Done.

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/2393493002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:60: const base::TimeDelta& sample,
On 2016/11/12 00:51:14, Alexei Svitkine (very slow) wrote:
> Nit: I think base::TimeDelta should be passed by value. See its header.

Done. Also fixed the TimeDelta used in histogram_functions

[nikunjb, 2016-11-12 01:36:51]

nikunjb@google.com changed reviewers:
+ thakis@chromium.org

[nikunjb, 2016-11-12 01:36:51]

thakis@chromium.org: Please review changes in base/BUILD.gn and
base/test/histogram_tester.*

[nikunjb, 2016-11-12 01:37:16]

The CQ bit was checked by nikunjb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 01:37:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-11-12 01:43:45]

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

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:63: const std::string& name, TimeDelta
sample, TimeDelta min,
Nit: 1 param per line if first one wraps. Maybe just run "git cl format"?

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:88: void UmaHistogramLongTimes100(
I suggest not including this one. We tend to discourage having 100-bucket
versions of histograms these days so not including seems better.

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
File base/metrics/histogram_functions.h (right):

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:19: // when histogram name is generated at
runtime. The functionality is
Nit: "when the histogram name"

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:20: // equivalent to macros defined in
histogram_macros.h for recording histogram.
Nit: Remove "for recording histogram"

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:22: // constants.
Nit: Combine with the previous sentence. Also suggest rewording, e.g.

", but allowing non-constant histogram names."

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:24: // additional memory lookups. So, these
shouldn't be used in performance
Nit: "By about a few additional memory lookups" sounds pretty vague and is not
obvious what it means. I suggest either rephrasing to be more precise ("because
the histogram objects are not cached between calls") or just removing the extra
detail.

[nikunjb, 2016-11-12 02:06:53]

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

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:63: const std::string& name, TimeDelta
sample, TimeDelta min,
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> Nit: 1 param per line if first one wraps. Maybe just run "git cl format"?

Done. That was convenient. :)

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.cc:88: void UmaHistogramLongTimes100(
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> I suggest not including this one. We tend to discourage having 100-bucket
> versions of histograms these days so not including seems better.

Done.

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
File base/metrics/histogram_functions.h (right):

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:19: // when histogram name is generated at
runtime. The functionality is
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> Nit: "when the histogram name"

Done.

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:20: // equivalent to macros defined in
histogram_macros.h for recording histogram.
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> Nit: Remove "for recording histogram"

Done.

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:22: // constants.
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> Nit: Combine with the previous sentence. Also suggest rewording, e.g.
> 
> ", but allowing non-constant histogram names."

Done.

https://codereview.chromium.org/2393493002/diff/80001/base/metrics/histogram_...
base/metrics/histogram_functions.h:24: // additional memory lookups. So, these
shouldn't be used in performance
On 2016/11/12 01:43:45, Alexei Svitkine (very slow) wrote:
> Nit: "By about a few additional memory lookups" sounds pretty vague and is not
> obvious what it means. I suggest either rephrasing to be more precise
("because
> the histogram objects are not cached between calls") or just removing the
extra
> detail.

Done. Rephrased to add additional comment. I wanted to give the order of speed
difference, so the user can make a judgement when using it (since retrieving
histogram from memory is not exactly a slow operation under normal
circumstances).

[Alexei Svitkine (slow), 2016-11-12 02:20:42]

lgtm, but please fix the CL description

After you land this, it would be good to migrate some of the users of the raw
APIs to these helper functions.

https://codereview.chromium.org/2393493002/diff/100001/base/test/histogram_te...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/2393493002/diff/100001/base/test/histogram_te...
base/test/histogram_tester.h:57: // We know exact number of samples for buckets
corresponding to a
Nit: wrapping is off - some words can still fit on this line.

[nikunjb, 2016-11-12 02:22:38]

Description was changed from

==========
'Change skeleton' for adding histograms function API. For starting discussion on
API.

BUG=650375
==========

to

==========
'Change skeleton' for adding histograms function API. For starting discussion on
API.

BUG=650375
==========

[nikunjb, 2016-11-14 19:13:02]

https://codereview.chromium.org/2393493002/diff/100001/base/test/histogram_te...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/2393493002/diff/100001/base/test/histogram_te...
base/test/histogram_tester.h:57: // We know exact number of samples for buckets
corresponding to a
On 2016/11/12 02:20:42, Alexei Svitkine (very slow) wrote:
> Nit: wrapping is off - some words can still fit on this line.

Done.

[nikunjb, 2016-11-14 19:16:47]

Description was changed from

==========
'Change skeleton' for adding histograms function API. For starting discussion on
API.

BUG=650375
==========

to

==========
Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375
==========

[Nico, 2016-11-14 20:00:27]

rs-lgtm

[nikunjb, 2016-11-14 20:06:15]

The CQ bit was checked by nikunjb@google.com

[nikunjb, 2016-11-14 20:06:16]

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/2393493002/#ps120001
(title: "Review comments")

[commit-bot: I haz the power, 2016-11-14 20:07:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 22:07:00]

Description was changed from

==========
Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375
==========

to

==========
Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375
==========

[commit-bot: I haz the power, 2016-11-14 22:07:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-14 22:08:46]

Description was changed from

==========
Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375
==========

to

==========
Expose macros from base/metrics/histogram_macros.h as functions

BUG=650375

Committed: https://crrev.com/09d945012fa2c299ace0b044277d67901d6158a5
Cr-Commit-Position: refs/heads/master@{#431917}
==========

[commit-bot: I haz the power, 2016-11-14 22:08:47]

Patchset 7 (id:??) landed as
https://crrev.com/09d945012fa2c299ace0b044277d67901d6158a5
Cr-Commit-Position: refs/heads/master@{#431917}

[wychen, 2016-11-14 23:56:10]

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

[wychen, 2016-11-14 23:56:11]

https://codereview.chromium.org/2393493002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_functions_unittest.cc (right):

https://codereview.chromium.org/2393493002/diff/120001/base/metrics/histogram...
base/metrics/histogram_functions_unittest.cc:40: TEST(HistogramFunctionsTest,
HistogramEnumeration) {
Might worth adding some no-compile tests. Alas it is disabled by default.

https://codereview.chromium.org/2393493002/diff/120001/base/metrics/histogram...
File base/metrics/histogram_macros.h (right):

https://codereview.chromium.org/2393493002/diff/120001/base/metrics/histogram...
base/metrics/histogram_macros.h:71: UMA_HISTOGRAM_ENUMERATION(name, sample,
value_max)
Ah. I was confused why we have to +1 twice.