Address ThreadHeap::willObjectBeLazilySwept() corner case.

third_party/WebKit/Source/platform/heap/HeapPage.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 31 matching lines @@
 #include "platform/heap/PageMemory.h"
 #include "platform/heap/PagePool.h"
 #include "platform/heap/SafePoint.h"
 #include "platform/heap/ThreadState.h"
 #include "public/platform/Platform.h"
 #include "wtf/Assertions.h"
 #include "wtf/ContainerAnnotations.h"
 #include "wtf/CurrentTime.h"
 #include "wtf/LeakAnnotations.h"
 #include "wtf/PassOwnPtr.h"
+#include "wtf/TemporaryChange.h"
 #include "wtf/allocator/PageAllocator.h"
 #include "wtf/allocator/Partitions.h"
 
 #ifdef ANNOTATE_CONTIGUOUS_CONTAINER
 // FIXME: have ContainerAnnotations.h define an ENABLE_-style name instead.
 #define ENABLE_ASAN_CONTAINER_ANNOTATIONS 1
 
 // When finalizing a non-inlined vector backing store/container, remove
 // its contiguous container annotation. Required as it will not be destructed
 // from its Vector.
@@ skipping 246 matching lines @@
 {
     // It might be heavy to call Platform::current()->monotonicallyIncreasingTimeSeconds()
     // per page (i.e., 128 KB sweep or one LargeObject sweep), so we check
     // the deadline per 10 pages.
     static const int deadlineCheckInterval = 10;
 
     RELEASE_ASSERT(getThreadState()->isSweepingInProgress());
     ASSERT(getThreadState()->sweepForbidden());
     ASSERT(!getThreadState()->isMainThread() || ScriptForbiddenScope::isScriptForbidden());
 
+    NormalPageArena* normalArena = nullptr;
+    if (m_firstUnsweptPage && !m_firstUnsweptPage->isLargeObjectPage()) {

[haraken, 2016/05/31 05:41:16]

Why is it enough to check if the first unswept page is a large object page or
not? The second unswept page can be a normal page.

[sof, 2016/05/31 07:24:09]

How can an arena contain a mixture of large and normal pages?

+        // Mark this NormalPageArena as being lazily swept.
+        NormalPage* normalPage = reinterpret_cast<NormalPage*>(m_firstUnsweptPage);
+        normalArena = normalPage->arenaForNormalPage();
+        normalArena->setIsLazySweeping(true);
+    }
     int pageCount = 1;
     while (m_firstUnsweptPage) {
         sweepUnsweptPage();
         if (pageCount % deadlineCheckInterval == 0) {
             if (deadlineSeconds <= monotonicallyIncreasingTime()) {
                 // Deadline has come.
                 ThreadHeap::reportMemoryUsageForTracing();
+                if (normalArena)
+                    normalArena->setIsLazySweeping(false);
                 return !m_firstUnsweptPage;
             }
         }
         pageCount++;
     }
     ThreadHeap::reportMemoryUsageForTracing();
+    if (normalArena)
+        normalArena->setIsLazySweeping(false);
     return true;
 }
 
 void BaseArena::completeSweep()
 {
     RELEASE_ASSERT(getThreadState()->isSweepingInProgress());
     ASSERT(getThreadState()->sweepForbidden());
     ASSERT(!getThreadState()->isMainThread() || ScriptForbiddenScope::isScriptForbidden());
 
     while (m_firstUnsweptPage) {
         sweepUnsweptPage();
     }
     ThreadHeap::reportMemoryUsageForTracing();
 }
 
 Address BaseArena::allocateLargeObject(size_t allocationSize, size_t gcInfoIndex)
 {
     // TODO(sof): should need arise, support eagerly finalized large objects.
     CHECK(arenaIndex() != BlinkGC::EagerSweepArenaIndex);
     LargeObjectArena* largeObjectArena = static_cast<LargeObjectArena*>(getThreadState()->arena(BlinkGC::LargeObjectArenaIndex));
     Address largeObject = largeObjectArena -> allocateLargeObjectPage(allocationSize, gcInfoIndex);
     ASAN_MARK_LARGE_VECTOR_CONTAINER(this, largeObject);
     return largeObject;
 }
 
+bool BaseArena::willObjectBeLazilySwept(BasePage* page, void* objectPointer) const
+{
+    // If not on the current page being (potentially) lazily swept, |objectPointer|
+    // is an unmarked, sweepable object.
+    if (page != m_firstUnsweptPage)
+        return true;
+
+    DCHECK(!page->isLargeObjectPage());
+    // Check if the arena is currently being lazily swept.
+    NormalPage* normalPage = reinterpret_cast<NormalPage*>(page);
+    NormalPageArena* normalArena = normalPage->arenaForNormalPage();
+    if (!normalArena->isLazySweeping())
+        return true;
+
+    // Rare special case: unmarked object is on the page being lazily swept,
+    // and a finalizer for an object on that page calls ThreadHeap::willObjectBeLazilySwept().
+    //
+    // Need to determine if |objectPointer| represents a live (unmarked) object or an
+    // unmarked object that will be lazily swept later. As lazy page sweeping
+    // doesn't record a frontier pointer representing how far along it is, the
+    // page is scanned from the start, skipping past freed & unmarked regions.
+    //
+    // If no marked objects are encountered before |objectPointer|, we know
+    // that the finalizing object calling willObjectBeLazilySwept() comes later,
+    // and |objectPointer| has been deemed to be alive already (=> it won't be swept.)
+    //
+    // If a marked object is encountered before |objectPointer|, it will
+    // not have been lazily swept past already. Hence it represents an unmarked,
+    // sweepable object.
+    //
+    // As willObjectBeLazilySwept() is used rarely and it happening to be
+    // used while runnning a finalizer on the page being lazily swept is
+    // even rarer, the page scan is considered acceptable and something
+    // really wanted -- willObjectBeLazilySwept()'s result can be trusted.
+    Address pageEnd = normalPage->payloadEnd();
+    for (Address headerAddress = normalPage->payload(); headerAddress < pageEnd; ) {

[haraken, 2016/05/30 23:54:49]

Instead of scanning the entire page, it would be better to remember
m_headerAddressCurrentlyBeingLazilySwept and start scanning the page from
m_headerAddressCurrentlyBeingLazilySwept.

If objectPointer < m_headerAddressCurrentlyBeingLazilySwept, you can
unconditionally return false. Otherwise, you can start scanning from
m_headerAddressCurrentlyBeingLazilySwept.

We can replace the m_isLazySweeping flag with
m_headerAddressCurrentlyBeingLazilySwept.

[sof, 2016/05/31 05:13:23]

You can make trade offs like that, i.e., take the cost of maintaining a
per-arena pointer value that's written to and kept up-to-date across all
finalizations. It will not be read and used except in this rather very unusual
case; which is why I elected not to. That was how I reasoned.

[haraken, 2016/05/31 05:26:38]

Yeah, that makes sense. The reason I didn't want to scan the area before
m_headerAddressCurrentlyBeingLazilySwept is that the area may already be reused
for new objects. But I realized that the current algorithm will just work for
the area as well.

+        HeapObjectHeader* header = reinterpret_cast<HeapObjectHeader*>(headerAddress);
+        size_t size = header->size();
+        // Scan made it to |objectPointer| without encountering any marked objects.
+        //  => lazy sweep will have processed this unmarked, but live, object.
+        //  => |objectPointer| will not be lazily swept.
+        //
+        // Notice that |objectPointer| might be pointer to a GarbageCollectedMixin,
+        // hence using fromPayload() to derive the HeapObjectHeader isn't possible
+        // (and use its value to check if |headerAddress| is equal to it.)
+        if (headerAddress > objectPointer)
+            return false;
+        if (!header->isFree() && header->isMarked()) {

[haraken, 2016/05/30 23:54:49]

Hmm, I don't fully understand why this check is doing.

I guess what we need is:

  if (headerAddress <= objectPointer && objectPointer < headerAddress) {
    return !header->isMarked();
  }

[sof, 2016/05/31 05:13:23]

That won't work as you could be scanning freelist entries where checkHeader()
will fail.

[haraken, 2016/05/31 05:26:38]

Sorry, I meant:

for (/* scan the page */) {
  if (headerAddress <= objectPointer && objectPointer < headerAddress +
objectSize) {
    return !header->isFree() && !header->isMarked();
  }
}

Why do you return true when '!header->isFree() && header->isMarked()'? I think
that you need to return false in that case because the object is alive.

[sof, 2016/05/31 05:32:25]

No, we already know that objectPointer is unmarked. But if we hit a marked
object before it, we do know that it is unmarked due to not having been marked
(and not having been lazy-sweep processed already.) Hence it should return true.

[haraken, 2016/05/31 05:41:16]

OK. Do you need to check !header->isFree()? If header->isMarked() returns true,
header->isFree() should return false.

+            // There must be a marked object on this page and the one located must
+            // have room after it for the unmarked |objectPointer| object.
+            DCHECK(headerAddress + size < pageEnd);
+            return true;
+        }
+        headerAddress += size;
+    }
+    NOTREACHED();
+    return true;
+}
+
 NormalPageArena::NormalPageArena(ThreadState* state, int index)
     : BaseArena(state, index)
     , m_currentAllocationPoint(nullptr)
     , m_remainingAllocationSize(0)
     , m_lastRemainingAllocationSize(0)
     , m_promptlyFreedSize(0)
+    , m_isLazySweeping(false)
 {
     clearFreeLists();
 }
 
 void NormalPageArena::clearFreeLists()
 {
     setAllocationPoint(nullptr, 0);
     m_freeList.clear();
 }
 
@@ skipping 242 matching lines @@
     ASSERT(pageFromObject(reinterpret_cast<Address>(header)) == findPageFromAddress(reinterpret_cast<Address>(header)));
     m_promptlyFreedSize += shrinkSize;
     header->setSize(allocationSize);
     SET_MEMORY_INACCESSIBLE(shrinkAddress + sizeof(HeapObjectHeader), shrinkSize - sizeof(HeapObjectHeader));
     return false;
 }
 
 Address NormalPageArena::lazySweepPages(size_t allocationSize, size_t gcInfoIndex)
 {
     ASSERT(!hasCurrentAllocationArea());
+    TemporaryChange<bool> isLazySweeping(m_isLazySweeping, true);
     Address result = nullptr;
     while (m_firstUnsweptPage) {
         BasePage* page = m_firstUnsweptPage;
         if (page->isEmpty()) {
             page->unlink(&m_firstUnsweptPage);
             page->removeFromHeap();
         } else {
             // Sweep a page and move the page from m_firstUnsweptPages to
             // m_firstPages.
             page->sweep();
@@ skipping 932 matching lines @@
 
     m_hasEntries = true;
     size_t index = hash(address);
     ASSERT(!(index & 1));
     Address cachePage = roundToBlinkPageStart(address);
     m_entries[index + 1] = m_entries[index];
     m_entries[index] = cachePage;
 }
 
 } // namespace blink