Backed out changeset 2a613f5a5866 (bug 1119537) for hazard failures

MozReview-Commit-ID: HjSuYxycsVu

js/src/builtin/TestingFunctions.cpp

@@ -798,8 +798,6 @@ 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,24 +81,7 @@ class BackgroundAllocTask : public GCParallelTask
     bool enabled() const { return enabled_; }
 
   protected:
-    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;
+    virtual void run() override;
 };
 
 /*
@@ -967,7 +950,7 @@ class GCRuntime
     void endSweepPhase(bool lastGC);
     void sweepZones(FreeOp* fop, bool lastGC);
     void decommitAllWithoutUnlocking(const AutoLockGC& lock);
-    void startDecommit();
+    void decommitArenas(AutoLockGC& lock);
     void expireChunksAndArenas(bool shouldShrink, AutoLockGC& lock);
     void queueZonesForBackgroundSweep(ZoneList& zones);
     void sweepBackgroundThings(ZoneList& zones, LifoAlloc& freeBlocks, ThreadType threadType);
@@ -1344,7 +1327,6 @@ 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(!isRunningWithLockHeld());
+    MOZ_ASSERT(!isRunning());
     MOZ_ASSERT(buffers_.empty());
     mozilla::Swap(buffers_, buffersToFree);
 }

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

@@ -12,7 +12,6 @@ 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,
@@ -24,7 +23,6 @@ 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:
@@ -36,7 +34,6 @@ 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:
@@ -48,7 +45,6 @@ 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
@@ -59,5 +55,4 @@ gcslice(1000000);
 assertEq(gcstate(), "sweep");
 gcslice(1000000);
 while (gcstate() == "finalize") { gcslice(1); }
-while (gcstate() == "decommit") { gcslice(1); }
 assertEq(gcstate(), "none");

js/src/jsgc.cpp

@@ -1031,7 +1031,7 @@ void
 GCRuntime::startBackgroundAllocTaskIfIdle()
 {
     AutoLockHelperThreadState helperLock;
-    if (allocTask.isRunningWithLockHeld())
+    if (allocTask.isRunning())
         return;
 
     // Join the previous invocation of the task. This will return immediately
@@ -1178,7 +1178,6 @@ GCRuntime::GCRuntime(JSRuntime* rt) :
 #endif
     lock(nullptr),
     allocTask(rt, emptyChunks_),
-    decommitTask(rt),
     helperState(rt)
 {
     setGCMode(JSGC_MODE_GLOBAL);
@@ -1362,7 +1361,6 @@ GCRuntime::finish()
      */
     helperState.finish();
     allocTask.cancel(GCParallelTask::CancelAndWait);
-    decommitTask.cancel(GCParallelTask::CancelAndWait);
 
 #ifdef JS_GC_ZEAL
     /* Free memory associated with GC verification. */
@@ -3341,67 +3339,43 @@ GCRuntime::decommitAllWithoutUnlocking(const AutoLockGC& lock)
 }
 
 void
-GCRuntime::startDecommit()
+GCRuntime::decommitArenas(AutoLockGC& lock)
 {
-    MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
-    MOZ_ASSERT(!decommitTask.isRunning());
+    // 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);
 
-    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);
-            }
+    // 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);
         }
     }
-    decommitTask.setChunksToScan(toDecommit);
 
-    if (sweepOnBackgroundThread && decommitTask.start())
-        return;
+    // 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);
 
-    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(runtime, lock);
+            bool ok = chunk->decommitOneFreeArena(rt, lock);
 
-            // 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;
+            // FIXME Bug 1095620: add cancellation support when this becomes
+            // a ParallelTask.
+            if (/* cancel_ || */ !ok)
+                return;
         }
     }
-    toDecommit.clearAndFree();
+    MOZ_ASSERT(availableChunks(lock).verify());
 }
 
 void
@@ -3412,6 +3386,9 @@ GCRuntime::expireChunksAndArenas(bool shouldShrink, AutoLockGC& lock)
         AutoUnlockGC unlock(lock);
         FreeChunkPool(rt, toFree);
     }
+
+    if (shouldShrink)
+        decommitArenas(lock);
 }
 
 void
@@ -5721,7 +5698,6 @@ GCRuntime::endCompactPhase(JS::gcreason::Reason reason)
 void
 GCRuntime::finishCollection(JS::gcreason::Reason reason)
 {
-    assertBackgroundSweepingFinished();
     MOZ_ASSERT(marker.isDrained());
     marker.stop();
     clearBufferedGrayRoots();
@@ -5750,6 +5726,15 @@ 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*
@@ -5898,12 +5883,6 @@ 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");
     }
@@ -6165,28 +6144,13 @@ GCRuntime::incrementalCollectSlice(SliceBudget& budget, JS::gcreason::Reason rea
             endCompactPhase(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_CRASH("unexpected GC incrementalState");
+        MOZ_ASSERT(false);
     }
 }
 
@@ -6315,12 +6279,10 @@ GCRuntime::gcCycle(bool nonincrementalByAPI, SliceBudget& budget, JS::gcreason::
     {
         gcstats::AutoPhase ap(stats, gcstats::PHASE_WAIT_BACKGROUND_THREAD);
 
-        // Background finalization and decommit are finished by defininition
-        // before we can start a new GC session.
-        if (!isIncrementalGCInProgress()) {
-            assertBackgroundSweepingFinished();
-            MOZ_ASSERT(!decommitTask.isRunning());
-        }
+        // 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();
 
         // We must also wait for background allocation to finish so we can
         // avoid taking the GC lock when manipulating the chunks during the GC.
@@ -6657,9 +6619,6 @@ 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,8 +49,7 @@ enum State {
     MARK,
     SWEEP,
     FINALIZE,
-    COMPACT,
-    DECOMMIT
+    COMPACT
 };
 
 /* Map from C++ type to alloc kind. JSObject does not have a 1:1 mapping, so must use Arena::thingSize. */
@@ -962,7 +961,6 @@ 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,19 +1089,12 @@ js::GCParallelTask::runFromHelperThread()
 }
 
 bool
-js::GCParallelTask::isRunningWithLockHeld() const
+js::GCParallelTask::isRunning() const
 {
     MOZ_ASSERT(HelperThreadState().isLocked());
     return state == Dispatched;
 }
 
-bool
-js::GCParallelTask::isRunning() const
-{
-    AutoLockHelperThreadState helperLock;
-    return isRunningWithLockHeld();
-}
-
 void
 HelperThread::handleGCParallelWorkload()
 {