Revert of Oilpan: Remove duplicated code between HeapPage and LargeObject

Revert of Oilpan: Remove duplicated code between HeapPage and LargeObject (patchset #5 id:80001 of https://codereview.chromium.org/840223002/)

Reason for revert:
This has apparently broken a bunch of chrome side tests: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29%2832%29/builds/9539

Original issue's description:
> Oilpan: Remove duplicated code between HeapPage and LargeObject
> 
> Basically, this CL changes:
> 
> class ThreadHeap {
>   ...; // Methods for both HeapPages and LargeObjects
>   HeapPage* m_firstPage;
>   LargeObject* m_firstLargeObject;
> };
> 
> to:
> 
> class ThreadHeap {
>   ...;  // Common methods between HeapPages and LargeObjects.
>   BaseHeapPage* m_firstPage;
> };
> class ThreadHeapForHeapPage {  ...; // Methods for HeapPages. }
> class TheadHeapForLargeObject { ...; // Methods for LargeObjects. }
> 
> and thus remove various duplicated code between HeapPage and LargeObject. To achieve the goal, this CL creates a dedicated ThreadHeap for LargeObjects (i.e., LargeObjectHeap).
> 
> BUG=446386
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189782

TBR=oilpan-reviews@chromium.org,sigbjornf@opera.com,haraken@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=446386
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189828

[caseq, 2015-02-09 17:59:53]

Created Revert of Oilpan: Remove duplicated code between HeapPage and
LargeObject

[commit-bot: I haz the power, 2015-02-09 18:00:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907943002/1

[commit-bot: I haz the power, 2015-02-09 18:01:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-09 18:01:06]

Failed to apply patch for Source/platform/heap/Heap.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/platform/heap/Heap.cpp
  Hunk #1 FAILED at 532.
  Hunk #2 FAILED at 581.
  Hunk #3 FAILED at 598.
  Hunk #4 FAILED at 608.
  Hunk #5 FAILED at 627.
  Hunk #7 FAILED at 667.
  Hunk #8 FAILED at 702.
  Hunk #9 FAILED at 724.
  Hunk #10 FAILED at 753.
  Hunk #11 FAILED at 789.
  Hunk #12 FAILED at 820.
  Hunk #13 FAILED at 842.
  Hunk #15 FAILED at 909.
  Hunk #16 FAILED at 933.
  Hunk #17 FAILED at 956.
  Hunk #19 FAILED at 988.
  Hunk #21 FAILED at 1008.
  Hunk #22 FAILED at 1049.
  Hunk #24 succeeded at 1188 with fuzz 1.
  Hunk #25 FAILED at 1293.
  Hunk #26 FAILED at 1306.
  Hunk #28 FAILED at 1340.
  Hunk #29 FAILED at 1367.
  Hunk #30 FAILED at 1395.
  Hunk #31 FAILED at 1407.
  Hunk #32 FAILED at 1419.
  Hunk #33 FAILED at 1512.
  Hunk #34 FAILED at 1554.
  Hunk #35 FAILED at 1587.
  Hunk #36 FAILED at 1620.
  Hunk #37 FAILED at 2546.
  Hunk #38 FAILED at 2583.
  Hunk #39 FAILED at 2620.
  Hunk #40 FAILED at 2663.
  33 out of 40 hunks FAILED -- saving rejects to file
Source/platform/heap/Heap.cpp.rej

Patch:       Source/platform/heap/Heap.cpp
Index: Source/platform/heap/Heap.cpp
diff --git a/Source/platform/heap/Heap.cpp b/Source/platform/heap/Heap.cpp
index
754988e56e81a7d132ff104c7e02185de8e009d9..ee78353fee1f2e20aa0e6d4c70db925e4b5823d1
100644
--- a/Source/platform/heap/Heap.cpp
+++ b/Source/platform/heap/Heap.cpp
@@ -532,48 +532,42 @@
         header->markDead();
 }
 
-void LargeObject::removeFromHeap()
-{
-    static_cast<ThreadHeapForLargeObject*>(heap())->freeLargeObject(this);
-}
-
-FreeList::FreeList()
-    : m_biggestFreeListIndex(0)
-{
+void LargeObject::removeFromHeap(ThreadHeap* heap)
+{
+    heap->freeLargeObject(this);
 }
 
 ThreadHeap::ThreadHeap(ThreadState* state, int index)
-    : m_firstPage(nullptr)
+    : m_currentAllocationPoint(nullptr)
+    , m_remainingAllocationSize(0)
+    , m_lastRemainingAllocationSize(0)
+    , m_firstPage(nullptr)
+    , m_firstLargeObject(nullptr)
     , m_firstUnsweptPage(nullptr)
+    , m_firstUnsweptLargeObject(nullptr)
     , m_threadState(state)
     , m_index(index)
+    , m_promptlyFreedSize(0)
 #if ENABLE(GC_PROFILING)
     , m_cumulativeAllocationSize(0)
     , m_allocationCount(0)
     , m_inlineAllocationCount(0)
 #endif
 {
-}
-
-ThreadHeapForHeapPage::ThreadHeapForHeapPage(ThreadState* state, int index)
-    : ThreadHeap(state, index)
-    , m_currentAllocationPoint(nullptr)
-    , m_remainingAllocationSize(0)
-    , m_lastRemainingAllocationSize(0)
-    , m_promptlyFreedSize(0)
-{
     clearFreeLists();
 }
 
-ThreadHeapForLargeObject::ThreadHeapForLargeObject(ThreadState* state, int
index)
-    : ThreadHeap(state, index)
+FreeList::FreeList()
+    : m_biggestFreeListIndex(0)
 {
 }
 
 ThreadHeap::~ThreadHeap()
 {
     ASSERT(!m_firstPage);
+    ASSERT(!m_firstLargeObject);
     ASSERT(!m_firstUnsweptPage);
+    ASSERT(!m_firstUnsweptLargeObject);
 }
 
 void ThreadHeap::cleanupPages()
@@ -581,15 +575,22 @@
     clearFreeLists();
 
     ASSERT(!m_firstUnsweptPage);
+    ASSERT(!m_firstUnsweptLargeObject);
     // Add the ThreadHeap's pages to the orphanedPagePool.
-    for (BaseHeapPage* page = m_firstPage; page; page = page->next()) {
-        Heap::decreaseAllocatedSpace(page->size());
-        Heap::orphanedPagePool()->addOrphanedPage(heapIndex(), page);
+    for (HeapPage* page = m_firstPage; page; page = page->m_next) {
+        Heap::decreaseAllocatedSpace(blinkPageSize);
+        Heap::orphanedPagePool()->addOrphanedPage(m_index, page);
     }
     m_firstPage = nullptr;
-}
-
-void ThreadHeapForHeapPage::updateRemainingAllocationSize()
+
+    for (LargeObject* largeObject = m_firstLargeObject; largeObject;
largeObject = largeObject->m_next) {
+        Heap::decreaseAllocatedSpace(largeObject->size());
+        Heap::orphanedPagePool()->addOrphanedPage(m_index, largeObject);
+    }
+    m_firstLargeObject = nullptr;
+}
+
+void ThreadHeap::updateRemainingAllocationSize()
 {
     if (m_lastRemainingAllocationSize > remainingAllocationSize()) {
         Heap::increaseAllocatedObjectSize(m_lastRemainingAllocationSize -
remainingAllocationSize());
@@ -598,7 +599,7 @@
     ASSERT(m_lastRemainingAllocationSize == remainingAllocationSize());
 }
 
-void ThreadHeapForHeapPage::setAllocationPoint(Address point, size_t size)
+void ThreadHeap::setAllocationPoint(Address point, size_t size)
 {
 #if ENABLE(ASSERT)
     if (point) {
@@ -608,15 +609,14 @@
         ASSERT(size <= static_cast<HeapPage*>(page)->payloadSize());
     }
 #endif
-    if (hasCurrentAllocationArea()) {
+    if (hasCurrentAllocationArea())
         addToFreeList(currentAllocationPoint(), remainingAllocationSize());
-    }
     updateRemainingAllocationSize();
     m_currentAllocationPoint = point;
     m_lastRemainingAllocationSize = m_remainingAllocationSize = size;
 }
 
-Address ThreadHeapForHeapPage::outOfLineAllocate(size_t allocationSize, size_t
gcInfoIndex)
+Address ThreadHeap::outOfLineAllocate(size_t allocationSize, size_t
gcInfoIndex)
 {
     ASSERT(allocationSize > remainingAllocationSize());
     ASSERT(allocationSize >= allocationGranularity);
@@ -627,7 +627,7 @@
 
     // 1. If this allocation is big enough, allocate a large object.
     if (allocationSize >= largeObjectSizeThreshold)
-        return
static_cast<ThreadHeapForLargeObject*>(threadState()->heap(LargeObjectHeap))->allocateLargeObject(allocationSize,
gcInfoIndex);
+        return allocateLargeObject(allocationSize, gcInfoIndex);
 
     // 2. Check if we should trigger a GC.
     updateRemainingAllocationSize();
@@ -643,7 +643,7 @@
 
     // 5. Lazily sweep pages of this heap until we find a freed area for
     // this allocation or we finish sweeping all pages of this heap.
-    result = lazySweep(allocationSize, gcInfoIndex);
+    result = lazySweepPages(allocationSize, gcInfoIndex);
     if (result)
         return result;
 
@@ -667,7 +667,7 @@
     return result;
 }
 
-Address ThreadHeapForHeapPage::allocateFromFreeList(size_t allocationSize,
size_t gcInfoIndex)
+Address ThreadHeap::allocateFromFreeList(size_t allocationSize, size_t
gcInfoIndex)
 {
     // Try reusing a block from the largest bin. The underlying reasoning
     // being that we want to amortize this slow allocation call by carving
@@ -702,14 +702,20 @@
 {
     ASSERT(!threadState()->isInGC());
     ASSERT(!m_firstUnsweptPage);
+    ASSERT(!m_firstUnsweptLargeObject);
 
     // Move all pages to a list of unswept pages.
     m_firstUnsweptPage = m_firstPage;
+    m_firstUnsweptLargeObject = m_firstLargeObject;
     m_firstPage = nullptr;
-}
-
-Address ThreadHeap::lazySweep(size_t allocationSize, size_t gcInfoIndex)
-{
+    m_firstLargeObject = nullptr;
+}
+
+Address ThreadHeap::lazySweepPages(size_t allocationSize, size_t gcInfoIndex)
+{
+    ASSERT(!hasCurrentAllocationArea());
+    ASSERT(allocationSize < largeObjectSizeThreshold);
+
     // If there are no pages to be swept, return immediately.
     if (!m_firstUnsweptPage)
         return nullptr;
@@ -724,27 +730,17 @@
         return nullptr;
 
     TRACE_EVENT0("blink_gc", "ThreadHeap::lazySweepPages");
-    ThreadState::SweepForbiddenScope scope(threadState());
+    ThreadState::SweepForbiddenScope scope(m_threadState);
 
     if (threadState()->isMainThread())
         ScriptForbiddenScope::enter();
 
-    Address result = lazySweepPages(allocationSize, gcInfoIndex);
-
-    if (threadState()->isMainThread())
-        ScriptForbiddenScope::exit();
-    return result;
-}
-
-Address ThreadHeapForHeapPage::lazySweepPages(size_t allocationSize, size_t
gcInfoIndex)
-{
-    ASSERT(!hasCurrentAllocationArea());
     Address result = nullptr;
     while (m_firstUnsweptPage) {
-        BaseHeapPage* page = m_firstUnsweptPage;
+        HeapPage* page = m_firstUnsweptPage;
         if (page->isEmpty()) {
             page->unlink(&m_firstUnsweptPage);
-            page->removeFromHeap();
+            page->removeFromHeap(this);
         } else {
             // Sweep a page and move the page from m_firstUnsweptPages to
             // m_firstPages.
@@ -753,33 +749,84 @@
             page->link(&m_firstPage);
             page->markAsSwept();
 
-            // For HeapPage, stop lazy sweeping once we find a slot to
-            // allocate a new object.
             result = allocateFromFreeList(allocationSize, gcInfoIndex);
             if (result)
                 break;
         }
     }
+
+    if (threadState()->isMainThread())
+        ScriptForbiddenScope::exit();
     return result;
 }
 
-Address ThreadHeapForLargeObject::lazySweepPages(size_t allocationSize, size_t
gcInfoIndex)
-{
-    Address result = nullptr;
+bool ThreadHeap::lazySweepLargeObjects(size_t allocationSize)
+{
+    ASSERT(allocationSize >= largeObjectSizeThreshold);
+
+    // If there are no large objects to be swept, return immediately.
+    if (!m_firstUnsweptLargeObject)
+        return false;
+
+    RELEASE_ASSERT(threadState()->isSweepingInProgress());
+
+    // lazySweepLargeObjects() can be called recursively if finalizers invoked
+    // in page->sweep() allocate memory and the allocation triggers
+    // lazySweepLargeObjects(). This check prevents the sweeping from being
+    // executed recursively.
+    if (threadState()->sweepForbidden())
+        return false;
+
+    TRACE_EVENT0("blink_gc", "ThreadHeap::lazySweepLargeObjects");
+    ThreadState::SweepForbiddenScope scope(m_threadState);
+
+    if (threadState()->isMainThread())
+        ScriptForbiddenScope::enter();
+
+    bool result = false;
     size_t sweptSize = 0;
-    while (m_firstUnsweptPage) {
-        BaseHeapPage* page = m_firstUnsweptPage;
-        if (page->isEmpty()) {
-            sweptSize += static_cast<LargeObject*>(page)->payloadSize() +
sizeof(HeapObjectHeader);
-            page->unlink(&m_firstUnsweptPage);
-            page->removeFromHeap();
-            // For Lar…
(message too large)

[commit-bot: I haz the power, 2015-02-09 18:01:14]

The CQ bit was unchecked by commit-bot@chromium.org

[sof, 2015-02-09 18:32:13]

thanks for reverting, lgtm.

to get a clean revert, i think you also need to revert
https://codereview.chromium.org/906213002 also.

[caseq, 2015-02-09 18:35:08]

On 2015/02/09 18:32:13, sof wrote:
> thanks for reverting, lgtm.
> 
> to get a clean revert, i think you also need to revert
> https://codereview.chromium.org/906213002 also.

Ouch, indeed! Thanks!

[caseq, 2015-02-09 18:38:22]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2015-02-09 18:39:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907943002/1

[commit-bot: I haz the power, 2015-02-09 18:40:31]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189828

[haraken, 2015-02-10 01:04:32]

LGTM