Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361933002:
Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* … (Closed)
DescriptionRefactor histogram_macros.h.
This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h
BUG=649410
Committed: https://crrev.com/a1ee205ef9f3dce6af516cf18a7b6426ebbd8d55
Cr-Commit-Position: refs/heads/master@{#421241}
Patch Set 1 #
Total comments: 64
Patch Set 2 : mark comments #Patch Set 3 : extra fixes #
Total comments: 20
Patch Set 4 : asvitkine #
Total comments: 10
Patch Set 5 : asvitkine2 #Patch Set 6 : reword #
Total comments: 8
Patch Set 7 : mpearson comments 2 #
Messages
Total messages: 30 (10 generated)
Description was changed from ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file BUG=649410 ========== to ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file BUG=649410 ==========
Description was changed from ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file BUG=649410 ========== to ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h BUG=649410 ==========
rkaplow@chromium.org changed reviewers: + asvitkine@chromium.org, mpearson@chromium.org
This is good work, a huge improvement! I have a bunch of comments, all pretty easy to resolve I think. This took some serious heads-down review time. It was hard to keep all the movement of macros and comments in my head at once. Oy. Now I need a break. :-) Thanks for doing this, mark https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base... base/metrics/histogram_base.h:125: // TODO(rkaplow): Look into this, but looks like this is unused and can Resolve before submitting. (I see only 4 cases this flag is referred to. You should be able to fix these in this changelist or in an easy follow-up changelist. The TODO should not end up in the codebase.) https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (left): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:116: // All of these macros must be called with |name| as a runtime constant --- it This comments needs to stay in this file, prominently. Perhaps in each section? (You make the comment about exponential-size buckets prominent in each section. This seems more important to me.) https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:183: // Support histograming of an enumerated value. Samples should be one of the Please keep this comment by the CUSTOM_ENUMERATION histogram, even if you are discovering people from using it. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:8: #include "base/logging.h" I think this doesn't need logging anymore. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:14: // TODO(nikunjb): Move sparse macros here. nit: here -> to this file https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:28: // Note that all these macros default to exponential histograms - i.e. the nit: omit "Note that" https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:55: // as the number of buckets recorded. Usage: Please mention here that values less than the min are always included in an underflow bucket. This is makes sense to always use a min of 1 even if you're going to emit 0 values to the histogram. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:69: // sample must be a number measured in ms. This last sentence is wrong I think. Isn't the input a TimeDuration object? Did you mean to say that the value emitted will be recorded at millisecond granularity? If so, you might want to clarify the sample usage while you're here. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:107: // for a method to execute. This measures up to 10 seconds (this uses nit: consider (This uses UMA_HISTOGRAM_TIMES under the hood.) i.e., don't embed the parenthetical sentence in the other sentence https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:129: // sample must be a number measured in ms. ms -> kilobytes https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:155: // Enum values can be appended, but the order should never change. The value Instead of "Enum values can be appended, but the order should never change." consider something like "Enum values can be appended, but existing enums must never be renumbered or delete and reused." https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:171: // Histogram for percentages. This will be 100 buckets of length 1. length -> size https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:182: // stability log. |MetricsService::PrepareInitialStabilityLog|. are you missing the word "See" at the beginning of the second "sentence"? Perhaps even "See comments by declaration of MetricsService::PrepareInitialStabilityLog()." https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:212: // For a enum with customized range. In general, sparse histograms should be nit: a -> an https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Some of the macros in this file need better comments. But I suppose this is internal stuff so it's not too important. It's not required to improve them for this changelist. :-) https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:101: nit: extra blank line https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:102: // This nested macro is necessary to expand __COUNTER__ to an actual value. warning comment about helper macro? https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:106: nit: extra blank line https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:107: #define SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key) \ warning comment about helper macro? https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... File base/metrics/local_histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Consider histogram_macros_local instead for better ordering / grouping when listing files. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:8: nit: remove extra blank line https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:23: #define LOCAL_HISTOGRAM_COUNTS_100(name, sample) \ FYI, in this file, it looks like you merely copied the macros to here. I didn't review them closely. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:37: //------------------------------------------------------------------------------ optional nit: here and below the two blanks lines seem too much for me. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:40: // For usage details, see the equivalents in histogram_macros.h. nit: I know this duplicates the same sentence as above. However, the spacing looks odd. Either combine with the previous line or put a blank line // before the usage details line (and do the same elsewhere). https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:62: Missing usage details line? https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:72: #define LOCAL_HISTOGRAM_PERCENTAGE(name, under_one_hundred) \ A questionable section to put this in.. Maybe it's own section? I don't feel strongly though. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:81: #define LOCAL_HISTOGRAM_COUNTS(name, sample) \ This is only used in 7 files in the codepath (including two within base/metrics). It should be easy to retire/remove it. Then we can get rid of this whole section. Best done in another changelist though. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:84: #define LOCAL_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \ Appears to not be used anywhere. You can probably remove this.
nikunjb@chromium.org changed reviewers: + nikunjb@chromium.org
https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (left): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:102: #define STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ I see usage of this outside histogram_macros.h e.g //src/components/metrics/system_memory_stats_recorder_linux.cc and src/chromecast/base/metrics/cast_histograms.h Since this is only moved it won't have compilation issues, but is it okay keep them using internal implementations? https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:57: #define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \ Small comment: Generally it is easier to follow the code if the most general #define is at the top and special values are below it. So, first define UMA_HISTOGRAM_CUSTOM_COUNTS and then specialize the macro with counts like UMA_HISTOGRAM_COUNTS_100 (Same with UMA_HISTOGRAM_CUSTOM_TIMES)
https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:57: #define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \ On 2016/09/23 21:14:12, nikunjb1 wrote: > Small comment: Generally it is easier to follow the code if the most general > #define is at the top and special values are below it. > > So, first define UMA_HISTOGRAM_CUSTOM_COUNTS and then specialize the macro with > counts like UMA_HISTOGRAM_COUNTS_100 > > (Same with UMA_HISTOGRAM_CUSTOM_TIMES) I agree that Nik's suggestion would make the code easier to read. However, I think we should offer the more common macros first. I prefer this order. People should only have to read down to CUSTOM_whatever if they don't see anything they like sooner.
thanks for review! Sorry it was a bit awkward to review with all the pieces moving around. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base... base/metrics/histogram_base.h:125: // TODO(rkaplow): Look into this, but looks like this is unused and can On 2016/09/23 19:30:41, Mark P wrote: > Resolve before submitting. > (I see only 4 cases this flag is referred to. You should be able to fix these > in this changelist or in an easy follow-up changelist. The TODO should not end > up in the codebase.) I think there's a bit of cleanup associated with this, so would rather do in a subsequent CL. I don't mind removing the TODO if you'd like but I think it makes sense to commit for now so it doesn't get forgotten (or, less easily) https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (left): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:102: #define STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ On 2016/09/23 21:14:12, nikunjb1 wrote: > I see usage of this outside histogram_macros.h e.g > //src/components/metrics/system_memory_stats_recorder_linux.cc > > and > src/chromecast/base/metrics/cast_histograms.h > > Since this is only moved it won't have compilation issues, but is it okay keep > them using internal implementations? Probably not ideal - we'd need to see why they were doing this. For example https://codesearch.chromium.org/chromium/src/components/metrics/system_memory... - not sure this was needed. One thing we don't make easy to do is a numeric linear histogram. I wonder if we should add that for cases where people might want it. I'd rather not do in this CL but we could - what do you guys think? https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:116: // All of these macros must be called with |name| as a runtime constant --- it On 2016/09/23 19:30:41, Mark P wrote: > This comments needs to stay in this file, prominently. Perhaps in each section? > (You make the comment about exponential-size buckets prominent in each section. > This seems more important to me.) made a less technical version at the top of the file. Add one liners at the beginning of each section https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:183: // Support histograming of an enumerated value. Samples should be one of the On 2016/09/23 19:30:42, Mark P wrote: > Please keep this comment by the CUSTOM_ENUMERATION histogram, even if you are > discovering people from using it. This was attached to the local version - I assume we'd rather put it with the UMA version now, right? I put it with the UMA version as I just made the comment in the local_ section to refer to UMA documentation. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:8: #include "base/logging.h" On 2016/09/23 19:30:42, Mark P wrote: > I think this doesn't need logging anymore. Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:14: // TODO(nikunjb): Move sparse macros here. On 2016/09/23 19:30:41, Mark P wrote: > nit: here -> to this file Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:28: // Note that all these macros default to exponential histograms - i.e. the On 2016/09/23 19:30:41, Mark P wrote: > nit: omit "Note that" Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:55: // as the number of buckets recorded. Usage: On 2016/09/23 19:30:41, Mark P wrote: > Please mention here that values less than the min are always included in an > underflow bucket. This is makes sense to always use a min of 1 even if you're > going to emit 0 values to the histogram. gave a quick answer - think it's worth putting an example or this clear enough? https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:57: #define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \ On 2016/09/23 21:24:08, Mark P wrote: > On 2016/09/23 21:14:12, nikunjb1 wrote: > > Small comment: Generally it is easier to follow the code if the most general > > #define is at the top and special values are below it. > > > > So, first define UMA_HISTOGRAM_CUSTOM_COUNTS and then specialize the macro > with > > counts like UMA_HISTOGRAM_COUNTS_100 > > > > (Same with UMA_HISTOGRAM_CUSTOM_TIMES) > > I agree that Nik's suggestion would make the code easier to read. However, I > think we should offer the more common macros first. I prefer this order. > People should only have to read down to CUSTOM_whatever if they don't see > anything they like sooner. That was my reasoning for placing it this way - I'd rather them see the easy macros first. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:69: // sample must be a number measured in ms. On 2016/09/23 19:30:42, Mark P wrote: > This last sentence is wrong I think. Isn't the input a TimeDuration object? > > Did you mean to say that the value emitted will be recorded at millisecond > granularity? > > If so, you might want to clarify the sample usage while you're here. Ah, thanks. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:107: // for a method to execute. This measures up to 10 seconds (this uses On 2016/09/23 19:30:42, Mark P wrote: > nit: consider > (This uses UMA_HISTOGRAM_TIMES under the hood.) > i.e., don't embed the parenthetical sentence in the other sentence just removed the parenthesis. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:129: // sample must be a number measured in ms. On 2016/09/23 19:30:42, Mark P wrote: > ms -> kilobytes Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:155: // Enum values can be appended, but the order should never change. The value On 2016/09/23 19:30:41, Mark P wrote: > Instead of "Enum values can be appended, but the order should never change." > consider something like > "Enum values can be appended, but existing enums must never be renumbered or > delete and reused." Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:171: // Histogram for percentages. This will be 100 buckets of length 1. On 2016/09/23 19:30:41, Mark P wrote: > length -> size Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:182: // stability log. |MetricsService::PrepareInitialStabilityLog|. On 2016/09/23 19:30:41, Mark P wrote: > are you missing the word "See" at the beginning of the second "sentence"? > Perhaps even "See comments by declaration of > MetricsService::PrepareInitialStabilityLog()." yup I was. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:212: // For a enum with customized range. In general, sparse histograms should be On 2016/09/23 19:30:42, Mark P wrote: > nit: a -> an Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/23 19:30:42, Mark P wrote: > Some of the macros in this file need better comments. But I suppose this is > internal stuff so it's not too important. It's not required to improve them for > this changelist. :-) Acknowledged. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:101: On 2016/09/23 19:30:42, Mark P wrote: > nit: extra blank line Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:102: // This nested macro is necessary to expand __COUNTER__ to an actual value. On 2016/09/23 19:30:42, Mark P wrote: > warning comment about helper macro? Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:106: On 2016/09/23 19:30:42, Mark P wrote: > nit: extra blank line Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros_internal.h:107: #define SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key) \ On 2016/09/23 19:30:42, Mark P wrote: > warning comment about helper macro? Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... File base/metrics/local_histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/23 19:30:42, Mark P wrote: > Consider histogram_macros_local instead for better ordering / grouping when > listing files. Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:8: On 2016/09/23 19:30:42, Mark P wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:23: #define LOCAL_HISTOGRAM_COUNTS_100(name, sample) \ On 2016/09/23 19:30:42, Mark P wrote: > FYI, in this file, it looks like you merely copied the macros to here. I didn't > review them closely. I just did reordering in general - I don't think I added any new ones here. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:37: //------------------------------------------------------------------------------ On 2016/09/23 19:30:43, Mark P wrote: > optional nit: here and below the two blanks lines seem too much for me. Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:40: // For usage details, see the equivalents in histogram_macros.h. On 2016/09/23 19:30:42, Mark P wrote: > nit: I know this duplicates the same sentence as above. However, the spacing > looks odd. Either combine with the previous line or put a blank line // before > the usage details line (and do the same elsewhere). Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:62: On 2016/09/23 19:30:42, Mark P wrote: > Missing usage details line? Done. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:72: #define LOCAL_HISTOGRAM_PERCENTAGE(name, under_one_hundred) \ On 2016/09/23 19:30:43, Mark P wrote: > A questionable section to put this in.. Maybe it's own section? I don't feel > strongly though. agreed - I ended up putting it with enums since it acts as an enum under the hood although usage wise it's not a bit different. I'm fine moving it if anyone feels like it should be somewhere different. https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:81: #define LOCAL_HISTOGRAM_COUNTS(name, sample) \ On 2016/09/23 19:30:43, Mark P wrote: > This is only used in 7 files in the codepath (including two within > base/metrics). It should be easy to retire/remove it. Then we can get rid of > this whole section. > Best done in another changelist though. Ack https://codereview.chromium.org/2361933002/diff/1/base/metrics/local_histogra... base/metrics/local_histogram_macros.h:84: #define LOCAL_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \ On 2016/09/23 19:30:42, Mark P wrote: > Appears to not be used anywhere. You can probably remove this. I did consider this. I was thinking it might make sense to leave since it makes sense to mirror the UMA_ versions. On the other hand, since this is not encouraged for use, and is not being used at all now, seems fine to remove.
Looks great overall, some comments below. :) https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_base.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_base.h:126: // be removed. FYI: I disagree with the earlier comment about this about resolving this TODO in this CL. I think this CL is already doing a lot so I support adding the TODO here and looking at this in a follow up CL. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:38: // Sample Usage: Nit: Don't capitalize usage, i.e. "Sample usage:". Same for all the other places. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:39: // UMA_HISTOGRAM_COUNTS_100("My.Histogram", sample); Nit: I suggest indenting these sample usage lines by 2 spaces, so "// UMA_HISTOGRAM_"... https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:53: #define UMA_HISTOGRAM_COUNTS_1000000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(\ 1000000 is quite a mouthful. Let's name it 10M and also maybe add 100M. I would also maybe add a comment to encourage people to be quite generous with their maxes, since a common error is specifying too low of a max and needing to later increase the range. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:61: // Usage: Nit: "Sample usage:" and put an empty line above, to be consistent with the other examples. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:104: // as the number of buckets recorded. Usage: Nit: "Sample usage:" on a separate line with a blank before. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:116: // Sample usage: Nit: Add a blank line before. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:153: // Enumeration histograms. Nit: How about making this first? Otherwise someone has to scroll through the more advanced counts to get here. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:182: // Histogram for percentages. This will be 100 buckets of size 1. I'd put its own header here, since I don't think people think about this as an enum. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:106: #define SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, is_long, key) \ Nit: Add INTERNAL_ prefix here and to the macro below. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_macros_local.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros_local.h:8: Remove extra blank.
https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:38: // Sample Usage: On 2016/09/26 20:35:48, Alexei Svitkine (very slow) wrote: > Nit: Don't capitalize usage, i.e. "Sample usage:". Same for all the other > places. Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:39: // UMA_HISTOGRAM_COUNTS_100("My.Histogram", sample); On 2016/09/26 20:35:48, Alexei Svitkine (very slow) wrote: > Nit: I suggest indenting these sample usage lines by 2 spaces, so "// > UMA_HISTOGRAM_"... Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:53: #define UMA_HISTOGRAM_COUNTS_1000000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(\ On 2016/09/26 20:35:47, Alexei Svitkine (very slow) wrote: > 1000000 is quite a mouthful. Let's name it 10M and also maybe add 100M. I would > also maybe add a comment to encourage people to be quite generous with their > maxes, since a common error is specifying too low of a max and needing to later > increase the range. done, switched the example too https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:61: // Usage: On 2016/09/26 20:35:47, Alexei Svitkine (very slow) wrote: > Nit: "Sample usage:" and put an empty line above, to be consistent with the > other examples. Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:104: // as the number of buckets recorded. Usage: On 2016/09/26 20:35:48, Alexei Svitkine (very slow) wrote: > Nit: "Sample usage:" on a separate line with a blank before. Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:116: // Sample usage: On 2016/09/26 20:35:48, Alexei Svitkine (very slow) wrote: > Nit: Add a blank line before. Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:153: // Enumeration histograms. On 2016/09/26 20:35:47, Alexei Svitkine (very slow) wrote: > Nit: How about making this first? Otherwise someone has to scroll through the > more advanced counts to get here. done, and local to match https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros.h:182: // Histogram for percentages. This will be 100 buckets of size 1. On 2016/09/26 20:35:47, Alexei Svitkine (very slow) wrote: > I'd put its own header here, since I don't think people think about this as an > enum. Done. https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/40001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:106: #define SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, is_long, key) \ On 2016/09/26 20:35:48, Alexei Svitkine (very slow) wrote: > Nit: Add INTERNAL_ prefix here and to the macro below. Done.
LGTM with a few comments below. Please wait for your other two reviewers, though - as they might see things I missed. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:36: // UMA_HISTOGRAM_ENUMERATION("My.Enumeration", Enum::VALUE, Enum::EVENT_MAX); Nit: Enums actually don't require prefixing with "Enum::" foo unless you use enum classes, which require casting. So this example isn't very real. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:37: // Enum values can be appended, but existing enums must never be renumbered or Nit: "Enum values can be appended" sounds confusing to me. I would rephrase ("new enum values can be added later on, but"). https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:229: // Deprecated histograms. Not recommended for current use. Nit: Deprecated histogram macros. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:13: // TODO(rkaplow): Improve commenting of these methods. Please add a top level comment mentioning that these are internal macros and should not be used by outside code. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:110: #define INTERNAL_SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key) \ Reduce spaces to make \ fit in 80 cols. Same on line 106 above.
https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:36: // UMA_HISTOGRAM_ENUMERATION("My.Enumeration", Enum::VALUE, Enum::EVENT_MAX); On 2016/09/26 21:25:17, Alexei Svitkine (very slow) wrote: > Nit: Enums actually don't require prefixing with "Enum::" foo unless you use > enum classes, which require casting. So this example isn't very real. Done. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:37: // Enum values can be appended, but existing enums must never be renumbered or On 2016/09/26 21:25:16, Alexei Svitkine (very slow) wrote: > Nit: "Enum values can be appended" sounds confusing to me. I would rephrase > ("new enum values can be added later on, but"). Done. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros.h:229: // Deprecated histograms. Not recommended for current use. On 2016/09/26 21:25:17, Alexei Svitkine (very slow) wrote: > Nit: Deprecated histogram macros. Done. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... File base/metrics/histogram_macros_internal.h (right): https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:13: // TODO(rkaplow): Improve commenting of these methods. On 2016/09/26 21:25:17, Alexei Svitkine (very slow) wrote: > Please add a top level comment mentioning that these are internal macros and > should not be used by outside code. Done. https://codereview.chromium.org/2361933002/diff/60001/base/metrics/histogram_... base/metrics/histogram_macros_internal.h:110: #define INTERNAL_SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key) \ On 2016/09/26 21:25:17, Alexei Svitkine (very slow) wrote: > Reduce spaces to make \ fit in 80 cols. Same on line 106 above. Done. Surprised the presubmit didn't catch this, it caught earlier >80 violations.
lgtm
lgtm pending resolution of comments below https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_base... base/metrics/histogram_base.h:125: // TODO(rkaplow): Look into this, but looks like this is unused and can On 2016/09/26 20:16:09, rkaplow wrote: > On 2016/09/23 19:30:41, Mark P wrote: > > Resolve before submitting. > > (I see only 4 cases this flag is referred to. You should be able to fix these > > in this changelist or in an easy follow-up changelist. The TODO should not > end > > up in the codebase.) > > I think there's a bit of cleanup associated with this, so would rather do in a > subsequent CL. I don't mind removing the TODO if you'd like but I think it makes > sense to commit for now so it doesn't get forgotten (or, less easily) Okay. https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (left): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:102: #define STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ On 2016/09/26 20:16:10, rkaplow wrote: > On 2016/09/23 21:14:12, nikunjb1 wrote: > > I see usage of this outside histogram_macros.h e.g > > //src/components/metrics/system_memory_stats_recorder_linux.cc > > > > and > > src/chromecast/base/metrics/cast_histograms.h > > > > Since this is only moved it won't have compilation issues, but is it okay keep > > them using internal implementations? > > Probably not ideal - we'd need to see why they were doing this. > > For example > https://codesearch.chromium.org/chromium/src/components/metrics/system_memory... > - not sure this was needed. > > One thing we don't make easy to do is a numeric linear histogram. I wonder if we > should add that for cases where people might want it. > > I'd rather not do in this CL but we could - what do you guys think? Generally I've been advising people to use ENUMERATION when they want linear histograms. (It's only a small encouragement; I don't feel strongly.) It's hard to screw up the bucketing if you change things later. Maybe we should simply add a LINEAR macro instead? https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:107: // buckets. On 2016/09/26 20:16:10, rkaplow wrote: > On 2016/09/23 19:30:41, Mark P wrote: > > Please mention here that values less than the min are always included in an > > underflow bucket. This is makes sense to always use a min of 1 even if you're > > going to emit 0 values to the histogram. > > gave a quick answer - think it's worth putting an example or this clear enough? I think you should say explicitly that therefore min values should be >=1; putting a lower min is not necessary. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:164: // for a method to execute. This measures up to 10 seconds. This uses nit: second This could be "It"; the sentence will read better. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:188: // All of these macros must be called with |name| as a runtime constant. You forgot this warning in the section on count histograms and the section on percent histograms. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros_local.h (right): https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros_local.h:38: //------------------------------------------------------------------------------ optional: I suggest the counts section should go first (as you had it originally). comment withdrawn: I see Alexei and I disagree, and I don't feel strongly.
https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:107: // buckets. On 2016/09/26 23:06:47, Mark P wrote: > On 2016/09/26 20:16:10, rkaplow wrote: > > On 2016/09/23 19:30:41, Mark P wrote: > > > Please mention here that values less than the min are always included in an > > > underflow bucket. This is makes sense to always use a min of 1 even if > you're > > > going to emit 0 values to the histogram. > > > > gave a quick answer - think it's worth putting an example or this clear > enough? > > I think you should say explicitly that therefore min values should be >=1; > putting a lower min is not necessary. Done. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:164: // for a method to execute. This measures up to 10 seconds. This uses On 2016/09/26 23:06:47, Mark P wrote: > nit: second This could be "It"; the sentence will read better. Done. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros.h:188: // All of these macros must be called with |name| as a runtime constant. On 2016/09/26 23:06:47, Mark P wrote: > You forgot this warning in the section on count histograms and the section on > percent histograms. Done. https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... File base/metrics/histogram_macros_local.h (right): https://codereview.chromium.org/2361933002/diff/100001/base/metrics/histogram... base/metrics/histogram_macros_local.h:38: //------------------------------------------------------------------------------ On 2016/09/26 23:06:47, Mark P wrote: > optional: I suggest the counts section should go first (as you had it > originally). > > comment withdrawn: I see Alexei and I disagree, and I don't feel strongly. Ok - I don't personally care much, so will leave as is.
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, mpearson@chromium.org, nikunjb@chromium.org Link to the patchset: https://codereview.chromium.org/2361933002/#ps120001 (title: "mpearson comments 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rkaplow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h BUG=649410 ========== to ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h BUG=649410 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h BUG=649410 ========== to ========== Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* to a seperate file, and hides the internal macros that do not need to be exposed to histogram users into an internal file. This doesn't require any client changes as both files are includes in histogram_macros.h BUG=649410 Committed: https://crrev.com/a1ee205ef9f3dce6af516cf18a7b6426ebbd8d55 Cr-Commit-Position: refs/heads/master@{#421241} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a1ee205ef9f3dce6af516cf18a7b6426ebbd8d55 Cr-Commit-Position: refs/heads/master@{#421241}
Message was sent while issue was closed.
https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... File base/metrics/histogram_macros.h (left): https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... base/metrics/histogram_macros.h:102: #define STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ On 2016/09/26 23:06:47, Mark P wrote: > On 2016/09/26 20:16:10, rkaplow wrote: > > On 2016/09/23 21:14:12, nikunjb1 wrote: > > > I see usage of this outside histogram_macros.h e.g > > > //src/components/metrics/system_memory_stats_recorder_linux.cc > > > > > > and > > > src/chromecast/base/metrics/cast_histograms.h > > > > > > Since this is only moved it won't have compilation issues, but is it okay > keep > > > them using internal implementations? > > > > Probably not ideal - we'd need to see why they were doing this. > > > > For example > > > https://codesearch.chromium.org/chromium/src/components/metrics/system_memory... > > - not sure this was needed. > > > > One thing we don't make easy to do is a numeric linear histogram. I wonder if > we > > should add that for cases where people might want it. > > > > I'd rather not do in this CL but we could - what do you guys think? > > Generally I've been advising people to use ENUMERATION when they want linear > histograms. (It's only a small encouragement; I don't feel strongly.) It's > hard to screw up the bucketing if you change things later. > > Maybe we should simply add a LINEAR macro instead? What do you think about this suggestion?
Message was sent while issue was closed.
On 2016/09/29 23:15:00, Mark P wrote: > https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... > File base/metrics/histogram_macros.h (left): > > https://codereview.chromium.org/2361933002/diff/1/base/metrics/histogram_macr... > base/metrics/histogram_macros.h:102: #define > STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ > On 2016/09/26 23:06:47, Mark P wrote: > > On 2016/09/26 20:16:10, rkaplow wrote: > > > On 2016/09/23 21:14:12, nikunjb1 wrote: > > > > I see usage of this outside histogram_macros.h e.g > > > > //src/components/metrics/system_memory_stats_recorder_linux.cc > > > > > > > > and > > > > src/chromecast/base/metrics/cast_histograms.h > > > > > > > > Since this is only moved it won't have compilation issues, but is it okay > > keep > > > > them using internal implementations? > > > > > > Probably not ideal - we'd need to see why they were doing this. > > > > > > For example > > > > > > https://codesearch.chromium.org/chromium/src/components/metrics/system_memory... > > > - not sure this was needed. > > > > > > One thing we don't make easy to do is a numeric linear histogram. I wonder > if > > we > > > should add that for cases where people might want it. > > > > > > I'd rather not do in this CL but we could - what do you guys think? > > > > Generally I've been advising people to use ENUMERATION when they want linear > > histograms. (It's only a small encouragement; I don't feel strongly.) It's > > hard to screw up the bucketing if you change things later. > > > > Maybe we should simply add a LINEAR macro instead? > > What do you think about this suggestion? I'm not against it, but I'm not sure how commonly it would be used - and when we should be recommending linear over exponential. One example is allowing stuff like percentiles with courser detailing - with bucket of 5% or something like that. Anyway, seems reasonable to add. I can do it.
Message was sent while issue was closed.
On Fri, Sep 30, 2016 at 8:27 AM, <rkaplow@chromium.org> wrote: > On 2016/09/29 23:15:00, Mark P wrote: > > > https://codereview.chromium.org/2361933002/diff/1/base/ > metrics/histogram_macros.h > > File base/metrics/histogram_macros.h (left): > > > > > https://codereview.chromium.org/2361933002/diff/1/base/ > metrics/histogram_macros.h#oldcode102 > > base/metrics/histogram_macros.h:102: #define > > STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ > > On 2016/09/26 23:06:47, Mark P wrote: > > > On 2016/09/26 20:16:10, rkaplow wrote: > > > > On 2016/09/23 21:14:12, nikunjb1 wrote: > > > > > I see usage of this outside histogram_macros.h e.g > > > > > //src/components/metrics/system_memory_stats_recorder_linux.cc > > > > > > > > > > and > > > > > src/chromecast/base/metrics/cast_histograms.h > > > > > > > > > > Since this is only moved it won't have compilation issues, but is > it > okay > > > keep > > > > > them using internal implementations? > > > > > > > > Probably not ideal - we'd need to see why they were doing this. > > > > > > > > For example > > > > > > > > > > https://codesearch.chromium.org/chromium/src/components/ > metrics/system_memory_stats_recorder_linux.cc?q=STATIC_ > HISTOGRAM_POINTER_BLOCK&sq=package:chromium&dr=C&l=19 > > > > - not sure this was needed. > > > > > > > > One thing we don't make easy to do is a numeric linear histogram. I > wonder > > if > > > we > > > > should add that for cases where people might want it. > > > > > > > > I'd rather not do in this CL but we could - what do you guys think? > > > > > > Generally I've been advising people to use ENUMERATION when they want > linear > > > histograms. (It's only a small encouragement; I don't feel strongly.) > It's > > > hard to screw up the bucketing if you change things later. > > > > > > Maybe we should simply add a LINEAR macro instead? > > > > What do you think about this suggestion? > > I'm not against it, but I'm not sure how commonly it would be used - and > when we > should be recommending linear over exponential. > > One example is allowing stuff like percentiles with courser detailing - > with > bucket of 5% or something like that. > Anyway, seems reasonable to add. I can do it. > I agree that I don't think we should be recommending it, but I think we should be providing a macros that supports it. It always feels weird to be and code writers when I tell people to use the ENUMERATION macro without an enum simply because they want a linear histogram... Thanks preemptively for adding it. I think a linear bucket-size one will suffice (basically an alias for enumerated histograms); I haven't seen as much need to support other bucket sizes. --mark > > https://codereview.chromium.org/2361933002/ > -- 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.
Message was sent while issue was closed.
On 2016/09/30 15:57:47, Mark P wrote: > On Fri, Sep 30, 2016 at 8:27 AM, <mailto:rkaplow@chromium.org> wrote: > > > On 2016/09/29 23:15:00, Mark P wrote: > > > > > https://codereview.chromium.org/2361933002/diff/1/base/ > > metrics/histogram_macros.h > > > File base/metrics/histogram_macros.h (left): > > > > > > > > https://codereview.chromium.org/2361933002/diff/1/base/ > > metrics/histogram_macros.h#oldcode102 > > > base/metrics/histogram_macros.h:102: #define > > > STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name, \ > > > On 2016/09/26 23:06:47, Mark P wrote: > > > > On 2016/09/26 20:16:10, rkaplow wrote: > > > > > On 2016/09/23 21:14:12, nikunjb1 wrote: > > > > > > I see usage of this outside histogram_macros.h e.g > > > > > > //src/components/metrics/system_memory_stats_recorder_linux.cc > > > > > > > > > > > > and > > > > > > src/chromecast/base/metrics/cast_histograms.h > > > > > > > > > > > > Since this is only moved it won't have compilation issues, but is > > it > > okay > > > > keep > > > > > > them using internal implementations? > > > > > > > > > > Probably not ideal - we'd need to see why they were doing this. > > > > > > > > > > For example > > > > > > > > > > > > > > https://codesearch.chromium.org/chromium/src/components/ > > metrics/system_memory_stats_recorder_linux.cc?q=STATIC_ > > HISTOGRAM_POINTER_BLOCK&sq=package:chromium&dr=C&l=19 > > > > > - not sure this was needed. > > > > > > > > > > One thing we don't make easy to do is a numeric linear histogram. I > > wonder > > > if > > > > we > > > > > should add that for cases where people might want it. > > > > > > > > > > I'd rather not do in this CL but we could - what do you guys think? > > > > > > > > Generally I've been advising people to use ENUMERATION when they want > > linear > > > > histograms. (It's only a small encouragement; I don't feel strongly.) > > It's > > > > hard to screw up the bucketing if you change things later. > > > > > > > > Maybe we should simply add a LINEAR macro instead? > > > > > > What do you think about this suggestion? > > > > I'm not against it, but I'm not sure how commonly it would be used - and > > when we > > should be recommending linear over exponential. > > > > One example is allowing stuff like percentiles with courser detailing - > > with > > bucket of 5% or something like that. > > Anyway, seems reasonable to add. I can do it. > > > > I agree that I don't think we should be recommending it, but I think we > should be providing a macros that supports it. It always feels weird to be > and code writers when I tell people to use the ENUMERATION macro without an > enum simply because they want a linear histogram... > > Thanks preemptively for adding it. > > I think a linear bucket-size one will suffice (basically an alias for > enumerated histograms); I haven't seen as much need to support other bucket > sizes. Hey Mark - I was looking into supporting the general use case for linear histograms, since there are a few places where it is used, often for measuring up to a fixed max (like a percentage), but with less granularity. However there are some issues with the suggested parameters which has lead to some incorrect linear histograms. I wrote up some of my thoughts on it here. https://bugs.chromium.org/p/chromium/issues/detail?id=651950 I can still just add a macro for the size-1 version which is still the most common, for now. > > --mark > > > > > > https://codereview.chromium.org/2361933002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
