Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
509923003:
about_flags::ReportCustomFlags() should use signed 32-bit IDs. (Closed)
|
Created:
6 years, 3 months ago by Alexander Alekseev Modified:
6 years, 3 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptionabout_flags::ReportCustomFlags() should use signed 32-bit IDs.
On 32-bit architectures UMA Histogram IDs (enum
LoginCustomFlags) are negative. So HistogramBase::Sample
(which is int32_t) should be used.
BUG=408196
TEST=manual
Committed: https://crrev.com/0107c327ff297fad57af3e10253b84862df820d1
Cr-Commit-Position: refs/heads/master@{#293062}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update after review. #Patch Set 3 : Allow negative UMA IDs. #Patch Set 4 : Moved comment to the beginning of enum. #
Total comments: 2
Patch Set 5 : Rebased. #Patch Set 6 : Update after review. #
Total comments: 2
Patch Set 7 : Use base::HistogramBase::Sample instead of int32_t everywhere. #Patch Set 8 : Cleanup. #
Total comments: 4
Patch Set 9 : Cleanup. #
Messages
Total messages: 26 (1 generated)
alemate@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org
Please review: sky@ : chrome/browser/about_flags.cc isherman@ : tools/metrics/histograms/histograms.xml
I don't understand this. 32bit architecture support 32bit unsigned values, right? https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2242: // Need to strip sign bit. You need a better comment. Document 'why' we need to strip the sign bit, not what the code is doing.
https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2243: return static_cast<uint32_t>(metrics::HashMetricName(switch_name)) & Why not just static_cast to <int32_t>? That has the added benefit that you'd maintain consistency with the data that's currently being uploaded.
https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2242: // Need to strip sign bit. On 2014/08/27 20:24:53, sky wrote: > You need a better comment. Document 'why' we need to strip the sign bit, not > what the code is doing. Done. https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2243: return static_cast<uint32_t>(metrics::HashMetricName(switch_name)) & On 2014/08/27 22:05:20, Ilya Sherman wrote: > Why not just static_cast to <int32_t>? That has the added benefit that you'd > maintain consistency with the data that's currently being uploaded. static_cast<int32_t>((uint64_t)(-1LL)) == -1 So I changed the return type, but AND is still needed here.
https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2243: return static_cast<uint32_t>(metrics::HashMetricName(switch_name)) & On 2014/08/28 02:48:55, alemate wrote: > On 2014/08/27 22:05:20, Ilya Sherman wrote: > > Why not just static_cast to <int32_t>? That has the added benefit that you'd > > maintain consistency with the data that's currently being uploaded. > > static_cast<int32_t>((uint64_t)(-1LL)) == -1 > > So I changed the return type, but AND is still needed here. I don't understand the problem. What's wrong with reporting -1?
https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:2243: return static_cast<uint32_t>(metrics::HashMetricName(switch_name)) & On 2014/08/28 03:49:57, Ilya Sherman wrote: > On 2014/08/28 02:48:55, alemate wrote: > > On 2014/08/27 22:05:20, Ilya Sherman wrote: > > > Why not just static_cast to <int32_t>? That has the added benefit that > you'd > > > maintain consistency with the data that's currently being uploaded. > > > > static_cast<int32_t>((uint64_t)(-1LL)) == -1 > > > > So I changed the return type, but AND is still needed here. > > I don't understand the problem. What's wrong with reporting -1? Except that it is unnatural, nothing wrong. Done. PS: BTW, pretty-print.py formats comments inside enum with negative values incorrectly. So this histograms.xml is uploaded with bypass-hooks option.
Sorry, I still don't get the problem here. Why can't 32bit report a 32bit unsigned value? On Thu, Aug 28, 2014 at 7:40 AM, <alemate@chromium.org> wrote: > > https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.cc > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/509923003/diff/1/chrome/browser/about_flags.c... > chrome/browser/about_flags.cc:2243: return > static_cast<uint32_t>(metrics::HashMetricName(switch_name)) & > On 2014/08/28 03:49:57, Ilya Sherman wrote: >> >> On 2014/08/28 02:48:55, alemate wrote: >> > On 2014/08/27 22:05:20, Ilya Sherman wrote: >> > > Why not just static_cast to <int32_t>? That has the added benefit > > that >> >> you'd >> > > maintain consistency with the data that's currently being > > uploaded. >> >> > >> > static_cast<int32_t>((uint64_t)(-1LL)) == -1 >> > >> > So I changed the return type, but AND is still needed here. > > >> I don't understand the problem. What's wrong with reporting -1? > > > Except that it is unnatural, nothing wrong. Done. > > PS: BTW, pretty-print.py formats comments inside enum with negative > values incorrectly. So this histograms.xml is uploaded with bypass-hooks > option. > > https://codereview.chromium.org/509923003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/28 14:40:52, alemate wrote: > PS: BTW, pretty-print.py formats comments inside enum with negative values > incorrectly. So this histograms.xml is uploaded with bypass-hooks option. Please don't do that. You'll just make the next person to edit histograms.xml have to clean up after you. What is the problem you encountered with how pretty-print.py formats comments inside an enum with negative values? Can you update the script to address the issue? On 2014/08/28 15:49:34, sky wrote: > Sorry, I still don't get the problem here. Why can't 32bit report a > 32bit unsigned value? The issue is that histogram buckets are signed integers: https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...
> On 2014/08/28 15:49:34, sky wrote: > > Sorry, I still don't get the problem here. Why can't 32bit report a > > 32bit unsigned value? > > The issue is that histogram buckets are signed integers: > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... And we get different values depending on the architecture (positive on 64-bit and negative on 32-bit if ID > 2^32-1).
On Thu, Aug 28, 2014 at 10:45 AM, <alemate@chromium.org> wrote: >> On 2014/08/28 15:49:34, sky wrote: >> > Sorry, I still don't get the problem here. Why can't 32bit report a >> > 32bit unsigned value? > > >> The issue is that histogram buckets are signed integers: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > And we get different values depending on the architecture (positive on > 64-bit > and negative on 32-bit if ID > 2^32-1). If we're uploading HistogramBase::Sample, shouldn't it be a fixed size and not varying depending upon the architecture? -Scott To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/28 19:04:03, sky wrote: > On Thu, Aug 28, 2014 at 10:45 AM, <mailto:alemate@chromium.org> wrote: > >> On 2014/08/28 15:49:34, sky wrote: > >> > Sorry, I still don't get the problem here. Why can't 32bit report a > >> > 32bit unsigned value? > > > > > >> The issue is that histogram buckets are signed integers: > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > > > And we get different values depending on the architecture (positive on > > 64-bit > > and negative on 32-bit if ID > 2^32-1). I don't think that's true -- |int| is 32-bit on both 32- and 64-bit architectures, AFAIK. > If we're uploading HistogramBase::Sample, shouldn't it be a fixed size > and not varying depending upon the architecture? Yes, this was an oversight. I landed a change yesterday to use int32_t rather than just in.
On Thu, Aug 28, 2014 at 12:10 PM, <isherman@chromium.org> wrote: > On 2014/08/28 19:04:03, sky wrote: > >> On Thu, Aug 28, 2014 at 10:45 AM, <mailto:alemate@chromium.org> wrote: >> >> On 2014/08/28 15:49:34, sky wrote: >> >> > Sorry, I still don't get the problem here. Why can't 32bit report a >> >> > 32bit unsigned value? >> > >> > >> >> The issue is that histogram buckets are signed integers: >> > >> > >> > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... >> >> > >> > And we get different values depending on the architecture (positive on >> > 64-bit >> > and negative on 32-bit if ID > 2^32-1). > > > I don't think that's true -- |int| is 32-bit on both 32- and 64-bit > architectures, AFAIK. > > >> If we're uploading HistogramBase::Sample, shouldn't it be a fixed size >> and not varying depending upon the architecture? > > > Yes, this was an oversight. I landed a change yesterday to use int32_t > rather > than just in. Does that mean nothing needs to be done here? -Scott > > https://codereview.chromium.org/509923003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/28 23:05:24, sky wrote: > On Thu, Aug 28, 2014 at 12:10 PM, <mailto:isherman@chromium.org> wrote: > > On 2014/08/28 19:04:03, sky wrote: > > > >> On Thu, Aug 28, 2014 at 10:45 AM, <mailto:alemate@chromium.org> wrote: > >> >> On 2014/08/28 15:49:34, sky wrote: > >> >> > Sorry, I still don't get the problem here. Why can't 32bit report a > >> >> > 32bit unsigned value? > >> > > >> > > >> >> The issue is that histogram buckets are signed integers: > >> > > >> > > >> > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > >> > >> > > >> > And we get different values depending on the architecture (positive on > >> > 64-bit > >> > and negative on 32-bit if ID > 2^32-1). > > > > > > I don't think that's true -- |int| is 32-bit on both 32- and 64-bit > > architectures, AFAIK. > > > > > >> If we're uploading HistogramBase::Sample, shouldn't it be a fixed size > >> and not varying depending upon the architecture? > > > > > > Yes, this was an oversight. I landed a change yesterday to use int32_t > > rather > > than just in. > > Does that mean nothing needs to be done here? Nope. Histograms expect signed integers for bucket ids, so this client code should use signed integers. This won't affect the correctness of the uploaded data, since the unsigned ints will just implicitly be converted to signed ones. However, it's important to at least fill histograms.xml with the correct mapping, which should include negative bucket ids. It's probably best for code cleanliness not to rely on an implicit conversion from unsigned to signed anywhere in this code, which is what this CL is fixing.
https://codereview.chromium.org/509923003/diff/60001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/60001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2241: int32_t GetSwitchUMAId(const std::string& switch_name) { Shouldn't you use the same types as used by histogram, by which I mean a common typedef?
Please review. On 2014/08/28 17:36:27, Ilya Sherman wrote: > On 2014/08/28 14:40:52, alemate wrote: > > PS: BTW, pretty-print.py formats comments inside enum with negative values > > incorrectly. So this histograms.xml is uploaded with bypass-hooks option. > > Please don't do that. You'll just make the next person to edit histograms.xml > have to clean up after you. > > What is the problem you encountered with how pretty-print.py formats comments > inside an enum with negative values? Can you update the script to address the > issue? I've updated script to keep comments with the next xml node when sorting enums. https://codereview.chromium.org/509923003/diff/60001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/509923003/diff/60001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2241: int32_t GetSwitchUMAId(const std::string& switch_name) { On 2014/08/28 23:27:29, sky wrote: > Shouldn't you use the same types as used by histogram, by which I mean a common > typedef? Done.
https://codereview.chromium.org/509923003/diff/100001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/509923003/diff/100001/chrome/browser/about_fl... chrome/browser/about_flags.h:174: int32_t GetSwitchUMAId(const std::string& switch_name); How come you aren't using base::HistogramBase::Sample here?
https://codereview.chromium.org/509923003/diff/100001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/509923003/diff/100001/chrome/browser/about_fl... chrome/browser/about_flags.h:174: int32_t GetSwitchUMAId(const std::string& switch_name); On 2014/09/02 17:13:22, sky wrote: > How come you aren't using base::HistogramBase::Sample here? Done. Sorry, I missed it.
LGTM
LGTM % nits. Thanks. https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/pretty_print.py (right): https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:84: unset_node_keys_indexes = [] nit: Perhaps something like "pending_node_indices" or something like that? At least, definitely "indices", and "keys" shouldn't be plural if "indices is". https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:96: # Therefore we delay setting key before next node is found. nit: "before next node is found" -> "until the next node is found"
https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/pretty_print.py (right): https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:84: unset_node_keys_indexes = [] On 2014/09/02 22:29:48, Ilya Sherman wrote: > nit: Perhaps something like "pending_node_indices" or something like that? At > least, definitely "indices", and "keys" shouldn't be plural if "indices is". Done. https://codereview.chromium.org/509923003/diff/140001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:96: # Therefore we delay setting key before next node is found. On 2014/09/02 22:29:48, Ilya Sherman wrote: > nit: "before next node is found" -> "until the next node is found" Done.
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/509923003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 8006292fc8c41df06a6e2b12741c605eafbf5665
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0107c327ff297fad57af3e10253b84862df820d1 Cr-Commit-Position: refs/heads/master@{#293062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
