Heap should not continually allocate new pages in steady state
https://bugs.webkit.org/show_bug.cgi?id=85936
Reviewed by Geoff Garen.
Currently, in steady state (i.e. a constant amount of live GC
memory with a constant rate of allocation) assuming we've just
finished a collection with X live blocks in CopiedSpace, we
increase our working set by X blocks in CopiedSpace with each
collection we perform. This is due to the fact that we allocate
until we run out of free blocks to use in the Heap before we
consider whether we should run a collection.
In the longer term, this issue will be mostly resolved by
implementing quick release for the CopiedSpace. In the shorter
term, we should change our policy to check whether we should
allocate before trying to use a free block from the Heap. We
can change our policy to something more appropriate once we
have implemented quick release.
This change should also have the convenient side effect of
reducing the variance in GC-heavy tests (e.g. v8-splay) due
to fact that we are doing less VM allocation during copying
collection. Overall, this patch is performance neutral across
the benchmarks we track.
* heap/CopiedSpace.cpp:
(JSC::CopiedSpace::getFreshBlock): Shuffle the request from the BlockAllocator
around so that we only do it if the block request must succeed
i.e. after we've already checked whether we should do a collection.
* heap/MarkedAllocator.cpp:
(JSC::MarkedAllocator::allocateSlowCase): Ditto.
(JSC::MarkedAllocator::allocateBlock): We no longer have a failure mode in this
function because by the time we've called it, we've already checked whether we
should run a collection so there's no point in returning null.
* heap/MarkedAllocator.h: Removing old arguments from function declaration.
(MarkedAllocator):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@116484 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 11977d1..9c66066 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,43 @@
+2012-05-08 Mark Hahnenberg <mhahnenberg@apple.com>
+
+ Heap should not continually allocate new pages in steady state
+ https://bugs.webkit.org/show_bug.cgi?id=85936
+
+ Reviewed by Geoff Garen.
+
+ Currently, in steady state (i.e. a constant amount of live GC
+ memory with a constant rate of allocation) assuming we've just
+ finished a collection with X live blocks in CopiedSpace, we
+ increase our working set by X blocks in CopiedSpace with each
+ collection we perform. This is due to the fact that we allocate
+ until we run out of free blocks to use in the Heap before we
+ consider whether we should run a collection.
+
+ In the longer term, this issue will be mostly resolved by
+ implementing quick release for the CopiedSpace. In the shorter
+ term, we should change our policy to check whether we should
+ allocate before trying to use a free block from the Heap. We
+ can change our policy to something more appropriate once we
+ have implemented quick release.
+
+ This change should also have the convenient side effect of
+ reducing the variance in GC-heavy tests (e.g. v8-splay) due
+ to fact that we are doing less VM allocation during copying
+ collection. Overall, this patch is performance neutral across
+ the benchmarks we track.
+
+ * heap/CopiedSpace.cpp:
+ (JSC::CopiedSpace::getFreshBlock): Shuffle the request from the BlockAllocator
+ around so that we only do it if the block request must succeed
+ i.e. after we've already checked whether we should do a collection.
+ * heap/MarkedAllocator.cpp:
+ (JSC::MarkedAllocator::allocateSlowCase): Ditto.
+ (JSC::MarkedAllocator::allocateBlock): We no longer have a failure mode in this
+ function because by the time we've called it, we've already checked whether we
+ should run a collection so there's no point in returning null.
+ * heap/MarkedAllocator.h: Removing old arguments from function declaration.
+ (MarkedAllocator):
+
2012-05-08 Gavin Barraclough <barraclough@apple.com>
SIGFPE on divide in classic interpreter
diff --git a/Source/JavaScriptCore/heap/CopiedSpace.cpp b/Source/JavaScriptCore/heap/CopiedSpace.cpp
index 063ea65..5efd043 100644
--- a/Source/JavaScriptCore/heap/CopiedSpace.cpp
+++ b/Source/JavaScriptCore/heap/CopiedSpace.cpp
@@ -212,10 +212,10 @@
CheckedBoolean CopiedSpace::getFreshBlock(AllocationEffort allocationEffort, CopiedBlock** outBlock)
{
CopiedBlock* block = 0;
- if (HeapBlock* heapBlock = m_heap->blockAllocator().allocate())
- block = new (NotNull, heapBlock) CopiedBlock(heapBlock->m_allocation);
- else if (allocationEffort == AllocationMustSucceed) {
- if (!allocateNewBlock(&block)) {
+ if (allocationEffort == AllocationMustSucceed) {
+ if (HeapBlock* heapBlock = m_heap->blockAllocator().allocate())
+ block = new (NotNull, heapBlock) CopiedBlock(heapBlock->m_allocation);
+ else if (!allocateNewBlock(&block)) {
*outBlock = 0;
ASSERT_NOT_REACHED();
return false;
diff --git a/Source/JavaScriptCore/heap/MarkedAllocator.cpp b/Source/JavaScriptCore/heap/MarkedAllocator.cpp
index b5e5fff..01f00c3 100644
--- a/Source/JavaScriptCore/heap/MarkedAllocator.cpp
+++ b/Source/JavaScriptCore/heap/MarkedAllocator.cpp
@@ -68,44 +68,30 @@
if (LIKELY(result != 0))
return result;
- AllocationEffort allocationEffort;
-
- if (m_heap->shouldCollect())
- allocationEffort = AllocationCanFail;
- else
- allocationEffort = AllocationMustSucceed;
-
- MarkedBlock* block = allocateBlock(allocationEffort);
- if (block) {
- addBlock(block);
- void* result = tryAllocate();
- ASSERT(result);
- return result;
+ if (m_heap->shouldCollect()) {
+ m_heap->collect(Heap::DoNotSweep);
+
+ result = tryAllocate();
+ if (result)
+ return result;
}
-
- m_heap->collect(Heap::DoNotSweep);
-
- result = tryAllocate();
-
- if (result)
- return result;
-
+
ASSERT(!m_heap->shouldCollect());
- addBlock(allocateBlock(AllocationMustSucceed));
-
+ MarkedBlock* block = allocateBlock();
+ ASSERT(block);
+ addBlock(block);
+
result = tryAllocate();
ASSERT(result);
return result;
}
-MarkedBlock* MarkedAllocator::allocateBlock(AllocationEffort allocationEffort)
+MarkedBlock* MarkedAllocator::allocateBlock()
{
MarkedBlock* block = static_cast<MarkedBlock*>(m_heap->blockAllocator().allocate());
if (block)
block = MarkedBlock::recycle(block, m_heap, m_cellSize, m_cellsNeedDestruction);
- else if (allocationEffort == AllocationCanFail)
- return 0;
else
block = MarkedBlock::create(m_heap, m_cellSize, m_cellsNeedDestruction);
diff --git a/Source/JavaScriptCore/heap/MarkedAllocator.h b/Source/JavaScriptCore/heap/MarkedAllocator.h
index 8ad7e92..8b3620f 100644
--- a/Source/JavaScriptCore/heap/MarkedAllocator.h
+++ b/Source/JavaScriptCore/heap/MarkedAllocator.h
@@ -41,7 +41,7 @@
JS_EXPORT_PRIVATE void* allocateSlowCase();
void* tryAllocate();
void* tryAllocateHelper();
- MarkedBlock* allocateBlock(AllocationEffort);
+ MarkedBlock* allocateBlock();
MarkedBlock::FreeList m_freeList;
MarkedBlock* m_currentBlock;
