Note that this would purely be an internal accounting change and shouldn't actually impact any distribution.

This would be so much work to change, given the number of invocations of this macro in the codebase... I'm not convinced that it's worth doing.

FWIW, the existing structure has a pro as well as the cons that you listed: keeping the "max" up to date is easier, because the compiler handles the numbering automagically.

That said, we can leave this bug open if you like.  If anyone is actually willing to implement this change, I guess I wouldn't necessarily be opposed to it.  (Would need to consider it a bit more, TBH.  Enums used for histograms are relatively rarely also used in switch stmts, so the main advantage would be consistency with IPC.)

If we use max = largest used enum like this:
enum MyEnum {
  kFirst,
  kSecond,
  kThird,
  kMax = kThird,
};

We'd need to increment the max inside UMA_HISTOGRAM_ENUMERATION() to guarantee the overflow bucket is never used, right?

Correct. I believe it's easier to just change it so max has to be explicitly set: this is consistent with what the IPC macros expect, and lets us avoid the issue of having sentinel values that should never actually be serialized/deserialized by IPC. I see two possible approaches:
- Just unconditionally increment, and slowly go through and audit usages. Fix any that don't use the correct convention.
- Add a new macro and port things over.

I'm kind of in favor of the first (we could even write something to help us keep track of incorrect |boundary| usage), since I think it'd be confusing to have two macros, but I'm open to ideas =)

I'm not a fan of the first. It will cause all the arrays backing buckets to increase by 1 - which will be cause users to use more memory. (Histograms already account for too much memory use and we want to decrease this.)

And second it will create a bunch of crufty code that doesn't follow best practices - and that code will likely be copied by people as they write CLs - causing more burden for histograms reviewers.

So I think we should either leave things as-is or if we convert things, it should be done all at once in some automated fashion.

Simply increasing bucket count by 1 is an intermediate state; the end goal is to fix all usage, using tooling to help find any remaining violations so everything is consistent. The timeline would be ~1 week. I don't think the cost of doing that is too high, and the burden to histogram reviewers should be minimal once the conversion is done?

Doing this in one atomic CL seems impractical: there are ~1700 references to UMA_HISTOGRAM_ENUMERATION throughout the codebase.

Unconditionally incrementing should be fine, since the downside of this would be one more unused bucket in some cases.

However, it looks like we have lots of enums to clean up, even with aids from tools. How bad is it to keep one more entry in IPC serialization? Binary size bloat in the generated (de)serialization logic? If so, we can focus on those enums used in IPC macros, and only change the max item there to decrease the amount of work.

It's not binary bloat; it's that it makes it harder to reason about the various conditions that should hold for a given type over IPC. See https://codereview.chromium.org/2771943003/, where there's been some back and forth on how to best handle this for a Mojo enum.

In that mojo example, the code is fragile if the |max| or |size| is not named so.
https://codereview.chromium.org/2771943003/#msg18

It's no standard way to get the max of enum, but is there a way to get the max in mojom enum? If so, one solution would be using a new macro that gets the max automatically for IPC enums.

Mojo enums are generated, so Mojo could easily just generate a max value with a known name (in some way that won't collide with the other value names).

To clarify, I don't have a problem with fully converting everything within one week - it doesn't have to be one CL. My point is a state where things are only partially converted is unacceptable - i.e. it's fine to be in it for a short period of time like a week (as long as a branch point doesn't happen in this interval) - but not more than that.

It's still not obvious to me that the effort required is worth the benefits. Why not just keep things as-is?

I agree that if the main objective is to make it easier to record Mojo enums to UMA, it would be nice to simply provide a wrapper macro that automagically does the right thing for those enums.  (And, I extra-much agree that the pattern currently suggested in https://codereview.chromium.org/2771943003/ -- having the max be implicit -- is very fragile; IMO it's an anti-pattern.)

After thinking more about this bug, I don't think that we should change the semantics of the UMA_HISTOGRAM_ENUMERATION macro.  It's not that hard to write code like:

enum MyEnum {
  kFirst,
  kSecond,
  kThird,
  kMax = kThird,
};
UMA_HISTOGRAM_ENUMERATION("MyEnum", value, MyEnum::kMax + 1);

In code that I review, I strongly suggest using "max" to mean "last element" and "count" to mean "one-past the last element".  For most enums used with UMA histograms, which are neither involved in switch stmts nor sent via IPC, the "count" pattern is more convenient than the "max" pattern.

Re: why change it
Because it's a continual source of confusion. It doesn't happen very often, but ironically, that makes it less well-known and easier to miss.

Re: kMax + 1
This doesn't work without casts if the enum is scoped (which is the general recommendation for all new enumerations).

Re: count vs max
I think having count and max is kind of confusing. I can see the argument for convenience, but updating MAX hasn't been a problem in practice (you'll notice very quickly if you forget), whereas the opposite problem (forgetting that you need to explicitly not handle the sentinel COUNT value) is a much more silent problem.

Thanks Daniel, I think you're making good points.  I still kind of disagree, though =)  In particular, I'm unclear on your comment about max not being a problem in practice: How will I notice very quickly if I forget? And, how is it a silent problem if I forgot to not explicitly handle the sentinel COUNT value?

Re: noticing
I would hope people are testing their changes. =)
I know that for me, when I make a histogram change, I check that it has the expected effect on chrome://histograms.

I did some more thinking and I guess it's probably not a silent problem: assuming you're not using the default label in switch statements, the compiler will complain. So it really comes down to the extra code noise and ipc auditability: switches have extra case statements to NOTREACHED() the sentinel value, IPC needs to assert that it never tries to send the sentinel over IPC, etc.

This isn't /too/ bad if it's just scoped to enums for UMA. But so far, it seems like there's not much consensus on how to handle Mojo. Two possibilities:

1. Mojo enums that are intended for use with UMAs require a special annotation. Mojo would know how to generate the sentinel value and ensure it's never serialized/deserialized.

Not sure if there are predictability issues here. Forgetting the annotation would lead to brittle code that depends on code reviewers to notice. But only Mojo enums used in UMAs have the extra COUNT sentinel to handle.

2. Alternatively, all Mojo enums have a sentinel max and/or count value.

However, using the COUNT sentinel, then all switches on Mojo enums will need the extra case statement. However, taking the MAX sentinel makes maintainability of non-Mojo enums more difficult.

Regarding maintainability... we do have a clang plugin check that kMax is the largest value in the enumeration (but never enabled it). Would having a compile time check like this make people more comfortable with using MAX instead of COUNT?

Would it make sense to just generate a separate constant (not in the enum) EnumName_COUNT for all Mojo enums? The constant could be constexpr and thus hopefully not introduce any bloat. Then, if the enum would be used in a UMA macro, that special constant could be passed.

This way, you don't need to add any complexity of annotating certain constants and not others and the values will always be there if they're needed.

There was a long survey of what to do for enums in the past (I think by torne@) and at least in the code that I work in, it was decided to go with

enum Thing {
kFirst,
kSecond,
kThird,
kLastThing = kThird,
};

Specifically the *last* keyword was used throughout, and we explicitly did not add another value, because as you say that requires adding it to switches which is terrible times.

Because the kLastThing is literally the line beside any code changes in the enum that would affect it, it has not been a problem with kLastThing being incorrect a single time in the last few years since we started doing this.

FWIW, I do think that having a compile-time check that enforced correct semantics for kMax/kLast would alleviate a lot of my concerns.  I also agree with Alexei that we could probably generate something convenient for Mojo enums that doesn't make the enum more painful to use outside of the UMA context.

I filed issue 742517 to explore the idea of Mojo generating a separate constant for enum histogram boundary purposes.

(While I would personally still prefer to change how the boundary condition works, this would likely make things simpler for developers without having to resolve this bug =)

FWIW, I just ran into this in a non-mojo context here:
  https://chromium-review.googlesource.com/c/chromium/src/+/742227/8/chrome/browser/profiling_host/profiling_process_host.cc#607

The hilarity of the situation is kCount only exists for UMA, but:

1. It makes the primary logic for the enum (this switch) more annoying since we have a weird kCount value which has no semantic meaning other than to make the UMA api work.

2. You cannot leave out this case statement as clang will complain.

3. You cannot insert this case statement at the end just before the closing } since clang will complain.

4. If you insert a ; for a blank statement, clang format will reformat your code to:
   case: Mode::kCount:;

5. If you add a comment to prevent the line merging, clang format will incorrectly indent by 4 spaces:

   case: Mode::kCount:
       // Fall through to hit DLOG(FATAL) below.
       ;

6. If you miss that bad indentation (I did), lint will complain that you should use {} instead of ; for empty statements.

7. If you do all this, in debug mode, clang will generate sentinel code that does domain checks for > max enum value (as defined by spec 7.2.7) before entering the switch so you do actually get runtime protection.

Thus you end up with:
    case Mode::kCount:
      // Fall through to hit DLOG(FATAL) below.
      {}
  }

to handle an expanded enumeration domain that you didn't want anyways.



Agreed that it's work to shift things over. That might be too much work and I could accept that argument to leave things as is. However, I'm not sure I can buy the other arguments that forcing a kCount value on enums is a good idea for the codebase...

I thought we settled on kLast instead of kCount for these reasons. We def did in the gpu world.

enum { 
 A = 3,
 B,
 C,
 kLastThing = C
}

It's acceptable to use either kCount or kLast.  We recommend kCount by default because then there's no need to remember the "+ 1" or "kLast + 1" when recording to an enum macro; but it's also fine to use kLast for code that prefers it.  Just be careful to pass the correct boundary value to the enum macro!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8358f64279ca99b8abf8444ada936796c1659a5

commit d8358f64279ca99b8abf8444ada936796c1659a5
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Mar 30 18:36:58 2018

Update chrome clang plugin check for enumerators.

Rather than doing fuzzy matching on _LAST/Last, simple look for
kMaxValue. In addition, enforce that the value isn't unique, since that
defeats the entire point of using kMaxValue (avoiding pointless case
statements in switch statements).

Bug: 705169, 742517
Change-Id: I4499db30bf87a345bdc5c94b8d3f488823eb7504
Reviewed-on: https://chromium-review.googlesource.com/986723
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547217}
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/ChromeClassTester.cpp
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/ChromeClassTester.h
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/FindBadConstructsAction.cpp
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/FindBadConstructsConsumer.cpp
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/FindBadConstructsConsumer.h
[modify] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/Options.h
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value.cpp
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value.flags
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value.txt
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value_from_c.c
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value_from_c.flags
[delete] https://crrev.com/3a92f0d98a3b37e6f8ce4f72e8841bb58c36e592/tools/clang/plugins/tests/enum_last_value_from_c.txt
[add] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/tests/enum_max_value.cpp
[add] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/tests/enum_max_value.flags
[add] https://crrev.com/d8358f64279ca99b8abf8444ada936796c1659a5/tools/clang/plugins/tests/enum_max_value.txt

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a67dd55f55a4165c1d69df6fd502e7672e2c556d

commit a67dd55f55a4165c1d69df6fd502e7672e2c556d
Author: Daniel Cheng <dcheng@chromium.org>
Date: Sat Mar 31 02:30:46 2018

Normalize definition of CacheMetrics::kMaxValue.

For consistency, kMaxValue should share the value of the highest enumerator,
not be unique.

This also updates the internal macros for enumeration histograms to allow
flags to be passed through, so it can be reused for both the regular and
the local versions of the histogram macros.

Bug: 705169, 742517
Change-Id: Ia93e18e3a69bd828f4cbc4c7f1020b4cf3f13ca1
Reviewed-on: https://chromium-review.googlesource.com/988678
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547363}
[modify] https://crrev.com/a67dd55f55a4165c1d69df6fd502e7672e2c556d/base/metrics/histogram_macros.h
[modify] https://crrev.com/a67dd55f55a4165c1d69df6fd502e7672e2c556d/base/metrics/histogram_macros_internal.h
[modify] https://crrev.com/a67dd55f55a4165c1d69df6fd502e7672e2c556d/base/metrics/histogram_macros_local.h
[modify] https://crrev.com/a67dd55f55a4165c1d69df6fd502e7672e2c556d/content/renderer/dom_storage/local_storage_cached_areas.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f81237aa7519604c0f788a24cb0f54eb29e377e

commit 0f81237aa7519604c0f788a24cb0f54eb29e377e
Author: Daniel Cheng <dcheng@chromium.org>
Date: Thu Apr 12 00:28:59 2018

Enable chrome clang plugin check for kMaxValue enumerator.

Bug: 705169, 742517
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I105debbc2ccca76bd251d1f068a2fefb8bba3e09
Reviewed-on: https://chromium-review.googlesource.com/1004447
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549975}
[modify] https://crrev.com/0f81237aa7519604c0f788a24cb0f54eb29e377e/base/metrics/histogram_macros_unittest.cc
[modify] https://crrev.com/0f81237aa7519604c0f788a24cb0f54eb29e377e/build/config/clang/BUILD.gn
[modify] https://crrev.com/0f81237aa7519604c0f788a24cb0f54eb29e377e/gpu/ipc/service/gpu_vsync_provider_win.cc