Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127453002:
Oilpan: Introduce memory pool for CallbackStacks (Closed)
|
Created:
4 years, 5 months ago by haraken Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, kouhei+heap_chromium.org, oilpan-reviews, Mads Ager (chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOilpan: Introduce memory pool for CallbackStacks
Once we enable per-thread heaps, we will end up with having CallbackStacks per thread.
To prevent the memory bloat, this CL introduces a memory pool for CallbackStacks
shared among all threads. 256 KB is pre-allocated for the memory pool.
BUG=
Committed: https://crrev.com/d55ec436ad893ebace1f22b7f43bf98c6dba6e46
Cr-Commit-Position: refs/heads/master@{#404381}
Patch Set 1 #
Total comments: 10
Patch Set 2 : temp #
Total comments: 5
Patch Set 3 : temp #Patch Set 4 : temp #Patch Set 5 : temp #
Messages
Total messages: 26 (10 generated)
haraken@chromium.org changed reviewers: + keishi@chromium.org, sigbjornf@opera.com
PTAL
Some windows compilation failures being reported. https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:18: MutexLocker locker(m_mutex); Do you need to take this lock? https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:31: WTF::freePages(m_pooledMemory, kBlockSize * kPooledBlockCount * sizeof(CallbackStack::Item)); Could you tidy up the use of the expressions (kPooledBlockCount *)(kBlockSize * sizeof(CallbackStack::Item)) a bit for readability? https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:31: WTF::freePages(m_pooledMemory, kBlockSize * kPooledBlockCount * sizeof(CallbackStack::Item)); clear m_pooledMemory after freeing? https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:38: if (m_freeListFirst != -1) { (A short comment explaining the two code paths here would help someone to quicker grok what's going on here.) https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/CallbackStack.h (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.h:123: // 2048 * 8 * sizeof(Item) = 256 KB is pre-allocated for the underlying 256Kb for some choice of pointer size, yes. :) https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/Heap.cpp:228: , m_markingStack(wrapUnique(new CallbackStack())) Let's add CallbackStack::create() ? https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.cpp:963: m_threadLocalWeakCallbackStack->decommit(); Naming this clear() or reset() would be clearer, perhaps. But, wouldn't it make sense to decommit the thread-local stack after having completed the thread-local weak processing? If not, don't you risk having N threads holding on to a pooled stack, but the GCing thread having to commit & allocate its stacks from somewhere else.
Thanks for the careful review! Addressed all the comments. https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:37: static_assert((kBlockSize * sizeof(CallbackStack::Item)) % WTF::kPageAllocationGranularity == 0, "Allocated block size must be a multiple of WTF::kPageAllocationGranularity"); I don't think this assert is needed. So removed. https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.cpp:963: m_threadLocalWeakCallbackStack->decommit(); On 2016/07/05 14:19:02, sof wrote: > Naming this clear() or reset() would be clearer, perhaps. > > But, wouldn't it make sense to decommit the thread-local stack after having > completed the thread-local weak processing? If not, don't you risk having N > threads holding on to a pooled stack, but the GCing thread having to commit & > allocate its stacks from somewhere else. We're already calling decommit() at the end of ThreadState::threadLocalWeakProcessing(). I'd prefer "commit"/"decommit" to make the function names balanced (V8 is using the names).
https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (left): https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:10: size_t const CallbackStack::kMinimalBlockSize = WTF::kPageAllocationGranularity / sizeof(CallbackStack::Item); I noticed that WTF::kPageAllocationGranularity is 64 KB on Win. I guess we were allocating huge callback stacks on Win...
https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (left): https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:10: size_t const CallbackStack::kMinimalBlockSize = WTF::kPageAllocationGranularity / sizeof(CallbackStack::Item); On 2016/07/06 02:01:38, haraken wrote: > > I noticed that WTF::kPageAllocationGranularity is 64 KB on Win. I guess we were > allocating huge callback stacks on Win... > I wouldn't worry too much -- the non-main thread thread-local callback stack would have a 16k block vs 1k elsewhere; a block that would most likely not be exhausted due to light thread-local processing in worker contexts. All other callback stacks have the same size across targets.
I apologize for the delay - lgtm % addressing bot failures. https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/2127453002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/heap/ThreadState.cpp:963: m_threadLocalWeakCallbackStack->decommit(); On 2016/07/06 01:58:11, haraken wrote: > On 2016/07/05 14:19:02, sof wrote: > > Naming this clear() or reset() would be clearer, perhaps. > > > > But, wouldn't it make sense to decommit the thread-local stack after having > > completed the thread-local weak processing? If not, don't you risk having N > > threads holding on to a pooled stack, but the GCing thread having to commit & > > allocate its stacks from somewhere else. > > We're already calling decommit() at the end of > ThreadState::threadLocalWeakProcessing(). > Ah, of course we already are; good. > I'd prefer "commit"/"decommit" to make the function names balanced (V8 is using > the names). ok. https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:47: CallbackStack::Item* memory = static_cast<CallbackStack::Item*>(WTF::allocPages(nullptr, kBlockBytes, WTF::kPageAllocationGranularity, WTF::PageAccessible)); The block size/length isn't a multiple of kPageAllocationGranularity, on Windows.
https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:47: CallbackStack::Item* memory = static_cast<CallbackStack::Item*>(WTF::allocPages(nullptr, kBlockBytes, WTF::kPageAllocationGranularity, WTF::PageAccessible)); On 2016/07/06 13:22:17, sof wrote: > The block size/length isn't a multiple of kPageAllocationGranularity, on > Windows. On Windows, kPageAllocationGranularity is 64 KB. Does it mean that we need to allocate 64 KB of memory only for one CallbackStack?
https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/CallbackStack.cpp:47: CallbackStack::Item* memory = static_cast<CallbackStack::Item*>(WTF::allocPages(nullptr, kBlockBytes, WTF::kPageAllocationGranularity, WTF::PageAccessible)); On 2016/07/06 13:27:29, haraken wrote: > On 2016/07/06 13:22:17, sof wrote: > > The block size/length isn't a multiple of kPageAllocationGranularity, on > > Windows. > > On Windows, kPageAllocationGranularity is 64 KB. Does it mean that we need to > allocate 64 KB of memory only for one CallbackStack? If you want to use allocPages().. but why insist on that?
On 2016/07/06 13:33:07, sof wrote: > https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/CallbackStack.cpp (right): > > https://codereview.chromium.org/2127453002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/CallbackStack.cpp:47: > CallbackStack::Item* memory = > static_cast<CallbackStack::Item*>(WTF::allocPages(nullptr, kBlockBytes, > WTF::kPageAllocationGranularity, WTF::PageAccessible)); > On 2016/07/06 13:27:29, haraken wrote: > > On 2016/07/06 13:22:17, sof wrote: > > > The block size/length isn't a multiple of kPageAllocationGranularity, on > > > Windows. > > > > On Windows, kPageAllocationGranularity is 64 KB. Does it mean that we need to > > allocate 64 KB of memory only for one CallbackStack? > > If you want to use allocPages().. but why insist on that? Clever. Changed it to Partitions::fastMalloc/fastFree.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2127453002/#ps40001 (title: "temp")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_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 haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2127453002/#ps60001 (title: "temp")
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
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2127453002/#ps80001 (title: "temp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Oilpan: Introduce memory pool for CallbackStacks Once we enable per-thread heaps, we will end up with having CallbackStacks per thread. To prevent the memory bloat, this CL introduces a memory pool for CallbackStacks shared among all threads. 256 KB is pre-allocated for the memory pool. BUG= ========== to ========== Oilpan: Introduce memory pool for CallbackStacks Once we enable per-thread heaps, we will end up with having CallbackStacks per thread. To prevent the memory bloat, this CL introduces a memory pool for CallbackStacks shared among all threads. 256 KB is pre-allocated for the memory pool. BUG= Committed: https://crrev.com/d55ec436ad893ebace1f22b7f43bf98c6dba6e46 Cr-Commit-Position: refs/heads/master@{#404381} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d55ec436ad893ebace1f22b7f43bf98c6dba6e46 Cr-Commit-Position: refs/heads/master@{#404381} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
