Oilpan: Remove Heap::s_lastGCWasConservative

Source/platform/heap/Heap.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 1676 matching lines @@
 #if !ENABLE(ASSERT)
     if (s_heapDoesNotContainCache->lookup(address))
         return nullptr;
 #endif
 
     if (BasePage* page = lookup(address)) {
         ASSERT(page->contains(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;
     }
 
 #if !ENABLE(ASSERT)
     s_heapDoesNotContainCache->addEntry(address);
 #else
     if (!s_heapDoesNotContainCache->lookup(address))
         s_heapDoesNotContainCache->addEntry(address);
 #endif
     return nullptr;
@@ skipping 183 matching lines @@
     if (!gcScope.allThreadsParked()) {
         // Restore the original GCState.
         if (LIKELY(state->gcState() == ThreadState::StoppingOtherThreads))
             state->setGCState(originalGCState);
         return;
     }
 
     if (state->isMainThread())
         ScriptForbiddenScope::enter();
 
-    s_lastGCWasConservative = false;
-
     TRACE_EVENT2("blink_gc", "Heap::collectGarbage",
         "lazySweeping", gcType == ThreadState::GCWithoutSweep,
         "gcReason", gcReasonString(reason));
     TRACE_EVENT_SCOPED_SAMPLING_STATE("blink_gc", "BlinkGC");
     double timeStamp = WTF::currentTimeMS();
 #if ENABLE(GC_PROFILING)
     static_cast<MarkingVisitor<Visitor::GlobalMarking>*>(s_markingVisitor)->objectGraph().clear();
 #endif
 
     // Disallow allocation during garbage collection (but not during the
     // finalization that happens when the gcScope is torn down).
     ThreadState::NoAllocationScope noAllocationScope(state);
 
     preGC();
 
     StackFrameDepthScope stackDepthScope;
 
     size_t totalObjectSize = Heap::allocatedObjectSize() + Heap::markedObjectSize();
     Heap::resetHeapCounters();
 
     // 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);
 
     // Now we can delete all orphaned pages because there are no dangling
     // pointers to the orphaned pages.  (If we have such dangling pointers,
     // we should have crashed during marking before getting here.)
     orphanedPagePool()->decommitOrphanedPages();
 
     postGC(gcType);
@@ skipping 37 matching lines @@
         // only cases where this could happen is if a subsequent conservative
         // global GC finds a "pointer" on the stack or due to a programming
         // error where an object has a dangling cross-thread pointer to an
         // object on this heap.
         state->visitPersistents(&markingVisitor);
 
         // 2. Trace objects reachable from the thread's persistent roots
         // 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);
 
         state->postGC(ThreadState::GCWithSweep);
     }
     state->preSweep();
 }
 
 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 {
         {
             // 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)) { }
         }
 
         {
@@ skipping 202 matching lines @@
     s_externalObjectSizeAtLastGC = WTF::Partitions::totalSizeOfCommittedPages();
 }
 
 Visitor* Heap::s_markingVisitor;
 CallbackStack* Heap::s_markingStack;
 CallbackStack* Heap::s_postMarkingCallbackStack;
 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;
 size_t Heap::s_allocatedObjectSize = 0;
 size_t Heap::s_allocatedSpace = 0;
 size_t Heap::s_markedObjectSize = 0;
 // We don't want to use 0 KB for the initial value because it may end up
 // triggering the first GC of some thread too prematurely.
 size_t Heap::s_estimatedLiveObjectSize = 512 * 1024;
 size_t Heap::s_externalObjectSizeAtLastGC = 0;
 double Heap::s_estimatedMarkingTimePerByte = 0.0;
 
 } // namespace blink