Simple BlinkGC heap compaction.

third_party/WebKit/Source/platform/heap/ThreadState.cpp

Index: third_party/WebKit/Source/platform/heap/ThreadState.cpp
diff --git a/third_party/WebKit/Source/platform/heap/ThreadState.cpp b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
index 9057cec1faf2a469839695d299c41d01ff2d33ec..a433dff585f7b32e4e3816e19b1fd34e005af7f0 100644
--- a/third_party/WebKit/Source/platform/heap/ThreadState.cpp
+++ b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
@@ -38,6 +38,7 @@
 #include "platform/heap/CallbackStack.h"
 #include "platform/heap/Handle.h"
 #include "platform/heap/Heap.h"
+#include "platform/heap/HeapCompact.h"
 #include "platform/heap/PagePool.h"
 #include "platform/heap/SafePoint.h"
 #include "platform/heap/Visitor.h"
@@ -79,7 +80,7 @@ uint8_t ThreadState::s_mainThreadStateStorage[sizeof(ThreadState)];
 
 const size_t defaultAllocatedObjectSizeThreshold = 100 * 1024;
 
-const char* gcReasonString(BlinkGC::GCReason reason) {
+const char* ThreadState::gcReasonString(BlinkGC::GCReason reason) {
   switch (reason) {
     case BlinkGC::IdleGC:
       return "IdleGC";
@@ -504,6 +505,7 @@ void ThreadState::threadLocalWeakProcessing() {
   // Due to the complexity, we just forbid allocations.
   NoAllocationScope noAllocationScope(this);
 
+  GCForbiddenScope gcForbiddenScope(this);

[haraken, 2016/12/09 07:25:56]

I'm curious why you need this. Isn't SweepForbiddenScope enough for preventing
GCs from getting triggered during the weak processing?

[sof, 2016/12/09 21:44:05]

See the Visitor constructor, rather than having it enter a GC forbidden scope
during its lifetime, we instead DCHECK() that we are when creating. Hence all
Visitor instances must be created while in such a scope - it seems best not to
try to overturn that setup here, and perhaps be more discriminating.

   std::unique_ptr<Visitor> visitor =
       Visitor::create(this, BlinkGC::ThreadLocalWeakProcessing);
 
@@ -1040,6 +1042,33 @@ void ThreadState::makeConsistentForGC() {
     m_arenas[i]->makeConsistentForGC();
 }
 
+void ThreadState::compact() {
+  if (!heap().compaction()->isCompacting())
+    return;
+
+  SweepForbiddenScope scope(this);
+  ScriptForbiddenIfMainThreadScope scriptForbiddenScope;

[haraken, 2016/12/09 07:25:56]

Also shall we add NoAllocationScope just in case?

[sof, 2016/12/09 21:44:05]

Done.

+
+  // Compaction is done eagerly and before the mutator threads get
+  // to run again. Doing it lazily is problematic, as the mutator's
+  // references to live objects could suddenly be invalidated by
+  // compaction of a page/heap. We do know all the references to
+  // the relocating objects just after marking, but won't later.
+  // (e.g., stack references could have been created, new objects
+  // created which refer to old collection objects, and so on.)
+
+  // Compact the hash table backing store arena first, it usually has
+  // higher fragmentation and is larger.
+  //
+  // TODO: implement bail out wrt any overall deadline, not compacting
+  // the remaining arenas if the time budget has been exceeded.
+  heap().compaction()->startThreadCompaction(this);
+  for (int i = BlinkGC::HashTableArenaIndex; i >= BlinkGC::Vector1ArenaIndex;
+       --i)
+    static_cast<NormalPageArena*>(m_arenas[i])->sweepAndCompact();
+  heap().compaction()->finishedThreadCompaction(this);
+}
+
 void ThreadState::makeConsistentForMutator() {
   ASSERT(isInGC());
   for (int i = 0; i < BlinkGC::NumberOfArenas; ++i)
@@ -1123,9 +1152,12 @@ void ThreadState::preSweep() {
 
   eagerSweep();
 
+  compact();

[haraken, 2016/12/09 07:25:56]

Add a comment and mention why this must be put after pre-finalization and eager
sweeping.

[sof, 2016/12/09 21:44:05]

Done.

+
 #if defined(ADDRESS_SANITIZER)
   poisonAllHeaps();
 #endif
+
   if (previousGCState == EagerSweepScheduled) {
     // Eager sweeping should happen only in testing.
     completeSweep();
@@ -1674,7 +1706,7 @@ void ThreadState::collectGarbage(BlinkGC::StackState stackState,
   RELEASE_ASSERT(!isGCForbidden());
   completeSweep();
 
-  std::unique_ptr<Visitor> visitor = Visitor::create(this, gcType);
+  GCForbiddenScope gcForbiddenScope(this);

[haraken, 2016/12/09 07:25:56]

I'm just curious but why did you add this? (I'm fine with adding it.)

[sof, 2016/12/09 21:44:05]

See the Visitor constructor comment, we had to introduce a GC forbidden lock
this early on to avoid some thread interference in the past. (I don't have the
bug references at hand).

This just preserves that; making changes in this area is high risk.

 
   SafePointScope safePointScope(stackState, this);
 
@@ -1685,6 +1717,12 @@ void ThreadState::collectGarbage(BlinkGC::StackState stackState,
   if (!parkThreadsScope.parkThreads())
     return;
 
+  BlinkGC::GCType visitorType = gcType;
+  if (heap().compaction()->shouldCompact(this, gcType, reason))
+    visitorType = heap().compaction()->initialize(this);

[haraken, 2016/12/09 07:25:56]

I'd prefer updating |gcType| instead of using |visitorType|, since the rest of
this function uses |gcType|.

[sof, 2016/12/09 21:44:05]

Notice that we do lose the with/without sweeping distinction for |gcType| when
creating the visitor via |visitorType|. We need to preserve that for later
|gcType| uses, which is why it is done this way.

[haraken, 2016/12/12 01:25:10]

Ah, got it. Would it be tidier to use a boolean flag (or enum) to specify if the
visitor should do the compaction or not?

  Visitor::create(this, gcType, ShouldCompactHeap);

+
+  std::unique_ptr<Visitor> visitor = Visitor::create(this, visitorType);
+
   ScriptForbiddenIfMainThreadScope scriptForbidden;
 
   TRACE_EVENT2("blink_gc,devtools.timeline", "BlinkGCMarking", "lazySweeping",
@@ -1785,6 +1823,7 @@ void ThreadState::collectGarbageForTerminatingThread() {
     // ahead while it is running, hence the termination GC does not enter a
     // safepoint. VisitorScope will not enter also a safepoint scope for
     // ThreadTerminationGC.
+    GCForbiddenScope gcForbiddenScope(this);
     std::unique_ptr<Visitor> visitor =
         Visitor::create(this, BlinkGC::ThreadTerminationGC);