|
|
Created:
3 years, 9 months ago by khmel Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, khmel+watch_chromium.org, asvitkine+watch_chromium.org, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), abarth-chromium, davemoore+watch_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Skip GMS Sign-In in case Android is already signed-in.
There are users' feedback reports where GMS Sign-In was attempted on
already signed account. This operation for re-sign in general is
safe. However Play Store may be busy with downloading and initializing.
This is the reason of real user failures with GMS Sign-In timeout.
We need detect this case and skip GMS Sign-in in case user is already
signed-in. This can happen on retry when Android was actually signed in
but failed later due another reason.
TBR=lhchavez@chromium.org
BUG=704982
TEST=Manually various cases and observe result in logcat and
chrome://histogramms
Review-Url: https://codereview.chromium.org/2771943003
Cr-Commit-Position: refs/heads/master@{#460030}
Committed: https://chromium.googlesource.com/chromium/src/+/eb71d90d60b690242af7c4fd623238071ecefba7
Patch Set 1 #
Total comments: 2
Patch Set 2 : comment updated #
Total comments: 14
Patch Set 3 : nits #Patch Set 4 : nit #Patch Set 5 : del transient enum #
Total comments: 1
Patch Set 6 : remove SIZE from mojom and use LAST_VALUE + 1 in UMA reporting #
Total comments: 10
Patch Set 7 : nits #Patch Set 8 : use sparse UMA #
Messages
Total messages: 49 (15 generated)
Description was changed from ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms ========== to ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms ==========
khmel@chromium.org changed reviewers: + lhchavez@chromium.org
Hi Luis, PTAL, Thanks!
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1651: + not recorded in case account status check failed for some reason. nit: "This is not recorded in case of account status check failure." do you plan to investigate why this happens?
khmel@chromium.org changed reviewers: + dcheng@chromium.org, isherman@chromium.org
Thank you Luis for comments Also adding isherman@ for UMA dcheng@ for mojo PTAL, Thanks! https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771943003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:1651: + not recorded in case account status check failed for some reason. On 2017/03/24 22:02:32, Luis Héctor Chávez wrote: > nit: "This is not recorded in case of account status check failure." > > do you plan to investigate why this happens? I don't expect this happens in real time (probably timeout in case of CPU overloaded). If I see noticeable number of such errors I will take a look. At this stage it is important for me % of normal flow.
Metrics LGTM, thanks. https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:122: enum class AccountCheckStatus : int { Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2771943003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771943003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81530: + <summary>Defines ARC account check statuses</summary> nit: I don't think this <summary> text is used anywhere, so it's fine to omit. (It's also fine to keep, if you prefer it for whatever reason.)
Thank you Ilya for review! https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:122: enum class AccountCheckStatus : int { On 2017/03/24 23:22:08, Ilya Sherman wrote: > Please document that this enum is used to back an UMA histogram, and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2771943003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771943003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:81530: + <summary>Defines ARC account check statuses</summary> On 2017/03/24 23:22:09, Ilya Sherman wrote: > nit: I don't think this <summary> text is used anywhere, so it's fine to omit. > (It's also fine to keep, if you prefer it for whatever reason.) Good point!
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:226: NOTREACHED() << "Unknown account check status"; Please typemap this enum instead =) (Or just use the mojo enum everywhere) https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:70: static_cast<int>(AccountCheckStatus::SIZE)); Hmm... this doesn't work without the static cast? (Guess not, filed https://crbug.com/705171 for fixing this) https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, Another possibility is to use the mojo enum throughout, but I guess this makes it hard. +yzshen, do you think it would make sense to make it easier to use enums for UMA? The tricky part is that the UMA histogram macro expects size to be strictly greater than the max possible value, rather than greater than or equal to...
Thank you for your comments https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:226: NOTREACHED() << "Unknown account check status"; On 2017/03/25 00:55:04, dcheng wrote: > Please typemap this enum instead =) > > (Or just use the mojo enum everywhere) Not sure if I got you correctly. I added << status; https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:70: static_cast<int>(AccountCheckStatus::SIZE)); On 2017/03/25 00:55:04, dcheng wrote: > Hmm... this doesn't work without the static cast? > > (Guess not, filed https://crbug.com/705171 for fixing this) Thank for caring this case! I confirmed that this does not compile. https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/25 00:55:04, dcheng wrote: > Another possibility is to use the mojo enum throughout, but I guess this makes > it hard. +yzshen, do you think it would make sense to make it easier to use > enums for UMA? The tricky part is that the UMA histogram macro expects size to > be strictly greater than the max possible value, rather than greater than or > equal to... This good discussion. But we already use similar mapping technique. Would it possible to keep it similar style so far?
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/25 00:55:04, dcheng wrote: > Another possibility is to use the mojo enum throughout, but I guess this makes > it hard. +yzshen, do you think it would make sense to make it easier to use > enums for UMA? The tricky part is that the UMA histogram macro expects size to > be strictly greater than the max possible value, rather than greater than or > equal to... (Sorry for late reply! I didn't notice this earlier. Please feel free to ping next time if I didn't reply in 24 hours.) It would be nice to have one less type. But I noticed the two relevant bug reports (cannot use enum class with UMA macro; max needs to be larger). It probably makes sense to have a separate type before those are resolved. Otherwise, we would have to do static_casts, and also have a unnecessary value in the mojom enum.
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 16:53:05, yzshen1 wrote: > On 2017/03/25 00:55:04, dcheng wrote: > > Another possibility is to use the mojo enum throughout, but I guess this makes > > it hard. +yzshen, do you think it would make sense to make it easier to use > > enums for UMA? The tricky part is that the UMA histogram macro expects size to > > be strictly greater than the max possible value, rather than greater than or > > equal to... > > (Sorry for late reply! I didn't notice this earlier. Please feel free to ping > next time if I didn't reply in 24 hours.) > > It would be nice to have one less type. But I noticed the two relevant bug > reports (cannot use enum class with UMA macro; max needs to be larger). It > probably makes sense to have a separate type before those are resolved. > Otherwise, we would have to do static_casts, and also have a unnecessary value > in the mojom enum. > FWIW, you don't have to add a new value to the mojom enum, you can just write "max + 1" for the histogram macro invocation. The static casts would indeed be needed, though.
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 17:00:40, Ilya Sherman wrote: > On 2017/03/27 16:53:05, yzshen1 wrote: > > On 2017/03/25 00:55:04, dcheng wrote: > > > Another possibility is to use the mojo enum throughout, but I guess this > makes > > > it hard. +yzshen, do you think it would make sense to make it easier to use > > > enums for UMA? The tricky part is that the UMA histogram macro expects size > to > > > be strictly greater than the max possible value, rather than greater than or > > > equal to... > > > > (Sorry for late reply! I didn't notice this earlier. Please feel free to ping > > next time if I didn't reply in 24 hours.) > > > > It would be nice to have one less type. But I noticed the two relevant bug > > reports (cannot use enum class with UMA macro; max needs to be larger). It > > probably makes sense to have a separate type before those are resolved. > > Otherwise, we would have to do static_casts, and also have a unnecessary value > > in the mojom enum. > > > > FWIW, you don't have to add a new value to the mojom enum, you can just write > "max + 1" for the histogram macro invocation. The static casts would indeed be > needed, though. WRT the "max + 1" approach: if "max" is not named explicitly as "max", I imagine it is possible that people update the mojom definition to append new enum values, but forget to update the macro accordingly.
https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, On 2017/03/27 17:07:46, yzshen1 wrote: > On 2017/03/27 17:00:40, Ilya Sherman wrote: > > On 2017/03/27 16:53:05, yzshen1 wrote: > > > On 2017/03/25 00:55:04, dcheng wrote: > > > > Another possibility is to use the mojo enum throughout, but I guess this > > makes > > > > it hard. +yzshen, do you think it would make sense to make it easier to > use > > > > enums for UMA? The tricky part is that the UMA histogram macro expects > size > > to > > > > be strictly greater than the max possible value, rather than greater than > or > > > > equal to... > > > > > > (Sorry for late reply! I didn't notice this earlier. Please feel free to > ping > > > next time if I didn't reply in 24 hours.) > > > > > > It would be nice to have one less type. But I noticed the two relevant bug > > > reports (cannot use enum class with UMA macro; max needs to be larger). It > > > probably makes sense to have a separate type before those are resolved. > > > Otherwise, we would have to do static_casts, and also have a unnecessary > value > > > in the mojom enum. > > > > > > > FWIW, you don't have to add a new value to the mojom enum, you can just write > > "max + 1" for the histogram macro invocation. The static casts would indeed > be > > needed, though. > > WRT the "max + 1" approach: if "max" is not named explicitly as "max", I imagine > it is possible that people update the mojom definition to append new enum > values, but forget to update the macro accordingly. Thank you for you comments! What is about including SIZE into mojom? This way we can protect extending enum. WDYT?
On 2017/03/27 17:17:39, khmel wrote: > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_optin_uma.h (right): > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, > On 2017/03/27 17:07:46, yzshen1 wrote: > > On 2017/03/27 17:00:40, Ilya Sherman wrote: > > > On 2017/03/27 16:53:05, yzshen1 wrote: > > > > On 2017/03/25 00:55:04, dcheng wrote: > > > > > Another possibility is to use the mojo enum throughout, but I guess this > > > makes > > > > > it hard. +yzshen, do you think it would make sense to make it easier to > > use > > > > > enums for UMA? The tricky part is that the UMA histogram macro expects > > size > > > to > > > > > be strictly greater than the max possible value, rather than greater > than > > or > > > > > equal to... > > > > > > > > (Sorry for late reply! I didn't notice this earlier. Please feel free to > > ping > > > > next time if I didn't reply in 24 hours.) > > > > > > > > It would be nice to have one less type. But I noticed the two relevant bug > > > > reports (cannot use enum class with UMA macro; max needs to be larger). It > > > > probably makes sense to have a separate type before those are resolved. > > > > Otherwise, we would have to do static_casts, and also have a unnecessary > > value > > > > in the mojom enum. > > > > > > > > > > FWIW, you don't have to add a new value to the mojom enum, you can just > write > > > "max + 1" for the histogram macro invocation. The static casts would indeed > > be > > > needed, though. > > > > WRT the "max + 1" approach: if "max" is not named explicitly as "max", I > imagine > > it is possible that people update the mojom definition to append new enum > > values, but forget to update the macro accordingly. > > Thank you for you comments! What is about including SIZE into mojom? This way > we can protect extending enum. WDYT? If the value is not supposed to be used in any mojo messages, I personally prefer we don't add it to the mojom. But I don't have a very strong opinion here. :)
On 2017/03/27 17:43:22, yzshen1 wrote: > On 2017/03/27 17:17:39, khmel wrote: > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_optin_uma.h (right): > > > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, > > On 2017/03/27 17:07:46, yzshen1 wrote: > > > On 2017/03/27 17:00:40, Ilya Sherman wrote: > > > > On 2017/03/27 16:53:05, yzshen1 wrote: > > > > > On 2017/03/25 00:55:04, dcheng wrote: > > > > > > Another possibility is to use the mojo enum throughout, but I guess > this > > > > makes > > > > > > it hard. +yzshen, do you think it would make sense to make it easier > to > > > use > > > > > > enums for UMA? The tricky part is that the UMA histogram macro expects > > > size > > > > to > > > > > > be strictly greater than the max possible value, rather than greater > > than > > > or > > > > > > equal to... > > > > > > > > > > (Sorry for late reply! I didn't notice this earlier. Please feel free to > > > ping > > > > > next time if I didn't reply in 24 hours.) > > > > > > > > > > It would be nice to have one less type. But I noticed the two relevant > bug > > > > > reports (cannot use enum class with UMA macro; max needs to be larger). > It > > > > > probably makes sense to have a separate type before those are resolved. > > > > > Otherwise, we would have to do static_casts, and also have a unnecessary > > > value > > > > > in the mojom enum. > > > > > > > > > > > > > FWIW, you don't have to add a new value to the mojom enum, you can just > > write > > > > "max + 1" for the histogram macro invocation. The static casts would > indeed > > > be > > > > needed, though. > > > > > > WRT the "max + 1" approach: if "max" is not named explicitly as "max", I > > imagine > > > it is possible that people update the mojom definition to append new enum > > > values, but forget to update the macro accordingly. > > > > Thank you for you comments! What is about including SIZE into mojom? This way > > we can protect extending enum. WDYT? > > If the value is not supposed to be used in any mojo messages, I personally > prefer we don't add it to the mojom. > But I don't have a very strong opinion here. :) Well :) Consider that removing one proxy enum for UMA is good reason we have 2 (or more) choices. 1) - define SIZE in mojom and use it for UMA call. 2) use static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1. I am OK with both. Let me know your preferences. We can add comment in mojo in case 2 for next enum update to update UMA reporting as well.
On 2017/03/27 17:54:56, khmel wrote: > On 2017/03/27 17:43:22, yzshen1 wrote: > > On 2017/03/27 17:17:39, khmel wrote: > > > > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/arc/arc_optin_uma.h (right): > > > > > > > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, > > > On 2017/03/27 17:07:46, yzshen1 wrote: > > > > On 2017/03/27 17:00:40, Ilya Sherman wrote: > > > > > On 2017/03/27 16:53:05, yzshen1 wrote: > > > > > > On 2017/03/25 00:55:04, dcheng wrote: > > > > > > > Another possibility is to use the mojo enum throughout, but I guess > > this > > > > > makes > > > > > > > it hard. +yzshen, do you think it would make sense to make it easier > > to > > > > use > > > > > > > enums for UMA? The tricky part is that the UMA histogram macro > expects > > > > size > > > > > to > > > > > > > be strictly greater than the max possible value, rather than greater > > > than > > > > or > > > > > > > equal to... > > > > > > > > > > > > (Sorry for late reply! I didn't notice this earlier. Please feel free > to > > > > ping > > > > > > next time if I didn't reply in 24 hours.) > > > > > > > > > > > > It would be nice to have one less type. But I noticed the two relevant > > bug > > > > > > reports (cannot use enum class with UMA macro; max needs to be > larger). > > It > > > > > > probably makes sense to have a separate type before those are > resolved. > > > > > > Otherwise, we would have to do static_casts, and also have a > unnecessary > > > > value > > > > > > in the mojom enum. > > > > > > > > > > > > > > > > FWIW, you don't have to add a new value to the mojom enum, you can just > > > write > > > > > "max + 1" for the histogram macro invocation. The static casts would > > indeed > > > > be > > > > > needed, though. > > > > > > > > WRT the "max + 1" approach: if "max" is not named explicitly as "max", I > > > imagine > > > > it is possible that people update the mojom definition to append new enum > > > > values, but forget to update the macro accordingly. > > > > > > Thank you for you comments! What is about including SIZE into mojom? This > way > > > we can protect extending enum. WDYT? > > > > If the value is not supposed to be used in any mojo messages, I personally > > prefer we don't add it to the mojom. > > But I don't have a very strong opinion here. :) > > Well :) Consider that removing one proxy enum for UMA is good reason we have 2 > (or more) choices. 1) - define SIZE in mojom and use it for UMA call. 2) use > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1. I am OK with > both. Let me know your preferences. We can add comment in mojo in case 2 for > next enum update to update UMA reporting as well. I would vote for (2) + comments in mojom!
On 2017/03/27 18:04:51, yzshen1 wrote: > On 2017/03/27 17:54:56, khmel wrote: > > On 2017/03/27 17:43:22, yzshen1 wrote: > > > On 2017/03/27 17:17:39, khmel wrote: > > > > > > > > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > > > File chrome/browser/chromeos/arc/arc_optin_uma.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2771943003/diff/20001/chrome/browser/chromeos... > > > > chrome/browser/chromeos/arc/arc_optin_uma.h:139: SIZE, > > > > On 2017/03/27 17:07:46, yzshen1 wrote: > > > > > On 2017/03/27 17:00:40, Ilya Sherman wrote: > > > > > > On 2017/03/27 16:53:05, yzshen1 wrote: > > > > > > > On 2017/03/25 00:55:04, dcheng wrote: > > > > > > > > Another possibility is to use the mojo enum throughout, but I > guess > > > this > > > > > > makes > > > > > > > > it hard. +yzshen, do you think it would make sense to make it > easier > > > to > > > > > use > > > > > > > > enums for UMA? The tricky part is that the UMA histogram macro > > expects > > > > > size > > > > > > to > > > > > > > > be strictly greater than the max possible value, rather than > greater > > > > than > > > > > or > > > > > > > > equal to... > > > > > > > > > > > > > > (Sorry for late reply! I didn't notice this earlier. Please feel > free > > to > > > > > ping > > > > > > > next time if I didn't reply in 24 hours.) > > > > > > > > > > > > > > It would be nice to have one less type. But I noticed the two > relevant > > > bug > > > > > > > reports (cannot use enum class with UMA macro; max needs to be > > larger). > > > It > > > > > > > probably makes sense to have a separate type before those are > > resolved. > > > > > > > Otherwise, we would have to do static_casts, and also have a > > unnecessary > > > > > value > > > > > > > in the mojom enum. > > > > > > > > > > > > > > > > > > > FWIW, you don't have to add a new value to the mojom enum, you can > just > > > > write > > > > > > "max + 1" for the histogram macro invocation. The static casts would > > > indeed > > > > > be > > > > > > needed, though. > > > > > > > > > > WRT the "max + 1" approach: if "max" is not named explicitly as "max", I > > > > imagine > > > > > it is possible that people update the mojom definition to append new > enum > > > > > values, but forget to update the macro accordingly. > > > > > > > > Thank you for you comments! What is about including SIZE into mojom? This > > way > > > > we can protect extending enum. WDYT? > > > > > > If the value is not supposed to be used in any mojo messages, I personally > > > prefer we don't add it to the mojom. > > > But I don't have a very strong opinion here. :) > > > > Well :) Consider that removing one proxy enum for UMA is good reason we have 2 > > (or more) choices. 1) - define SIZE in mojom and use it for UMA call. 2) use > > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1. I am OK with > > both. Let me know your preferences. We can add comment in mojo in case 2 for > > next enum update to update UMA reporting as well. > > I would vote for (2) + comments in mojom! Thanks, Done.
LGTM I have a CL in progress to fix the scoped enum issue: https://codereview.chromium.org/2776853002/. I also have some ideas on how to fix the max issue, then we can come back and clean this up. Maybe add a TODO that points to the max issue: https://crbug.com/705169 (no need to TODO the static_casts: I can find these and fix them pretty easily later on) https://codereview.chromium.org/2771943003/diff/80001/components/arc/common/a... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2771943003/diff/80001/components/arc/common/a... components/arc/common/auth.mojom:89: [MinVersion=9] SIZE = 5, (Perhaps mention that this should never be sent across the wire)
LGTM for using mojom enum with UMA.
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:68: DCHECK(status <= mojom::AccountCheckStatus::CHECK_FAILED); nit: DCHECK_LE https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); Hmm, I'm a bit nervous about requiring anyone who edits components/arc/common/auth.mojom to edit this file as well -- it seems rather fragile. Would be great to come up with a design pattern that doesn't require that -- both for this CL, and more generally to set a precedent for other such situations. Does Mojo not validate that enum constants fall in the appropriate range? If it does, how does it determine the correct range? FWIW, another option is to use a sparse histogram, which does not require an explicit maximum value to be passed to it. https://codereview.chromium.org/2771943003/diff/100001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2771943003/diff/100001/components/arc/common/... components/arc/common/auth.mojom:70: // have to update UMA reporting in Chrome. See nit: I'd phrase the second sentence as something like "NOTE: If you add any entries to this enum, you must also update the corresponding UMA callsite in Chrome." I'd also suggest moving that comment to the end of the enum, rather than the top, though I find that readers are likely to miss this sort of comment in either case.
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:68: DCHECK(status <= mojom::AccountCheckStatus::CHECK_FAILED); On 2017/03/27 19:03:28, Ilya Sherman wrote: > nit: DCHECK_LE Done. https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 19:03:28, Ilya Sherman wrote: > Hmm, I'm a bit nervous about requiring anyone who edits > components/arc/common/auth.mojom to edit this file as well -- it seems rather > fragile. Would be great to come up with a design pattern that doesn't require > that -- both for this CL, and more generally to set a precedent for other such > situations. Does Mojo not validate that enum constants fall in the appropriate > range? If it does, how does it determine the correct range? > > FWIW, another option is to use a sparse histogram, which does not require an > explicit maximum value to be passed to it. IIUC enum in mojom is rather constants (from my experience ARC++ mojom). It seems sparse histogram is used for sparsely distributed values, as followed from comments. Probably it is not the best to use it here. https://codereview.chromium.org/2771943003/diff/100001/components/arc/common/... File components/arc/common/auth.mojom (right): https://codereview.chromium.org/2771943003/diff/100001/components/arc/common/... components/arc/common/auth.mojom:70: // have to update UMA reporting in Chrome. See On 2017/03/27 19:03:28, Ilya Sherman wrote: > nit: I'd phrase the second sentence as something like "NOTE: If you add any > entries to this enum, you must also update the corresponding UMA callsite in > Chrome." I'd also suggest moving that comment to the end of the enum, rather > than the top, though I find that readers are likely to miss this sort of comment > in either case. Makes sense, thanks!
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 19:29:29, khmel wrote: > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > Hmm, I'm a bit nervous about requiring anyone who edits > > components/arc/common/auth.mojom to edit this file as well -- it seems rather > > fragile. Would be great to come up with a design pattern that doesn't require > > that -- both for this CL, and more generally to set a precedent for other such > > situations. Does Mojo not validate that enum constants fall in the > appropriate > > range? If it does, how does it determine the correct range? > > > > FWIW, another option is to use a sparse histogram, which does not require an > > explicit maximum value to be passed to it. > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). I'm not following what this means. Aren't all enums essentially constants? My question is: How does Mojo validate deserialized enum values? Does it simply not validate them? > It seems sparse histogram is used for sparsely distributed values, as followed from > comments. Probably it is not the best to use it here. I'm not necessarily pushing for it, but I would like to better understand: What's the disadvantage of using a sparse histogram here?
On 2017/03/27 20:07:37, Ilya Sherman wrote: > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_optin_uma.cc:71: > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); > On 2017/03/27 19:29:29, khmel wrote: > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > components/arc/common/auth.mojom to edit this file as well -- it seems > rather > > > fragile. Would be great to come up with a design pattern that doesn't > require > > > that -- both for this CL, and more generally to set a precedent for other > such > > > situations. Does Mojo not validate that enum constants fall in the > > appropriate > > > range? If it does, how does it determine the correct range? > > > > > > FWIW, another option is to use a sparse histogram, which does not require an > > > explicit maximum value to be passed to it. > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > I'm not following what this means. Aren't all enums essentially constants? My > question is: How does Mojo validate deserialized enum values? Does it simply > not validate them? If an enum is marked as "extensible", then yes, no validation is done by mojo. (But you can use EnumTraits to do type conversion / validation if needed.) Otherwise, any unknown value is rejected (by discarding the message and disconnect the message pipe). > > > It seems sparse histogram is used for sparsely distributed values, as followed > from > > comments. Probably it is not the best to use it here. > > I'm not necessarily pushing for it, but I would like to better understand: > What's the disadvantage of using a sparse histogram here?
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 20:07:37, Ilya Sherman wrote: > On 2017/03/27 19:29:29, khmel wrote: > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > components/arc/common/auth.mojom to edit this file as well -- it seems > rather > > > fragile. Would be great to come up with a design pattern that doesn't > require > > > that -- both for this CL, and more generally to set a precedent for other > such > > > situations. Does Mojo not validate that enum constants fall in the > > appropriate > > > range? If it does, how does it determine the correct range? > > > > > > FWIW, another option is to use a sparse histogram, which does not require an > > > explicit maximum value to be passed to it. > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > I'm not following what this means. Aren't all enums essentially constants? My > question is: How does Mojo validate deserialized enum values? Does it simply > not validate them? > > > It seems sparse histogram is used for sparsely distributed values, as followed > from > > comments. Probably it is not the best to use it here. > > I'm not necessarily pushing for it, but I would like to better understand: > What's the disadvantage of using a sparse histogram here? I got feeling that is slower than 'enum' histogram. however I am relying here on you opinion.
On 2017/03/27 20:35:57, khmel wrote: > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_optin_uma.cc:71: > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > On 2017/03/27 19:29:29, khmel wrote: > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > components/arc/common/auth.mojom to edit this file as well -- it seems > > rather > > > > fragile. Would be great to come up with a design pattern that doesn't > > require > > > > that -- both for this CL, and more generally to set a precedent for other > > such > > > > situations. Does Mojo not validate that enum constants fall in the > > > appropriate > > > > range? If it does, how does it determine the correct range? > > > > > > > > FWIW, another option is to use a sparse histogram, which does not require > an > > > > explicit maximum value to be passed to it. > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > > > I'm not following what this means. Aren't all enums essentially constants? > My > > question is: How does Mojo validate deserialized enum values? Does it simply > > not validate them? > > > > > It seems sparse histogram is used for sparsely distributed values, as > followed > > from > > > comments. Probably it is not the best to use it here. > > > > I'm not necessarily pushing for it, but I would like to better understand: > > What's the disadvantage of using a sparse histogram here? > > I got feeling that is slower than 'enum' histogram. however I am relying here on > you opinion. If we're recording an enumeration to a histogram, doesn't it make sense to use UMA_HISTOGRAM_ENUMERATION? And if that's not the case, then maybe we should update the documentation to reflect best practices?
Apologies, Yury, for getting a bit into the weeds on this CL. I do think it's important to set a good precedent for how UMA and Mojo enumerations should interact, and at the same time I realize that it's probably annoying for you to be blocked on this discussion. So, sorry about that! Hopefully we'll come to a consensus soon. On 2017/03/27 20:37:53, dcheng wrote: > If we're recording an enumeration to a histogram, doesn't it make sense to use > UMA_HISTOGRAM_ENUMERATION? And if that's not the case, then maybe we should > update the documentation to reflect best practices? So, depending on the specifics, either UMA_HISTOGRAM_ENUMERATION or UMA_HISTOGRAM_SPARSE_SLOWLY is the best choice for recording an enumeration to a histogram. UMA_HISTOGRAM_ENUMERATION is the fastest implementation; on the other hand, it's got a clumsier API, and trades off memory for performance. UMA_HISTOGRAM_SPARSE_SLOWLY is slightly slower (backed by a map rather than a vector, and therefore uses a lock to guard memory access); on the other hand, it doesn't require an explicit max param, and it's often more memory-efficient. For most code, dealing with relatively small and straightforward enumerations, either choice is probably fine -- the implementation differences just don't matter all that much in practice in most cases. https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 20:35:57, khmel wrote: > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > On 2017/03/27 19:29:29, khmel wrote: > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > components/arc/common/auth.mojom to edit this file as well -- it seems > > rather > > > > fragile. Would be great to come up with a design pattern that doesn't > > require > > > > that -- both for this CL, and more generally to set a precedent for other > > such > > > > situations. Does Mojo not validate that enum constants fall in the > > > appropriate > > > > range? If it does, how does it determine the correct range? > > > > > > > > FWIW, another option is to use a sparse histogram, which does not require > an > > > > explicit maximum value to be passed to it. > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > > > I'm not following what this means. Aren't all enums essentially constants? > My > > question is: How does Mojo validate deserialized enum values? Does it simply > > not validate them? > > > > > It seems sparse histogram is used for sparsely distributed values, as > followed > > from > > > comments. Probably it is not the best to use it here. > > > > I'm not necessarily pushing for it, but I would like to better understand: > > What's the disadvantage of using a sparse histogram here? > > I got feeling that is slower than 'enum' histogram. however I am relying here on > you opinion. It is a bit slower, yes. It's probably not a meaningful performance difference, though: UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no locks, caches the histogram pointer in a static local variable. UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and doesn't cache the pointer. These differences probably don't matter too much here -- though perhaps I am mistaken, and this is highly performance-sensitive code?
On 2017/03/27 20:54:29, Ilya Sherman wrote: > Apologies, Yury, for getting a bit into the weeds on this CL. I do think it's > important to set a good precedent for how UMA and Mojo enumerations should > interact, and at the same time I realize that it's probably annoying for you to > be blocked on this discussion. So, sorry about that! Hopefully we'll come to a > consensus soon. > > On 2017/03/27 20:37:53, dcheng wrote: > > If we're recording an enumeration to a histogram, doesn't it make sense to use > > UMA_HISTOGRAM_ENUMERATION? And if that's not the case, then maybe we should > > update the documentation to reflect best practices? > > So, depending on the specifics, either UMA_HISTOGRAM_ENUMERATION or > UMA_HISTOGRAM_SPARSE_SLOWLY is the best choice for recording an enumeration to a > histogram. UMA_HISTOGRAM_ENUMERATION is the fastest implementation; on the > other hand, it's got a clumsier API, and trades off memory for performance. > UMA_HISTOGRAM_SPARSE_SLOWLY is slightly slower (backed by a map rather than a > vector, and therefore uses a lock to guard memory access); on the other hand, it > doesn't require an explicit max param, and it's often more memory-efficient. > For most code, dealing with relatively small and straightforward enumerations, > either choice is probably fine -- the implementation differences just don't > matter all that much in practice in most cases. It feels like UMA_HISTOGRAM_ENUMERATION should just handle the efficiency details internally, switching between implementations automatically if the enum range exceeds some threshhold. > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_optin_uma.cc:71: > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); > On 2017/03/27 20:35:57, khmel wrote: > > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > > On 2017/03/27 19:29:29, khmel wrote: > > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > > components/arc/common/auth.mojom to edit this file as well -- it seems > > > rather > > > > > fragile. Would be great to come up with a design pattern that doesn't > > > require > > > > > that -- both for this CL, and more generally to set a precedent for > other > > > such > > > > > situations. Does Mojo not validate that enum constants fall in the > > > > appropriate > > > > > range? If it does, how does it determine the correct range? > > > > > > > > > > FWIW, another option is to use a sparse histogram, which does not > require > > an > > > > > explicit maximum value to be passed to it. > > > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > > > > > I'm not following what this means. Aren't all enums essentially constants? > > My > > > question is: How does Mojo validate deserialized enum values? Does it > simply > > > not validate them? > > > > > > > It seems sparse histogram is used for sparsely distributed values, as > > followed > > > from > > > > comments. Probably it is not the best to use it here. > > > > > > I'm not necessarily pushing for it, but I would like to better understand: > > > What's the disadvantage of using a sparse histogram here? > > > > I got feeling that is slower than 'enum' histogram. however I am relying here > on > > you opinion. > > It is a bit slower, yes. It's probably not a meaningful performance difference, > though: UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no locks, > caches the histogram pointer in a static local variable. > UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and > doesn't cache the pointer. These differences probably don't matter too much > here -- though perhaps I am mistaken, and this is highly performance-sensitive > code? I think the bigger issue is that we have a macro that's ostensibly for enumerations, but apparently (sometimes) isn't =)
https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_optin_uma.cc:71: static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); On 2017/03/27 20:54:29, Ilya Sherman wrote: > On 2017/03/27 20:35:57, khmel wrote: > > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > > On 2017/03/27 19:29:29, khmel wrote: > > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > > components/arc/common/auth.mojom to edit this file as well -- it seems > > > rather > > > > > fragile. Would be great to come up with a design pattern that doesn't > > > require > > > > > that -- both for this CL, and more generally to set a precedent for > other > > > such > > > > > situations. Does Mojo not validate that enum constants fall in the > > > > appropriate > > > > > range? If it does, how does it determine the correct range? > > > > > > > > > > FWIW, another option is to use a sparse histogram, which does not > require > > an > > > > > explicit maximum value to be passed to it. > > > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > > > > > I'm not following what this means. Aren't all enums essentially constants? > > My > > > question is: How does Mojo validate deserialized enum values? Does it > simply > > > not validate them? > > > > > > > It seems sparse histogram is used for sparsely distributed values, as > > followed > > > from > > > > comments. Probably it is not the best to use it here. > > > > > > I'm not necessarily pushing for it, but I would like to better understand: > > > What's the disadvantage of using a sparse histogram here? > > > > I got feeling that is slower than 'enum' histogram. however I am relying here > on > > you opinion. > > It is a bit slower, yes. It's probably not a meaningful performance difference, > though: UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no locks, > caches the histogram pointer in a static local variable. > UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and > doesn't cache the pointer. These differences probably don't matter too much > here -- though perhaps I am mistaken, and this is highly performance-sensitive > code? This code is invoked very infrequently. So sparse histogram should be OK. And thank you all for your idea! I am totally fine to have it discussed here.
This CL LGTM with a sparse histogram -- and you can remove the note in the mojom file if using a sparse histogram =) That said, we might want to wait for the discussion in http://crbug.com/705169 to settle on a conclusion. (Well, actually, we could always update the code after that settles; so I'm okay with unblocking this CL and continuing the discussion on the bug.) On 2017/03/27 21:00:50, dcheng wrote: > On 2017/03/27 20:54:29, Ilya Sherman wrote: > > Apologies, Yury, for getting a bit into the weeds on this CL. I do think it's > > important to set a good precedent for how UMA and Mojo enumerations should > > interact, and at the same time I realize that it's probably annoying for you > to > > be blocked on this discussion. So, sorry about that! Hopefully we'll come to > a > > consensus soon. > > > > On 2017/03/27 20:37:53, dcheng wrote: > > > If we're recording an enumeration to a histogram, doesn't it make sense to > use > > > UMA_HISTOGRAM_ENUMERATION? And if that's not the case, then maybe we should > > > update the documentation to reflect best practices? > > > > So, depending on the specifics, either UMA_HISTOGRAM_ENUMERATION or > > UMA_HISTOGRAM_SPARSE_SLOWLY is the best choice for recording an enumeration to > a > > histogram. UMA_HISTOGRAM_ENUMERATION is the fastest implementation; on the > > other hand, it's got a clumsier API, and trades off memory for performance. > > UMA_HISTOGRAM_SPARSE_SLOWLY is slightly slower (backed by a map rather than a > > vector, and therefore uses a lock to guard memory access); on the other hand, > it > > doesn't require an explicit max param, and it's often more memory-efficient. > > For most code, dealing with relatively small and straightforward enumerations, > > either choice is probably fine -- the implementation differences just don't > > matter all that much in practice in most cases. > > It feels like UMA_HISTOGRAM_ENUMERATION should just handle the efficiency > details internally, switching between implementations automatically if the enum > range exceeds some threshhold. Primarily, it's not a matter of what range the enum value is in; it's a matter of whether the macro is used in a tight loop. In case you've ever wondered why we even use macros rather than functions for UMA histograms, it's so that we can squeeze out a bit of extra performance by caching the histogram pointer in a static local variable. Given how optimized this code is, I don't think it's appropriate to add branching factors like choosing an implementation based on the range. > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > > > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_optin_uma.cc:71: > > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); > > On 2017/03/27 20:35:57, khmel wrote: > > > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > > > On 2017/03/27 19:29:29, khmel wrote: > > > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > > > components/arc/common/auth.mojom to edit this file as well -- it seems > > > > rather > > > > > > fragile. Would be great to come up with a design pattern that doesn't > > > > require > > > > > > that -- both for this CL, and more generally to set a precedent for > > other > > > > such > > > > > > situations. Does Mojo not validate that enum constants fall in the > > > > > appropriate > > > > > > range? If it does, how does it determine the correct range? > > > > > > > > > > > > FWIW, another option is to use a sparse histogram, which does not > > require > > > an > > > > > > explicit maximum value to be passed to it. > > > > > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ mojom). > > > > > > > > I'm not following what this means. Aren't all enums essentially > constants? > > > My > > > > question is: How does Mojo validate deserialized enum values? Does it > > simply > > > > not validate them? > > > > > > > > > It seems sparse histogram is used for sparsely distributed values, as > > > followed > > > > from > > > > > comments. Probably it is not the best to use it here. > > > > > > > > I'm not necessarily pushing for it, but I would like to better understand: > > > > What's the disadvantage of using a sparse histogram here? > > > > > > I got feeling that is slower than 'enum' histogram. however I am relying > here > > on > > > you opinion. > > > > It is a bit slower, yes. It's probably not a meaningful performance > difference, > > though: UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no > locks, > > caches the histogram pointer in a static local variable. > > UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and > > doesn't cache the pointer. These differences probably don't matter too much > > here -- though perhaps I am mistaken, and this is highly performance-sensitive > > code? > > I think the bigger issue is that we have a macro that's ostensibly for > enumerations, but apparently (sometimes) isn't =) Well, the macro *is* for enumerations =) But, there are multiple macros that can be used for enumerations. It definitely seems to be adding some confusion on this particular CL that there are multiple ways to accomplish more or less the same thing. In general, UMA macros are designed to be blazing fast, so that it's safe to use them in a tight loop. The exception to this rule is th the sparse histogram macro, which probably shouldn't be a macro in the first place. We could plausibly change this design decision, but I'm not convinced that the cost of doing such a major refactoring is worth the benefits.
On 2017/03/27 21:19:12, Ilya Sherman wrote: > This CL LGTM with a sparse histogram -- and you can remove the note in the mojom > file if using a sparse histogram =) > > That said, we might want to wait for the discussion in http://crbug.com/705169 > to settle on a conclusion. (Well, actually, we could always update the code > after that settles; so I'm okay with unblocking this CL and continuing the > discussion on the bug.) +1, no need to block this CL > > On 2017/03/27 21:00:50, dcheng wrote: > > On 2017/03/27 20:54:29, Ilya Sherman wrote: > > > Apologies, Yury, for getting a bit into the weeds on this CL. I do think > it's > > > important to set a good precedent for how UMA and Mojo enumerations should > > > interact, and at the same time I realize that it's probably annoying for you > > to > > > be blocked on this discussion. So, sorry about that! Hopefully we'll come > to > > a > > > consensus soon. > > > > > > On 2017/03/27 20:37:53, dcheng wrote: > > > > If we're recording an enumeration to a histogram, doesn't it make sense to > > use > > > > UMA_HISTOGRAM_ENUMERATION? And if that's not the case, then maybe we > should > > > > update the documentation to reflect best practices? > > > > > > So, depending on the specifics, either UMA_HISTOGRAM_ENUMERATION or > > > UMA_HISTOGRAM_SPARSE_SLOWLY is the best choice for recording an enumeration > to > > a > > > histogram. UMA_HISTOGRAM_ENUMERATION is the fastest implementation; on the > > > other hand, it's got a clumsier API, and trades off memory for performance. > > > UMA_HISTOGRAM_SPARSE_SLOWLY is slightly slower (backed by a map rather than > a > > > vector, and therefore uses a lock to guard memory access); on the other > hand, > > it > > > doesn't require an explicit max param, and it's often more memory-efficient. > > > > For most code, dealing with relatively small and straightforward > enumerations, > > > either choice is probably fine -- the implementation differences just don't > > > matter all that much in practice in most cases. > > > > It feels like UMA_HISTOGRAM_ENUMERATION should just handle the efficiency > > details internally, switching between implementations automatically if the > enum > > range exceeds some threshhold. > > Primarily, it's not a matter of what range the enum value is in; it's a matter > of whether the macro is used in a tight loop. In case you've ever wondered why > we even use macros rather than functions for UMA histograms, it's so that we can > squeeze out a bit of extra performance by caching the histogram pointer in a > static local variable. Given how optimized this code is, I don't think it's > appropriate to add branching factors like choosing an implementation based on > the range. The selection would be made at compile-time. > > > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > > > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > > > > > > > > https://codereview.chromium.org/2771943003/diff/100001/chrome/browser/chromeo... > > > chrome/browser/chromeos/arc/arc_optin_uma.cc:71: > > > static_cast<int>(mojom::AccountCheckStatus::CHECK_FAILED) + 1); > > > On 2017/03/27 20:35:57, khmel wrote: > > > > On 2017/03/27 20:07:37, Ilya Sherman wrote: > > > > > On 2017/03/27 19:29:29, khmel wrote: > > > > > > On 2017/03/27 19:03:28, Ilya Sherman wrote: > > > > > > > Hmm, I'm a bit nervous about requiring anyone who edits > > > > > > > components/arc/common/auth.mojom to edit this file as well -- it > seems > > > > > rather > > > > > > > fragile. Would be great to come up with a design pattern that > doesn't > > > > > require > > > > > > > that -- both for this CL, and more generally to set a precedent for > > > other > > > > > such > > > > > > > situations. Does Mojo not validate that enum constants fall in the > > > > > > appropriate > > > > > > > range? If it does, how does it determine the correct range? > > > > > > > > > > > > > > FWIW, another option is to use a sparse histogram, which does not > > > require > > > > an > > > > > > > explicit maximum value to be passed to it. > > > > > > > > > > > > IIUC enum in mojom is rather constants (from my experience ARC++ > mojom). > > > > > > > > > > I'm not following what this means. Aren't all enums essentially > > constants? > > > > My > > > > > question is: How does Mojo validate deserialized enum values? Does it > > > simply > > > > > not validate them? > > > > > > > > > > > It seems sparse histogram is used for sparsely distributed values, as > > > > followed > > > > > from > > > > > > comments. Probably it is not the best to use it here. > > > > > > > > > > I'm not necessarily pushing for it, but I would like to better > understand: > > > > > What's the disadvantage of using a sparse histogram here? > > > > > > > > I got feeling that is slower than 'enum' histogram. however I am relying > > here > > > on > > > > you opinion. > > > > > > It is a bit slower, yes. It's probably not a meaningful performance > > difference, > > > though: UMA_HISTOGRAM_ENUMERATION uses a vector-based implementation, no > > locks, > > > caches the histogram pointer in a static local variable. > > > UMA_HISTOGRAM_SPARSE_SLOWLY uses a map-based implementation, with locks, and > > > doesn't cache the pointer. These differences probably don't matter too much > > > here -- though perhaps I am mistaken, and this is highly > performance-sensitive > > > code? > > > > I think the bigger issue is that we have a macro that's ostensibly for > > enumerations, but apparently (sometimes) isn't =) > > Well, the macro *is* for enumerations =) But, there are multiple macros that > can be used for enumerations. It definitely seems to be adding some confusion > on this particular CL that there are multiple ways to accomplish more or less > the same thing. > > In general, UMA macros are designed to be blazing fast, so that it's safe to use > them in a tight loop. The exception to this rule is th the sparse histogram > macro, which probably shouldn't be a macro in the first place. We could > plausibly change this design decision, but I'm not convinced that the cost of > doing such a major refactoring is worth the benefits.
Description was changed from ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms ========== to ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. TBR=lhchavez@chromium.org BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms ==========
Thank you all for review! Luis does not have access to the Chromium currently and I got his permission offline. Added TBR.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2771943003/#ps140001 (title: "use sparse UMA")
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 khmel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490673981637650, "parent_rev": "5f7dd4a95099ce81d3253c949cf31896ce0d62c2", "commit_rev": "eb71d90d60b690242af7c4fd623238071ecefba7"}
Message was sent while issue was closed.
Description was changed from ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. TBR=lhchavez@chromium.org BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms ========== to ========== arc: Skip GMS Sign-In in case Android is already signed-in. There are users' feedback reports where GMS Sign-In was attempted on already signed account. This operation for re-sign in general is safe. However Play Store may be busy with downloading and initializing. This is the reason of real user failures with GMS Sign-In timeout. We need detect this case and skip GMS Sign-in in case user is already signed-in. This can happen on retry when Android was actually signed in but failed later due another reason. TBR=lhchavez@chromium.org BUG=704982 TEST=Manually various cases and observe result in logcat and chrome://histogramms Review-Url: https://codereview.chromium.org/2771943003 Cr-Commit-Position: refs/heads/master@{#460030} Committed: https://chromium.googlesource.com/chromium/src/+/eb71d90d60b690242af7c4fd6232... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/eb71d90d60b690242af7c4fd6232... |