Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792623002:
Track large array buffer allocations. (Closed)
DescriptionTrack large array buffer allocations.
Adds a counter for large array buffers. Used to give an indication of
how common large array buffers are allocated in V8.
For the moment, we assume a 1Mb cutoff for the notion of large array
buffers. We also use log2(length) to cleanly bucket sizes into a
histogram.
BUG=chromium:704922
R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org
Review-Url: https://codereview.chromium.org/2792623002
Cr-Commit-Position: refs/heads/master@{#44317}
Committed: https://chromium.googlesource.com/v8/v8/+/0c5f5a3acf284d4fffd84fd08efa154cb1a132b5
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix issues raised by bbudge #
Total comments: 4
Patch Set 3 : Simplify conversion to log2 value. #Messages
Total messages: 26 (16 generated)
The CQ bit was checked by kschimpf@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...
Please review. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2792623002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2792623002/diff/1/src/counters.h#newcode932 src/counters.h:932: HR(big_array_buffer_allocations, V8.BigArrayBufferAllocations, 1, 32, 32) nit: maybe rename so it's closer to array_buffer_new_size_failures, i.e. array_buffer_big_allocations. https://codereview.chromium.org/2792623002/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2792623002/diff/1/src/objects.cc#newcode19469 src/objects.cc:19469: int log2Value(T value) { Rather than roll your own, you could use the functions in v8/src/base/bits.h (count leading zeroes to get the log) https://cs.chromium.org/chromium/src/v8/src/base/bits.h?rcl=21a7487c06f10629d... https://codereview.chromium.org/2792623002/diff/1/src/objects.cc#newcode19490 src/objects.cc:19490: constexpr size_t MinBigAllocation = 1 << 20; nit: convention is to prefix 'k' to constants
The CQ bit was checked by kschimpf@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...
Fixed noted issues, ready for another review. https://codereview.chromium.org/2792623002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2792623002/diff/1/src/counters.h#newcode932 src/counters.h:932: HR(big_array_buffer_allocations, V8.BigArrayBufferAllocations, 1, 32, 32) On 2017/03/31 19:01:18, bbudge wrote: > nit: maybe rename so it's closer to array_buffer_new_size_failures, i.e. > array_buffer_big_allocations. Good point, fixing names. https://codereview.chromium.org/2792623002/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2792623002/diff/1/src/objects.cc#newcode19469 src/objects.cc:19469: int log2Value(T value) { On 2017/03/31 19:01:18, bbudge wrote: > Rather than roll your own, you could use the functions in v8/src/base/bits.h > (count leading zeroes to get the log) > > https://cs.chromium.org/chromium/src/v8/src/base/bits.h?rcl=21a7487c06f10629d... Thanks for the pointer. I agree that I shouldn't roll my own. However, I was unable to find an integer-based log2, and I didn't think of counting leading zeros. Changing. Question: Should I do this same simplification to bad allocation sizes (in CL https://codereview.chromium.org/2786913004 )? https://codereview.chromium.org/2792623002/diff/1/src/objects.cc#newcode19490 src/objects.cc:19490: constexpr size_t MinBigAllocation = 1 << 20; On 2017/03/31 19:01:18, bbudge wrote: > nit: convention is to prefix 'k' to constants Good point. I suffer from spending too many years in LLVM conventions! Fixing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Track large array buffer allocations. Adds a counter for large array buffers. Used to give an indication of how common large array buffers are allocated in V8. For the moment, we assume a 1Mb cutoff for the notion of large array buffers. We also use log2(length) to cleanly bucket sizes into a histogram. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org ========== to ========== Track large array buffer allocations. Adds a counter for large array buffers. Used to give an indication of how common large array buffers are allocated in V8. For the moment, we assume a 1Mb cutoff for the notion of large array buffers. We also use log2(length) to cleanly bucket sizes into a histogram. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ==========
kschimpf@chromium.org changed reviewers: + mtrofin@chromium.org
Mircea I am trying to get this in to meet the 59 release. Do you have time to review?
LGTM w/suggestion https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc#newcode1... src/objects.cc:19482: : (64 - base::bits::CountLeadingZeros64(allocated_length)))); You could also just do: sizeof(uint64_t) * kBitsPerByte - base::bits::CountLeadingZeros64(allocated_length) - 1 -1 if you wish to map 1 to 0, 2 to 1 etc. I'm not clear on how histograms work for ranges.
https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc#newcode1... src/objects.cc:19482: : (64 - base::bits::CountLeadingZeros64(allocated_length)))); On 2017/03/31 20:52:28, bbudge wrote: > You could also just do: > > sizeof(uint64_t) * kBitsPerByte - > base::bits::CountLeadingZeros64(allocated_length) - 1 > > -1 if you wish to map 1 to 0, 2 to 1 etc. I'm not clear on how histograms work > for ranges. The pointer sizes are something that may be fishable out of globals.h and you can use constants then.
https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc#newcode1... src/objects.cc:19482: : (64 - base::bits::CountLeadingZeros64(allocated_length)))); On 2017/03/31 20:52:28, bbudge wrote: > You could also just do: > > sizeof(uint64_t) * kBitsPerByte - > base::bits::CountLeadingZeros64(allocated_length) - 1 > > -1 if you wish to map 1 to 0, 2 to 1 etc. I'm not clear on how histograms work > for ranges. The recommendation is NOT to use value 0 in UMA histograms since that value is where all out of range data goes. Chrome documentation recommends starting with 1. That is why I did not subtract 1. Simplifying as suggested otherwise. https://codereview.chromium.org/2792623002/diff/20001/src/objects.cc#newcode1... src/objects.cc:19482: : (64 - base::bits::CountLeadingZeros64(allocated_length)))); On 2017/03/31 21:01:38, Mircea Trofin wrote: > On 2017/03/31 20:52:28, bbudge wrote: > > You could also just do: > > > > sizeof(uint64_t) * kBitsPerByte - > > base::bits::CountLeadingZeros64(allocated_length) - 1 > > > > -1 if you wish to map 1 to 0, 2 to 1 etc. I'm not clear on how histograms work > > for ranges. > > The pointer sizes are something that may be fishable out of globals.h and you > can use constants then. size_t is the number of bytes. Using uint64_t, as Bill suggests, fixes the issue.
The CQ bit was checked by kschimpf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/2792623002/#ps40001 (title: "Simplify conversion to log2 value.")
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 kschimpf@chromium.org
The CQ bit was checked by kschimpf@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": 40001, "attempt_start_ts": 1490995628700130,
"parent_rev": "2de2840f2ec238226504fcf05bac6f01cfd2b53d", "commit_rev":
"0c5f5a3acf284d4fffd84fd08efa154cb1a132b5"}
Message was sent while issue was closed.
Description was changed from ========== Track large array buffer allocations. Adds a counter for large array buffers. Used to give an indication of how common large array buffers are allocated in V8. For the moment, we assume a 1Mb cutoff for the notion of large array buffers. We also use log2(length) to cleanly bucket sizes into a histogram. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ========== to ========== Track large array buffer allocations. Adds a counter for large array buffers. Used to give an indication of how common large array buffers are allocated in V8. For the moment, we assume a 1Mb cutoff for the notion of large array buffers. We also use log2(length) to cleanly bucket sizes into a histogram. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org Review-Url: https://codereview.chromium.org/2792623002 Cr-Commit-Position: refs/heads/master@{#44317} Committed: https://chromium.googlesource.com/v8/v8/+/0c5f5a3acf284d4fffd84fd08efa154cb1a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/0c5f5a3acf284d4fffd84fd08efa154cb1a... |
