Oilpan: Remove Heap::s_lastGCWasConservative

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 ceba01cc78a0ed9a7cacc45f0ab819dfe72bc81e..4e29550329bd5084b12d7db466e9c6dbe5cbe642 100644
--- a/Source/platform/heap/Heap.cpp
+++ b/Source/platform/heap/Heap.cpp
@@ -1694,9 +1694,6 @@ Address Heap::checkAndMarkPointer(Visitor* visitor, Address address)
         ASSERT(!page->orphaned());
         ASSERT(!s_heapDoesNotContainCache->lookup(address));
         page->checkAndMarkPointer(visitor, address);
-        // FIXME: We only need to set the conservative flag if
-        // checkAndMarkPointer actually marked the pointer.
-        s_lastGCWasConservative = true;
         return address;
     }
 
@@ -1900,8 +1897,6 @@ void Heap::collectGarbage(ThreadState::StackState stackState, ThreadState::GCTyp
     if (state->isMainThread())
         ScriptForbiddenScope::enter();
 
-    s_lastGCWasConservative = false;
-
     TRACE_EVENT2("blink_gc", "Heap::collectGarbage",
         "lazySweeping", gcType == ThreadState::GCWithoutSweep,
         "gcReason", gcReasonString(reason));
@@ -1925,19 +1920,18 @@ void Heap::collectGarbage(ThreadState::StackState stackState, ThreadState::GCTyp
     // 1. Trace persistent roots.
     ThreadState::visitPersistentRoots(s_markingVisitor);
 
-    // 2. Trace objects reachable from the persistent roots including
-    // ephemerons.
+    // 2. Trace objects reachable from the persistent roots.

[Erik Corry Chromium.org, 2015/05/30 22:21:50]

Either you don't need this step, or it should include precisely reachable data
that is found through ephemerons.  I think in fact you don't need this step at
all.

[haraken, 2015/06/01 01:29:37]

Good point. Done.

     processMarkingStack(s_markingVisitor);

[Erik Corry Chromium.org, 2015/05/30 22:21:50]

The effect of this was to first find all the precisely reachable objects,
including those that were reachable due to ephemeron rules.  I don't think there
is any correctness-reason to do this, but it does make it easier to put an upper
bound on the amount of data that conservative stack scanning is incorrectly
keeping alive (usually it's very small). 

Any data kept alive by the stack (whether correctly or due to conservatism) will
not be marked until step 4.

With this CL there is not the distinction between the precisely found data and
the conservatively found data.  In the current code base I think there is no way
to monitor or measure that difference anyway, so it does not matter, I think
whether the live data is found in separate phases, or whether the phases are
mixed up.  So I think this change is good, modulo the other comments I have.

[haraken, 2015/06/01 01:29:37]

I don't fully understand this part. How does the order of the precise marking
and the conservative scanning affect the amount of data that the conservative
scanning incorrectly keeps alive? I guess the amount of data kept alive
incorrectly won't depend on the order.

[Erik Corry Chromium.org, 2015/06/08 13:26:38]

It doesn't change the amount kept alive, it just makes it easier to measure it. 
Or rather, it makes it easier to measure an amount of memory that is an upper
bound of the incorrectly kept-alive memory.  However there is no actual code
currently for dumping or tracking this memory, so it's not a big deal.

 
     // 3. Trace objects reachable from the stack.  We do this independent of the
     // given stackState since other threads might have a different stack state.
     ThreadState::visitStackRoots(s_markingVisitor);
 
-    // 4. Trace objects reachable from the stack "roots" including ephemerons.
-    // Only do the processing if we found a pointer to an object on one of the
-    // thread stacks.
-    if (lastGCWasConservative())
-        processMarkingStack(s_markingVisitor);
+    // 4. Trace objects reachable from the stack "roots".
+    processMarkingStack(s_markingVisitor);

[sof, 2015/05/29 12:54:47]

Hmm, I think I prefer it the way it is on trunk, but the naming of the flag
could be clearer.

[haraken, 2015/05/29 14:38:24]

If I'm not missing something, this change reduces the number of ephemeron
iterations of a conservative GC, doesn't it? Before this CL, the ephemeron
iterations were done in both 2 and 4. After this CL, they are done only in 5.

+
+    // 5. Trace ephemerons.
+    processEphemerons(s_markingVisitor);

[Erik Corry Chromium.org, 2015/05/30 22:21:50]

This starts off by doing step 4, so step 4 is not needed AFAIKT.

 
     postMarkingProcessing(s_markingVisitor);
     globalWeakProcessing(s_markingVisitor);
@@ -1995,6 +1989,9 @@ void Heap::collectGarbageForTerminatingThread(ThreadState* state)
         // including ephemerons.
         processMarkingStack(&markingVisitor);
 
+        // 3. Trace ephemerons.
+        processEphemerons(&markingVisitor);

[Erik Corry Chromium.org, 2015/05/30 22:21:50]

This starts by doing step 2.

+
         postMarkingProcessing(&markingVisitor);
         globalWeakProcessing(&markingVisitor);
 
@@ -2005,6 +2002,14 @@ void Heap::collectGarbageForTerminatingThread(ThreadState* state)
 
 void Heap::processMarkingStack(Visitor* markingVisitor)
 {
+    // Iteratively mark all objects that are reachable from the objects
+    // currently pushed onto the marking stack.
+    TRACE_EVENT0("blink_gc", "Heap::processMarkingStackSingleThreaded");
+    while (popAndInvokeTraceCallback(markingVisitor)) { }
+}
+
+void Heap::processEphemerons(Visitor* markingVisitor)
+{
     // Ephemeron fixed point loop.
     do {
         {
@@ -2227,7 +2232,6 @@ CallbackStack* Heap::s_globalWeakCallbackStack;
 CallbackStack* Heap::s_ephemeronStack;
 HeapDoesNotContainCache* Heap::s_heapDoesNotContainCache;
 bool Heap::s_shutdownCalled = false;
-bool Heap::s_lastGCWasConservative = false;
 FreePagePool* Heap::s_freePagePool;
 OrphanedPagePool* Heap::s_orphanedPagePool;
 Heap::RegionTree* Heap::s_regionTree = nullptr;