There is no good reason for WeakBlock to care about newly allocated objects
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Sep 2016 17:17:07 +0000 (17:17 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Sep 2016 17:17:07 +0000 (17:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162006

Reviewed by Geoffrey Garen.

WeakBlock scans itself in two modes:

visit: if a Weak in the block belongs to an unmarked object, ask the Weak to consider whether
    it should do things.

reap: if a Weak in a block belongs to an unmarked object, delete the Weak.

Except that "unmarked" has a peculiar meaning: WeakBlock defines it as
!markedOrNewlyAllocated. So, a newly allocated object will never be consulted about anything.
That sounds scary until you realize that newlyAllocated must have been cleared before we even
got here.

So, we were paying the price of checking newlyAllocated for no reason. This switches the code
to using isMarked(). I don't know why the code previously checked newlyAllocated, but I do
trust my reasoning.

* heap/LargeAllocation.h:
(JSC::LargeAllocation::isMarkedDuringWeakVisiting):
(JSC::LargeAllocation::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
* heap/MarkedBlock.h:
(JSC::MarkedBlock::isMarkedDuringWeakVisiting):
(JSC::MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
* heap/WeakBlock.cpp:
(JSC::WeakBlock::specializedVisit):
(JSC::WeakBlock::reap):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205979 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/LargeAllocation.h
Source/JavaScriptCore/heap/MarkedBlock.h
Source/JavaScriptCore/heap/WeakBlock.cpp

index bc52763..87fab10 100644 (file)
@@ -1,3 +1,36 @@
+2016-09-14  Filip Pizlo  <fpizlo@apple.com>
+
+        There is no good reason for WeakBlock to care about newly allocated objects
+        https://bugs.webkit.org/show_bug.cgi?id=162006
+
+        Reviewed by Geoffrey Garen.
+
+        WeakBlock scans itself in two modes:
+
+        visit: if a Weak in the block belongs to an unmarked object, ask the Weak to consider whether
+            it should do things.
+        
+        reap: if a Weak in a block belongs to an unmarked object, delete the Weak.
+
+        Except that "unmarked" has a peculiar meaning: WeakBlock defines it as
+        !markedOrNewlyAllocated. So, a newly allocated object will never be consulted about anything. 
+        That sounds scary until you realize that newlyAllocated must have been cleared before we even
+        got here.
+
+        So, we were paying the price of checking newlyAllocated for no reason. This switches the code
+        to using isMarked(). I don't know why the code previously checked newlyAllocated, but I do
+        trust my reasoning.
+
+        * heap/LargeAllocation.h:
+        (JSC::LargeAllocation::isMarkedDuringWeakVisiting):
+        (JSC::LargeAllocation::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
+        * heap/MarkedBlock.h:
+        (JSC::MarkedBlock::isMarkedDuringWeakVisiting):
+        (JSC::MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting): Deleted.
+        * heap/WeakBlock.cpp:
+        (JSC::WeakBlock::specializedVisit):
+        (JSC::WeakBlock::reap):
+
 2016-09-15  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r205931.
index b2bd91f..f246d13 100644 (file)
@@ -72,7 +72,7 @@ public:
     ALWAYS_INLINE bool isMarked() { return m_isMarked.load(std::memory_order_relaxed); }
     bool isMarkedOrNewlyAllocated() { return isMarked() || isNewlyAllocated(); }
     bool isMarkedOrNewlyAllocated(HeapCell*) { return isMarkedOrNewlyAllocated(); }
-    bool isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion, HeapCell*) { return isMarkedOrNewlyAllocated(); }
+    bool isMarkedDuringWeakVisiting(HeapVersion, HeapCell*) { return isMarked(); }
     bool isLive() { return isMarkedOrNewlyAllocated(); }
     
     bool hasValidCell() const { return m_hasValidCell; }
index d26aff9..047ca03 100644 (file)
@@ -251,7 +251,8 @@ public:
     bool testAndSetMarked(const void*);
         
     bool isMarkedOrNewlyAllocated(const HeapCell*);
-    bool isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion, const HeapCell*);
+    
+    bool isMarkedDuringWeakVisiting(HeapVersion, const HeapCell*);
 
     bool isAtom(const void*);
     void clearMarked(const void*);
@@ -561,11 +562,11 @@ inline bool MarkedBlock::isMarkedOrNewlyAllocated(const HeapCell* cell)
     return isMarked(cell) || (m_handle.m_newlyAllocated && m_handle.isNewlyAllocated(cell));
 }
 
-inline bool MarkedBlock::isMarkedOrNewlyAllocatedDuringWeakVisiting(HeapVersion heapVersion, const HeapCell* cell)
+inline bool MarkedBlock::isMarkedDuringWeakVisiting(HeapVersion heapVersion, const HeapCell* cell)
 {
     if (needsFlip(heapVersion))
         return false;
-    return isMarkedOrNewlyAllocated(cell);
+    return isMarked(cell);
 }
 
 inline bool MarkedBlock::Handle::isLive(const HeapCell* cell)
index b507bde..00dbc0e 100644 (file)
@@ -113,7 +113,7 @@ void WeakBlock::specializedVisit(ContainerType& container, HeapRootVisitor& heap
             continue;
 
         const JSValue& jsValue = weakImpl->jsValue();
-        if (container.isMarkedOrNewlyAllocatedDuringWeakVisiting(version, jsValue.asCell()))
+        if (container.isMarkedDuringWeakVisiting(version, jsValue.asCell()))
             continue;
         
         if (!weakHandleOwner->isReachableFromOpaqueRoots(Handle<Unknown>::wrapSlot(&const_cast<JSValue&>(jsValue)), weakImpl->context(), visitor))
@@ -157,7 +157,7 @@ void WeakBlock::reap()
         if (weakImpl->state() > WeakImpl::Dead)
             continue;
 
-        if (m_container.isMarkedOrNewlyAllocated(weakImpl->jsValue().asCell())) {
+        if (m_container.isMarked(weakImpl->jsValue().asCell())) {
             ASSERT(weakImpl->state() == WeakImpl::Live);
             continue;
         }