Changeset 267304 in webkit

Timestamp:

Sep 18, 2020, 6:22:42 PM (6 years ago)

Author:

ysuzuki@apple.com

Message:

[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):

Location:

trunk/Source/JavaScriptCore

Files:

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

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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-18  Saam Barati  <sbarati@apple.com>
 

trunk/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);
 }