Changeset 267481 in webkit

Timestamp:

Sep 23, 2020, 9:41:03 AM (6 years ago)

Author:

Alan Coon

Message:

Cherry-pick r267304. rdar://problem/69439749

[JSC] PreciseAllocation's isNewlyAllocated flag should be propagated from isMarked at GC begin phase to make isLive correct
​https://bugs.webkit.org/show_bug.cgi?id=216717

Reviewed by Mark Lam.

When starting full GC, at beginMarking, PreciseAllocation's mark bit is cleared to be usable for upcoming marking.
However, this means that HeapCell::isLive will see this object as dead until it is marked.
Let's consider that this object is not newly allocated one. Then, its isNewlyAllocated is false. And now mark bit

is also cleared. Since PreciseAllocation::isLive is isNewlyAllocated

isMarked, then it looks dead, while it is live.

This confuses HeapCell:isLive function and makes some of watchpoints perform wrong decisions (e.g. this condition is
no longer valid, let's just discard it).
At the beginning of full collection, we should propagate the old mark bit to isNewlyAllocated so that it looks live
during marking. This is similar trick to MarkedBlock::aboutToMark.

• heap/PreciseAllocation.cpp: (JSC::PreciseAllocation::flip):

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

Location:

branches/safari-610.2.6.1-branch/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/PreciseAllocation.cpp (modified) (1 diff)

branches/safari-610.2.6.1-branch/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-09-23  Russell Epstein  <repstein@apple.com>
+
+        Cherry-pick r267304. rdar://problem/69439749
+
+    [JSC] PreciseAllocation's isNewlyAllocated flag should be propagated from isMarked at GC begin phase to make isLive correct
+    https://bugs.webkit.org/show_bug.cgi?id=216717
+    
+    Reviewed by Mark Lam.
+    
+    When starting full GC, at beginMarking, PreciseAllocation's mark bit is cleared to be usable for upcoming marking.
+    However, this means that HeapCell::isLive will see this object as dead until it is marked.
+    Let's consider that this object is not newly allocated one. Then, its isNewlyAllocated is false. And now mark bit
+    is also cleared. Since PreciseAllocation::isLive is isNewlyAllocated || isMarked, then it looks dead, while it is live.
+    This confuses HeapCell:isLive function and makes some of watchpoints perform wrong decisions (e.g. this condition is
+    no longer valid, let's just discard it).
+    At the beginning of full collection, we should propagate the old mark bit to isNewlyAllocated so that it looks live
+    during marking. This is similar trick to MarkedBlock::aboutToMark.
+    
+    * heap/PreciseAllocation.cpp:
+    (JSC::PreciseAllocation::flip):
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@267304 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    2020-09-18  Yusuke Suzuki  <ysuzuki@apple.com>
+
+            [JSC] PreciseAllocation's isNewlyAllocated flag should be propagated from isMarked at GC begin phase to make isLive correct
+            https://bugs.webkit.org/show_bug.cgi?id=216717
+
+            Reviewed by Mark Lam.
+
+            When starting full GC, at beginMarking, PreciseAllocation's mark bit is cleared to be usable for upcoming marking.
+            However, this means that HeapCell::isLive will see this object as dead until it is marked.
+            Let's consider that this object is not newly allocated one. Then, its isNewlyAllocated is false. And now mark bit
+            is also cleared. Since PreciseAllocation::isLive is isNewlyAllocated || isMarked, then it looks dead, while it is live.
+            This confuses HeapCell:isLive function and makes some of watchpoints perform wrong decisions (e.g. this condition is
+            no longer valid, let's just discard it).
+            At the beginning of full collection, we should propagate the old mark bit to isNewlyAllocated so that it looks live
+            during marking. This is similar trick to MarkedBlock::aboutToMark.
+
+            * heap/PreciseAllocation.cpp:
+            (JSC::PreciseAllocation::flip):
+
 2020-09-10  Alan Coon  <alancoon@apple.com>
 

branches/safari-610.2.6.1-branch/Source/JavaScriptCore/heap/PreciseAllocation.cpp

@@ -214 +214 @@
 {
     ASSERT(heap()->collectionScope() == CollectionScope::Full);
-    clearMarked();
+    // Propagate the last time's mark bit to m_isNewlyAllocated so that `isLive` will say "yes" until this GC cycle finishes.
+    // After that, m_isNewlyAllocated is cleared again. So only previously marked or actually newly created objects survive.
+    // We do not need to care about concurrency here since marking thread is stopped right now. This is equivalent to the logic
+    // of MarkedBlock::aboutToMarkSlow.
+    // We invoke this function only when this is full collection. This ensures that at the end of upcoming cycle, we will
+    // clear NewlyAllocated bits of all objects. So this works correctly.
+    //
+    //                                      N: NewlyAllocated, M: Marked
+    //                                                 after this         at the end        When cycle
+    //                                            N M  function    N M     of cycle    N M  is finished   N M
+    // The live object survives the last cycle    0 1      =>      1 0        =>       1 1       =>       0 1    => live
+    // The dead object in the last cycle          0 0      =>      0 0        =>       0 0       =>       0 0    => dead
+    // The live object newly created after this            =>      1 0        =>       1 1       =>       0 1    => live
+    // The dead object newly created after this            =>      1 0        =>       1 0       =>       0 0    => dead
+    // The live object newly created before this  1 0      =>      1 0        =>       1 1       =>       0 1    => live
+    // The dead object newly created before this  1 0      =>      1 0        =>       1 0       =>       0 0    => dead
+    //                                                                                                    ^
+    //                                                              This is ensured since this function is used only for full GC.
+    m_isNewlyAllocated |= isMarked();
+    m_isMarked.store(false, std::memory_order_relaxed);
 }