Bug 1119537 - Make decommit a proper GC phase; r=jonco

js/src/builtin/TestingFunctions.cpp

@@ -798,6 +798,8 @@ GCState(JSContext* cx, unsigned argc, Value* vp)
         state = "finalize";
     else if (globalState == gc::COMPACT)
         state = "compact";
+    else if (globalState == gc::DECOMMIT)
+        state = "decommit";
     else
         MOZ_CRASH("Unobserveable global GC state");
 

js/src/gc/GCRuntime.h

@@ -81,7 +81,24 @@ class BackgroundAllocTask : public GCParallelTask
     bool enabled() const { return enabled_; }
 
   protected:
-    virtual void run() override;
+    void run() override;
+};
+
+// Search the provided Chunks for free arenas and decommit them.
+class BackgroundDecommitTask : public GCParallelTask
+{
+  public:
+    using ChunkVector = mozilla::Vector<Chunk*>;
+
+    explicit BackgroundDecommitTask(JSRuntime *rt) : runtime(rt) {}
+    void setChunksToScan(ChunkVector &chunks);
+
+  protected:
+    void run() override;
+
+  private:
+    JSRuntime* runtime;
+    ChunkVector toDecommit;
 };
 
 /*
@@ -950,7 +967,7 @@ class GCRuntime
     void endSweepPhase(bool lastGC);
     void sweepZones(FreeOp* fop, bool lastGC);
     void decommitAllWithoutUnlocking(const AutoLockGC& lock);
-    void decommitArenas(AutoLockGC& lock);
+    void startDecommit();
     void expireChunksAndArenas(bool shouldShrink, AutoLockGC& lock);
     void queueZonesForBackgroundSweep(ZoneList& zones);
     void sweepBackgroundThings(ZoneList& zones, LifoAlloc& freeBlocks, ThreadType threadType);
@@ -1327,6 +1344,7 @@ class GCRuntime
     mozilla::DebugOnly<mozilla::Atomic<PRThread*>> lockOwner;
 
     BackgroundAllocTask allocTask;
+    BackgroundDecommitTask decommitTask;
     GCHelperState helperState;
 
     /*

js/src/gc/Nursery.cpp

@@ -593,7 +593,7 @@ js::Nursery::FreeMallocedBuffersTask::transferBuffersToFree(MallocedBuffersSet&
 {
     // Transfer the contents of the source set to the task's buffers_ member by
     // swapping the sets, which also clears the source.
-    MOZ_ASSERT(!isRunning());
+    MOZ_ASSERT(!isRunningWithLockHeld());
     MOZ_ASSERT(buffers_.empty());
     mozilla::Swap(buffers_, buffersToFree);
 }

js/src/jit-test/tests/gc/incremental-state.js

@@ -12,6 +12,7 @@ assertEq(gcstate(), "none");
 // sized slices while background finalization is on-going, so we need to loop.
 gcslice(1000000);
 while (gcstate() == "finalize") { gcslice(1); }
+while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");
 
 // Incremental GC in multiple slices: if marking takes more than one slice,
@@ -23,6 +24,7 @@ gcslice(1000000);
 assertEq(gcstate(), "mark");
 gcslice(1000000);
 while (gcstate() == "finalize") { gcslice(1); }
+while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");
 
 // Zeal mode 8: Incremental GC in two main slices:
@@ -34,6 +36,7 @@ gcslice(1);
 assertEq(gcstate(), "mark");
 gcslice(1);
 while (gcstate() == "finalize") { gcslice(1); }
+while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");
 
 // Zeal mode 9: Incremental GC in two main slices:
@@ -45,6 +48,7 @@ gcslice(1);
 assertEq(gcstate(), "mark");
 gcslice(1);
 while (gcstate() == "finalize") { gcslice(1); }
+while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");
 
 // Zeal mode 10: Incremental GC in multiple slices (always yeilds before
@@ -55,4 +59,5 @@ gcslice(1000000);
 assertEq(gcstate(), "sweep");
 gcslice(1000000);
 while (gcstate() == "finalize") { gcslice(1); }
+while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");

js/src/jsgc.cpp

@@ -1074,7 +1074,7 @@ void
 GCRuntime::startBackgroundAllocTaskIfIdle()
 {
     AutoLockHelperThreadState helperLock;
-    if (allocTask.isRunning())
+    if (allocTask.isRunningWithLockHeld())
         return;
 
     // Join the previous invocation of the task. This will return immediately
@@ -1221,6 +1221,7 @@ GCRuntime::GCRuntime(JSRuntime* rt) :
 #endif
     lock(nullptr),
     allocTask(rt, emptyChunks_),
+    decommitTask(rt),
     helperState(rt)
 {
     setGCMode(JSGC_MODE_GLOBAL);
@@ -1404,6 +1405,7 @@ GCRuntime::finish()
      */
     helperState.finish();
     allocTask.cancel(GCParallelTask::CancelAndWait);
+    decommitTask.cancel(GCParallelTask::CancelAndWait);
 
 #ifdef JS_GC_ZEAL
     /* Free memory associated with GC verification. */
@@ -3405,43 +3407,67 @@ GCRuntime::decommitAllWithoutUnlocking(const AutoLockGC& lock)
 }
 
 void
-GCRuntime::decommitArenas(AutoLockGC& lock)
+GCRuntime::startDecommit()
 {
-    // Verify that all entries in the empty chunks pool are decommitted.
-    for (ChunkPool::Iter chunk(emptyChunks(lock)); !chunk.done(); chunk.next())
-        MOZ_ASSERT(!chunk->info.numArenasFreeCommitted);
+    MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
+    MOZ_ASSERT(!decommitTask.isRunning());
 
-    // Build a Vector of all current available Chunks. Since we release the
-    // gc lock while doing the decommit syscall, it is dangerous to iterate
-    // the available list directly, as concurrent operations can modify it.
-    mozilla::Vector<Chunk*> toDecommit;
-    MOZ_ASSERT(availableChunks(lock).verify());
-    for (ChunkPool::Iter iter(availableChunks(lock)); !iter.done(); iter.next()) {
-        if (!toDecommit.append(iter.get())) {
-            // The OOM handler does a full, immediate decommit, so there is
-            // nothing more to do here in any case.
-            return onOutOfMallocMemory(lock);
+    BackgroundDecommitTask::ChunkVector toDecommit;
+    {
+        AutoLockGC lock(rt);
+
+        // Verify that all entries in the empty chunks pool are already decommitted.
+        for (ChunkPool::Iter chunk(emptyChunks(lock)); !chunk.done(); chunk.next())
+            MOZ_ASSERT(!chunk->info.numArenasFreeCommitted);
+
+        // Since we release the GC lock while doing the decommit syscall below,
+        // it is dangerous to iterate the available list directly, as the main
+        // thread could modify it concurrently. Instead, we build and pass an
+        // explicit Vector containing the Chunks we want to visit.
+        MOZ_ASSERT(availableChunks(lock).verify());
+        for (ChunkPool::Iter iter(availableChunks(lock)); !iter.done(); iter.next()) {
+            if (!toDecommit.append(iter.get())) {
+                // The OOM handler does a full, immediate decommit.
+                return onOutOfMallocMemory(lock);
+            }
         }
     }
+    decommitTask.setChunksToScan(toDecommit);
 
-    // Start at the tail and stop before the first chunk: we allocate from the
-    // head and don't want to thrash with the mutator.
-    for (size_t i = toDecommit.length(); i > 1; --i) {
-        Chunk* chunk = toDecommit[i - 1];
-        MOZ_ASSERT(chunk);
+    if (sweepOnBackgroundThread && decommitTask.start())
+        return;
 
+    decommitTask.runFromMainThread(rt);
+}
+
+void
+js::gc::BackgroundDecommitTask::setChunksToScan(ChunkVector &chunks)
+{
+    MOZ_ASSERT(CurrentThreadCanAccessRuntime(runtime));
+    MOZ_ASSERT(!isRunning());
+    MOZ_ASSERT(toDecommit.empty());
+    Swap(toDecommit, chunks);
+}
+
+/* virtual */ void
+js::gc::BackgroundDecommitTask::run()
+{
+    AutoLockGC lock(runtime);
+
+    for (Chunk* chunk : toDecommit) {
         // The arena list is not doubly-linked, so we have to work in the free
         // list order and not in the natural order.
         while (chunk->info.numArenasFreeCommitted) {
-            bool ok = chunk->decommitOneFreeArena(rt, lock);
+            bool ok = chunk->decommitOneFreeArena(runtime, lock);
 
-            // FIXME Bug 1095620: add cancellation support when this becomes
-            // a ParallelTask.
-            if (/* cancel_ || */ !ok)
-                return;
+            // If we are low enough on memory that we can't update the page
+            // tables, or if we need to return for any other reason, break out
+            // of the loop.
+            if (cancel_ || !ok)
+                break;
         }
     }
-    MOZ_ASSERT(availableChunks(lock).verify());
+    toDecommit.clearAndFree();
 }
 
 void
@@ -3452,9 +3478,6 @@ GCRuntime::expireChunksAndArenas(bool shouldShrink, AutoLockGC& lock)
         AutoUnlockGC unlock(lock);
         FreeChunkPool(rt, toFree);
     }
-
-    if (shouldShrink)
-        decommitArenas(lock);
 }
 
 void
@@ -5764,6 +5787,7 @@ GCRuntime::endCompactPhase(JS::gcreason::Reason reason)
 void
 GCRuntime::finishCollection(JS::gcreason::Reason reason)
 {
+    assertBackgroundSweepingFinished();
     MOZ_ASSERT(marker.isDrained());
     marker.stop();
     clearBufferedGrayRoots();
@@ -5792,15 +5816,6 @@ GCRuntime::finishCollection(JS::gcreason::Reason reason)
     }
 
     lastGCTime = currentTime;
-
-    // If this is an OOM GC reason, wait on the background sweeping thread
-    // before returning to ensure that we free as much as possible. If this is
-    // a zeal-triggered GC, we want to ensure that the mutator can continue
-    // allocating on the same pages to reduce fragmentation.
-    if (IsOOMReason(reason) || reason == JS::gcreason::DEBUG_GC) {
-        gcstats::AutoPhase ap(stats, gcstats::PHASE_WAIT_BACKGROUND_THREAD);
-        rt->gc.waitBackgroundSweepOrAllocEnd();
-    }
 }
 
 static const char*
@@ -5981,6 +5996,12 @@ GCRuntime::resetIncrementalGC(const char* reason)
         break;
       }
 
+      case DECOMMIT: {
+        auto unlimited = SliceBudget::unlimited();
+        incrementalCollectSlice(unlimited, JS::gcreason::RESET);
+        break;
+      }
+
       default:
         MOZ_CRASH("Invalid incremental GC state");
     }
@@ -6243,13 +6264,28 @@ GCRuntime::incrementalCollectSlice(SliceBudget& budget, JS::gcreason::Reason rea
             endCompactPhase(reason);
         }
 
-        finishCollection(reason);
+        startDecommit();
+        incrementalState = DECOMMIT;
 
+        MOZ_FALLTHROUGH;
+
+      case DECOMMIT:
+        {
+            gcstats::AutoPhase ap(stats, gcstats::PHASE_WAIT_BACKGROUND_THREAD);
+
+            // Yield until background decommit is done.
+            if (isIncremental && decommitTask.isRunning())
+                break;
+
+            decommitTask.join();
+        }
+
+        finishCollection(reason);
         incrementalState = NO_INCREMENTAL;
         break;
 
       default:
-        MOZ_ASSERT(false);
+        MOZ_CRASH("unexpected GC incrementalState");
     }
 }
 
@@ -6378,10 +6414,12 @@ GCRuntime::gcCycle(bool nonincrementalByAPI, SliceBudget& budget, JS::gcreason::
     {
         gcstats::AutoPhase ap(stats, gcstats::PHASE_WAIT_BACKGROUND_THREAD);
 
-        // As we are about to clear the mark bits, wait for background
-        // finalization to finish. We only need to wait on the first slice.
-        if (!isIncrementalGCInProgress())
-            waitBackgroundSweepEnd();
+        // Background finalization and decommit are finished by defininition
+        // before we can start a new GC session.
+        if (!isIncrementalGCInProgress()) {
+            assertBackgroundSweepingFinished();
+            MOZ_ASSERT(!decommitTask.isRunning());
+        }
 
         // We must also wait for background allocation to finish so we can
         // avoid taking the GC lock when manipulating the chunks during the GC.
@@ -6718,6 +6756,9 @@ GCRuntime::onOutOfMallocMemory()
     // Stop allocating new chunks.
     allocTask.cancel(GCParallelTask::CancelAndWait);
 
+    // Make sure we release anything queued for release.
+    decommitTask.join();
+
     // Wait for background free of nursery huge slots to finish.
     nursery.waitBackgroundFreeEnd();
 

js/src/jsgc.h

@@ -49,7 +49,8 @@ enum State {
     MARK,
     SWEEP,
     FINALIZE,
-    COMPACT
+    COMPACT,
+    DECOMMIT
 };
 
 /* Map from C++ type to alloc kind. JSObject does not have a 1:1 mapping, so must use Arena::thingSize. */
@@ -1028,6 +1029,7 @@ class GCParallelTask
     }
 
     // Check if a task is actively running.
+    bool isRunningWithLockHeld() const;
     bool isRunning() const;
 
     // This should be friended to HelperThread, but cannot be because it

js/src/vm/HelperThreads.cpp

@@ -1089,12 +1089,19 @@ js::GCParallelTask::runFromHelperThread()
 }
 
 bool
-js::GCParallelTask::isRunning() const
+js::GCParallelTask::isRunningWithLockHeld() const
 {
     MOZ_ASSERT(HelperThreadState().isLocked());
     return state == Dispatched;
 }
 
+bool
+js::GCParallelTask::isRunning() const
+{
+    AutoLockHelperThreadState helperLock;
+    return isRunningWithLockHeld();
+}
+
 void
 HelperThread::handleGCParallelWorkload()
 {