Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786913004:
Add counter for new shared/array buffers that fail to allocate space. (Closed)
DescriptionAdd counter for new shared/array buffers that fail to allocate space.
Records histogram of ArrayBuffer/SharedArrayBuffer new allocations
that failed because it couldn't allocate space for the
buffer. Histogram is based on the buffer size requested.
This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests.
BUG=chromium:704922
R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org
Review-Url: https://codereview.chromium.org/2786913004
Cr-Commit-Position: refs/heads/master@{#44318}
Committed: https://chromium.googlesource.com/v8/v8/+/9e28cf1318c9ff274ef7599127f7af4cb51b42b5
Patch Set 1 #Patch Set 2 : Clean up counter name. #
Total comments: 6
Patch Set 3 : Move array buffer allocation failure counter #Patch Set 4 : Change to use logorithmic scale on allocation size. #
Total comments: 2
Patch Set 5 : Simplify conversion to log base. #Patch Set 6 : Fix merge conflict. #Messages
Total messages: 36 (22 generated)
Description was changed from ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is base of buffer size requests. BUG=chromium:704922 ========== to ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 ==========
kschimpf@chromium.org changed reviewers: + bbudge@chromium.org, bradnelson@chromium.org
Description was changed from ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 ========== to ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org ==========
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/2786913004/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7686 src/api.cc:7686: static base::Mutex mutex; I don't think you have to worry about locking. V8 Isolates are not thread safe, and clients have to acquire a global lock to use them: https://cs.chromium.org/chromium/src/v8/include/v8.h?rcl=19916f2fc070095a35a9... Checking usage of AddSample, I don't see any locking at those call sites either. https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7695 src/api.cc:7695: } // end of anonymous namespace nit: s/end of anonymous namespace/namespace chromium/v8 convention https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7705 src/api.cc:7705: if (!i::JSArrayBuffer::SetupAllocatingData(obj, i_isolate, byte_length)) { Did you consider making this change in the JSArrayBuffer::SetupAllocatingData function? Then you don't need the helper method. https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=19916f2fc070095a35...
https://codereview.chromium.org/2786913004/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7686 src/api.cc:7686: static base::Mutex mutex; On 2017/03/30 22:38:26, bbudge wrote: > I don't think you have to worry about locking. V8 Isolates are not thread safe, > and clients have to acquire a global lock to use them: > > https://cs.chromium.org/chromium/src/v8/include/v8.h?rcl=19916f2fc070095a35a9... > > Checking usage of AddSample, I don't see any locking at those call sites either. Good to know. I'll remove the lock. https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7695 src/api.cc:7695: } // end of anonymous namespace On 2017/03/30 22:38:26, bbudge wrote: > nit: s/end of anonymous namespace/namespace > > chromium/v8 convention Done. https://codereview.chromium.org/2786913004/diff/20001/src/api.cc#newcode7705 src/api.cc:7705: if (!i::JSArrayBuffer::SetupAllocatingData(obj, i_isolate, byte_length)) { On 2017/03/30 22:38:26, bbudge wrote: > Did you consider making this change in the JSArrayBuffer::SetupAllocatingData > function? Then you don't need the helper method. > > https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=19916f2fc070095a35... No, I had not. But it is a good idea. Moving code there.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still waiting for a review. Hoping to get this in today.
Please review. Note that I have changed the bad allocation size histogram to be logarithmic. This is to make it consistent with the big array buffer allocations CL.
Description was changed from ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org ========== to ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ==========
kschimpf@chromium.org changed reviewers: + mtrofin@chromium.org
Mircea, I'm trying to get this into release 59, and not getting any response from brad/bill. Could you review?
Mircea, I'm trying to get this into release 59, and not getting any response from brad/bill. Could you review?
LGTM w/suggestion same as on other CL for log size. https://codereview.chromium.org/2786913004/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2786913004/diff/60001/src/objects.cc#newcode1... src/objects.cc:19486: : (64 - base::bits::CountLeadingZeros64(allocated_length))); see my comment on the other log size histogram CL.
https://codereview.chromium.org/2786913004/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2786913004/diff/60001/src/objects.cc#newcode1... src/objects.cc:19486: : (64 - base::bits::CountLeadingZeros64(allocated_length))); On 2017/03/31 20:58:38, bbudge wrote: > see my comment on the other log size histogram CL. Done.
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/2786913004/#ps80001 (title: "Simplify conversion to log base.")
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...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/counters.h:
While running git apply --index -3 -p1;
error: patch failed: src/counters.h:928
Falling back to three-way merge...
Applied patch to 'src/counters.h' with conflicts.
U src/counters.h
Patch: src/counters.h
Index: src/counters.h
diff --git a/src/counters.h b/src/counters.h
index
31e45ab46312d909be4ca28498eaed6e903168d4..fc4abe97d8bee97a5da2e24f8c4fa3612322fb69
100644
--- a/src/counters.h
+++ b/src/counters.h
@@ -928,7 +928,8 @@ class RuntimeCallTimerScope {
HR(wasm_functions_per_asm_module, V8.WasmFunctionsPerModule.asm, 1, 100000, \
51) \
HR(wasm_functions_per_wasm_module, V8.WasmFunctionsPerModule.wasm, 1, \
- 100000, 51)
+ 100000, 51) \
+ HR(array_buffer_new_size_failures, V8.ArrayBufferNewSizeFailures, 1, 32, 32)
#define HISTOGRAM_TIMER_LIST(HT)
\
/* Garbage collection timers. */
\
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/2786913004/#ps100001 (title: "Fix merge conflict.")
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": 100001, "attempt_start_ts": 1490996994527560,
"parent_rev": "0c5f5a3acf284d4fffd84fd08efa154cb1a132b5", "commit_rev":
"9e28cf1318c9ff274ef7599127f7af4cb51b42b5"}
Message was sent while issue was closed.
Description was changed from ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ========== to ========== Add counter for new shared/array buffers that fail to allocate space. Records histogram of ArrayBuffer/SharedArrayBuffer new allocations that failed because it couldn't allocate space for the buffer. Histogram is based on the buffer size requested. This counter is intended to give some clue as to how often, and what sizes are being requested. Unfortunately, the how often can't be answered with the current counter. The problem is that V8 doesn't currently support this possibility yet. Hence, for now, introducing a counter that at least counts the number/size of failing requests. BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org Review-Url: https://codereview.chromium.org/2786913004 Cr-Commit-Position: refs/heads/master@{#44318} Committed: https://chromium.googlesource.com/v8/v8/+/9e28cf1318c9ff274ef7599127f7af4cb51... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/9e28cf1318c9ff274ef7599127f7af4cb51... |
