Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795463002:
Add histograms to track issues with JavaScript array buffers. (Closed)
DescriptionAdd histograms to track issues with JavaScript array buffers.
There has been concerns about that WASM could be using very large
Javascript array buffers (including shared array buffers). Currently,
there is no visibility to check this presumption.
This CL is to (at least temporarily) add to UMA histograms to get a
better understanding of this potential issue. It creates two
histograms. The first shows the frequency of allocating array buffers
of at least size 1Mb. The other shows the relative sizes of array
buffers when the memory request is unable to be met.
See the following for the corresponding V8 changes:
Large buffers: https://codereview.chromium.org/2792623002
New failures: https://codereview.chromium.org/2786913004
Fix to megabtye sizes: https://codereview.chromium.org/2795763002
BUG=chromium:704922
R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@chromium.org
Review-Url: https://codereview.chromium.org/2795463002
Cr-Commit-Position: refs/heads/master@{#461780}
Committed: https://chromium.googlesource.com/chromium/src/+/c69cbb716c31aa6fc635f52d314781d35222b73f
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix mispelling #
Total comments: 5
Patch Set 3 : Fix some issues with the histogram descriptions. #Patch Set 4 : Fix array buffer allocations to megabytes. #Messages
Total messages: 22 (11 generated)
Please review. Thanks.
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...
Description was changed from ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org ========== to ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@ch... ==========
kschimpf@chromium.org changed reviewers: + mtrofin@chromium.org
Mircea, Same here. Could you review?
LGTM w/nit but you still need histogram OWNER https://codereview.chromium.org/2795463002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795463002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75269: + meet the corresonding request. nit: corresponding
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
https://codereview.chromium.org/2795463002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795463002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:75269: + meet the corresonding request. On 2017/03/31 20:56:06, bbudge wrote: > nit: corresponding Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Waiting for approval from OWNER isherman.
Please don't land the C++ code for recording to histograms without getting histograms.xml reviewed. Sometimes -- as in this case -- your metrics reviewer will raise concerns about the structure of the histogram data itself. https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75253: +<histogram name="V8.ArrayBufferBigAllocations" units="log2(Size)"> Why do you record log2 of the size? Histogram buckets are already exponentially spaced. Also, the size in what units? https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75259: + allocations over 1Mb. Why does the histogram only track allocations over 1MB? It seems simple enough to toss the rest into an underflow bucket. https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75269: + meet the corresponding request. This sentence is a bit confusing. Could you please try to reword it a bit to be clearer?
Let me start out by apologizing for incorrectly committing the other CLs. I did not realize the order things had to be done. The main issue that needs to be resolved is that of how size is measured. As mentioned below, I used the log2(bytes) measurement because of issues with the length to an array buffer. If you strongly feel that I should create a new CL to fix the sizes to be integers, I will do that and correct the descriptions in the xml file. https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75253: +<histogram name="V8.ArrayBufferBigAllocations" units="log2(Size)"> On 2017/03/31 22:00:36, Ilya Sherman wrote: > Why do you record log2 of the size? Histogram buckets are already exponentially > spaced. Also, the size in what units? I switched to log2 size (in bytes) because the representation used by V8 to collect histograms uses type "int" while the size of the request is size_t. This disallows numbers greater than 2**31 which can possibly happen with WASM modules. Since the code in V8 for counters is used in many places with an "int" assumption, I did not want to change things so close to the 59 release. On the other hand, we would like some feedback on these concepts as soon as possible. Updating the units to log2(bytes). https://codereview.chromium.org/2795463002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75259: + allocations over 1Mb. On 2017/03/31 22:00:36, Ilya Sherman wrote: > Why does the histogram only track allocations over 1MB? It seems simple enough > to toss the rest into an underflow bucket. This is definitely a legitimate answer. I probably worried too much about the overhead of collecting this data. I did not want to slow down buffer array allocations. I also did not want to flood the histogram with a bucket that is so large that visually, it would not be easy to see problem sized allocations (which are large ones). That said, if you still feel that all allocations should be tracked, I will change the code.
Description was changed from ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@ch... ========== to ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 Fix to megabtye sizes: https://codereview.chromium.org/2795763002 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@ch... ==========
Ran D8 and verified data was being collected. Also modified V8 code to collect in megabytes. See CL https://codereview.chromium.org/2795763002 Gotten approval on changes from mtrofin. Waiting for trybots plus approval here before committing. Ready for another review.
Metrics LGTM, thanks.
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/2795463002/#ps60001 (title: "Fix array buffer allocations to megabytes.")
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": 60001, "attempt_start_ts": 1491325314670070,
"parent_rev": "1ba0e9fb97703fd0804f8c8d0b720ec7c9205a6b", "commit_rev":
"c69cbb716c31aa6fc635f52d314781d35222b73f"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 Fix to megabtye sizes: https://codereview.chromium.org/2795763002 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@ch... ========== to ========== Add histograms to track issues with JavaScript array buffers. There has been concerns about that WASM could be using very large Javascript array buffers (including shared array buffers). Currently, there is no visibility to check this presumption. This CL is to (at least temporarily) add to UMA histograms to get a better understanding of this potential issue. It creates two histograms. The first shows the frequency of allocating array buffers of at least size 1Mb. The other shows the relative sizes of array buffers when the memory request is unable to be met. See the following for the corresponding V8 changes: Large buffers: https://codereview.chromium.org/2792623002 New failures: https://codereview.chromium.org/2786913004 Fix to megabtye sizes: https://codereview.chromium.org/2795763002 BUG=chromium:704922 R=bbudge@chromium.org,brannelson@chromium.or,isherman@chromium.org,mtrofin@ch... Review-Url: https://codereview.chromium.org/2795463002 Cr-Commit-Position: refs/heads/master@{#461780} Committed: https://chromium.googlesource.com/chromium/src/+/c69cbb716c31aa6fc635f52d3147... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c69cbb716c31aa6fc635f52d3147... |
