Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898953007:
Disallow non-sparse histograms with too many buckets. (Closed)
|
Created:
3 years, 7 months ago by Alexei Svitkine (slow) Modified:
3 years, 7 months ago Reviewers:
bcwhite CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow non-sparse histograms with too many buckets.
Now that we have an optimization in place where we don't
allocate the full array until more than 1 bucket is used,
it's too easy to do something ridiculous like pass max
int32 to the bucket_count param without noticing it causing
a problem during local testing.
BUG=726342
Review-Url: https://codereview.chromium.org/2898953007
Cr-Commit-Position: refs/heads/master@{#474753}
Committed: https://chromium.googlesource.com/chromium/src/+/c49943dc741398caa352ace1d15c8fdf313fe942
Patch Set 1 #Patch Set 2 : 10002 #
Total comments: 2
Messages
Total messages: 18 (13 generated)
The CQ bit was checked by asvitkine@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by asvitkine@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.
Description was changed from ========== Disallow non-sparse histograms with too many buckets. Now that we have an optimization in place where we don't allocate the full array until more than 1 bucket is used, it's too easy to do something ridiculous like pass max int32 to the bucket_count param without noticing it causing a problem during local testing. BUG= ========== to ========== Disallow non-sparse histograms with too many buckets. Now that we have an optimization in place where we don't allocate the full array until more than 1 bucket is used, it's too easy to do something ridiculous like pass max int32 to the bucket_count param without noticing it causing a problem during local testing. BUG=726342 ==========
asvitkine@chromium.org changed reviewers: + bcwhite@chromium.org
lgtm https://codereview.chromium.org/2898953007/diff/20001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2898953007/diff/20001/base/metrics/histogram.... base/metrics/histogram.cc:405: constexpr uint32_t kMaxBucketCount = 10002; Even 10k sounds really, really large.
https://codereview.chromium.org/2898953007/diff/20001/base/metrics/histogram.cc File base/metrics/histogram.cc (right): https://codereview.chromium.org/2898953007/diff/20001/base/metrics/histogram.... base/metrics/histogram.cc:405: constexpr uint32_t kMaxBucketCount = 10002; On 2017/05/25 19:21:41, bcwhite wrote: > Even 10k sounds really, really large. Yeah agreed. There was a unit test that was using 10k so I picked a limit that doesn't require other changes for now. We can reduce it later - in code reviews I usually push back on anything 100 in size or more when I see it. :)
The CQ bit was checked by asvitkine@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": 20001, "attempt_start_ts": 1495740248190840,
"parent_rev": "13157f21a3e66cad40850e65c741795144817f2f", "commit_rev":
"c49943dc741398caa352ace1d15c8fdf313fe942"}
Message was sent while issue was closed.
Description was changed from ========== Disallow non-sparse histograms with too many buckets. Now that we have an optimization in place where we don't allocate the full array until more than 1 bucket is used, it's too easy to do something ridiculous like pass max int32 to the bucket_count param without noticing it causing a problem during local testing. BUG=726342 ========== to ========== Disallow non-sparse histograms with too many buckets. Now that we have an optimization in place where we don't allocate the full array until more than 1 bucket is used, it's too easy to do something ridiculous like pass max int32 to the bucket_count param without noticing it causing a problem during local testing. BUG=726342 Review-Url: https://codereview.chromium.org/2898953007 Cr-Commit-Position: refs/heads/master@{#474753} Committed: https://chromium.googlesource.com/chromium/src/+/c49943dc741398caa352ace1d15c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c49943dc741398caa352ace1d15c... |
