Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977493003:
Remove accounting from ArrayBuffer::Allocator (Closed)
|
Created:
4 years, 7 months ago by Michael Lippautz Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-wtf_chromium.org, blink-reviews-bindings_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove accounting from ArrayBuffer::Allocator
Memory is already accounted in V8 when using this allocator. This avoids
double-accounting which potentially also reduces (useless) GCs which are
triggered when reaching certain thresholds.
BUG=chromium:611688
LOG=N
Committed: https://crrev.com/9019cf320fdcc79ec31ecfe89f07783a726156a2
Cr-Commit-Position: refs/heads/master@{#394108}
Patch Set 1 #Patch Set 2 : Fix at call site #Patch Set 3 : Fix typo #
Total comments: 2
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs since GCs are triggered when reaching certain thresholds. BUG= ========== to ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG= ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org
ptal and let me know if you'd like them reshuffled differently (Also landing a change in V8 that should better document the contract for this allocator: https://codereview.chromium.org/1978773002/)
Instead of introducing the "accounted" and "unaccounted" API, can we move the AdjustAmountOfMemory call out of the allocateMemoryOrNull() to call sites that really want to account the memory?
Description was changed from ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG= ========== to ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG=chromium:611688 LOG=N ==========
On 2016/05/13 08:53:48, haraken wrote: > Instead of introducing the "accounted" and "unaccounted" API, can we move the > AdjustAmountOfMemory call out of the allocateMemoryOrNull() to call sites that > really want to account the memory? Thanks, ptal! Looks a lot better this way. With this change blink and V8 now agree on the contract for tracking the amount of external memory for ArrayBuffers.
ping ;)
Sorry, I missed this. LGTM.
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977493003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977493003/40001
Message was sent while issue was closed.
Description was changed from ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG=chromium:611688 LOG=N ========== to ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG=chromium:611688 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG=chromium:611688 LOG=N ========== to ========== Remove accounting from ArrayBuffer::Allocator Memory is already accounted in V8 when using this allocator. This avoids double-accounting which potentially also reduces (useless) GCs which are triggered when reaching certain thresholds. BUG=chromium:611688 LOG=N Committed: https://crrev.com/9019cf320fdcc79ec31ecfe89f07783a726156a2 Cr-Commit-Position: refs/heads/master@{#394108} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9019cf320fdcc79ec31ecfe89f07783a726156a2 Cr-Commit-Position: refs/heads/master@{#394108}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1977493003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp (right): https://codereview.chromium.org/1977493003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:142: if (s_adjustAmountOfExternalAllocatedMemoryFunction) What if allocateMemory() isn't able to allocate |sizeInBytes|, don't you either need to re-balance the books or delay this call until after the allocation (has succeeded)?
Message was sent while issue was closed.
https://codereview.chromium.org/1977493003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp (right): https://codereview.chromium.org/1977493003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:142: if (s_adjustAmountOfExternalAllocatedMemoryFunction) On 2016/05/17 21:05:04, sof wrote: > What if allocateMemory() isn't able to allocate |sizeInBytes|, don't you either > need to re-balance the books or delay this call until after the allocation (has > succeeded)? Thanks, you are right. I was not aware that a failing allocation does not trigger an OOM but rather represents a valid state. See: https://codereview.chromium.org/1993623002/ |
