Bug 1465108 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings a=RyanVM
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 10 May 2018 10:09:31 +0100
changeset 802286 6983f5845e9bb0cc79ee9eafb017f41a652970ad
parent 802285 095a9b56bb34649dea0e9bb8f38b523e33c3c9c0
child 802287 164918e56cb9029ab6d72f535ba6955f8fbc9021
push id111850
push userbmo:tom@mozilla.com
push dateThu, 31 May 2018 16:41:37 +0000
reviewerssfink, abillings, RyanVM
bugs1465108
milestone60.0.2
Bug 1465108 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings a=RyanVM
js/src/gc/Allocator.cpp
js/src/gc/GC.cpp
js/src/gc/GCParallelTask.h
js/src/gc/GCRuntime.h
js/src/gc/Nursery.cpp
js/src/gc/Statistics.h
js/src/vm/HelperThreads.cpp
--- a/js/src/gc/Allocator.cpp
+++ b/js/src/gc/Allocator.cpp
@@ -638,17 +638,17 @@ GCRuntime::pickChunk(AutoLockGCBgAlloc& 
     chunkAllocationSinceLastGC = true;
 
     availableChunks(lock).push(chunk);
 
     return chunk;
 }
 
 BackgroundAllocTask::BackgroundAllocTask(JSRuntime* rt, ChunkPool& pool)
-  : GCParallelTask(rt),
+  : GCParallelTaskHelper(rt),
     chunkPool_(pool),
     enabled_(CanUseExtraThreads() && GetCPUCount() >= 2)
 {
 }
 
 /* virtual */ void
 BackgroundAllocTask::run()
 {
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -2615,39 +2615,38 @@ ArenasToUpdate::getArenasToUpdate(AutoLo
         last = last->next;
         count++;
     }
 
     arena = last;
     return { begin, last->next };
 }
 
-struct UpdatePointersTask : public GCParallelTask
+struct UpdatePointersTask : public GCParallelTaskHelper<UpdatePointersTask>
 {
     // Maximum number of arenas to update in one block.
 #ifdef DEBUG
     static const unsigned MaxArenasToProcess = 16;
 #else
     static const unsigned MaxArenasToProcess = 256;
 #endif
 
     UpdatePointersTask(JSRuntime* rt, ArenasToUpdate* source, AutoLockHelperThreadState& lock)
-      : GCParallelTask(rt), source_(source)
+      : GCParallelTaskHelper(rt), source_(source)
     {
         arenas_.begin = nullptr;
         arenas_.end = nullptr;
     }
 
-    ~UpdatePointersTask() override { join(); }
+    void run();
 
   private:
     ArenasToUpdate* source_;
     ArenaListSegment arenas_;
 
-    virtual void run() override;
     bool getArenasToUpdate();
     void updateArenas();
 };
 
 bool
 UpdatePointersTask::getArenasToUpdate()
 {
     AutoLockHelperThreadState lock;
@@ -3910,40 +3909,32 @@ ArenaLists::checkEmptyArenaList(AllocKin
         }
     }
 #endif // DEBUG
     return isEmpty;
 }
 
 class MOZ_RAII js::gc::AutoRunParallelTask : public GCParallelTask
 {
-    using Func = void (*)(JSRuntime*);
-
-    Func func_;
     gcstats::PhaseKind phase_;
     AutoLockHelperThreadState& lock_;
 
   public:
-    AutoRunParallelTask(JSRuntime* rt, Func func, gcstats::PhaseKind phase,
+    AutoRunParallelTask(JSRuntime* rt, TaskFunc func, gcstats::PhaseKind phase,
                         AutoLockHelperThreadState& lock)
-      : GCParallelTask(rt),
-        func_(func),
+      : GCParallelTask(rt, func),
         phase_(phase),
         lock_(lock)
     {
         runtime()->gc.startTask(*this, phase_, lock_);
     }
 
     ~AutoRunParallelTask() {
         runtime()->gc.joinTask(*this, phase_, lock_);
     }
-
-    void run() override {
-        func_(runtime());
-    }
 };
 
 void
 GCRuntime::purgeRuntimeForMinorGC()
 { 
     // If external strings become nursery allocable, remember to call
     // zone->externalStringCache().purge() (and delete this assert.)
     MOZ_ASSERT(!IsNurseryAllocable(AllocKind::EXTERNAL_STRING));
@@ -4244,33 +4235,34 @@ PurgeShapeTablesForShrinkingGC(JSRuntime
         if (zone->keepShapeTables() || zone->isSelfHostingZone())
             continue;
         for (auto baseShape = zone->cellIter<BaseShape>(); !baseShape.done(); baseShape.next())
             baseShape->maybePurgeTable();
     }
 }
 
 static void
-UnmarkCollectedZones(JSRuntime* rt)
-{
+UnmarkCollectedZones(GCParallelTask* task)
+{
+    JSRuntime* rt = task->runtime();
     for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
         /* Unmark everything in the zones being collected. */
         zone->arenas.unmarkAll();
     }
 
     for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
         /* Unmark all weak maps in the zones being collected. */
         WeakMapBase::unmarkZone(zone);
     }
 }
 
 static void
-BufferGrayRoots(JSRuntime* rt)
-{
-    rt->gc.bufferGrayRoots();
+BufferGrayRoots(GCParallelTask* task)
+{
+    task->runtime()->gc.bufferGrayRoots();
 }
 
 bool
 GCRuntime::beginMarkPhase(JS::gcreason::Reason reason, AutoTraceSession& session)
 {
     MOZ_ASSERT(session.maybeLock.isSome());
 
 #ifdef DEBUG
@@ -5306,39 +5298,41 @@ GCRuntime::endMarkingSweepGroup(FreeOp* 
 
     /* We must not yield after this point before we start sweeping the group. */
     safeToYield = false;
 
     return Finished;
 }
 
 // Causes the given WeakCache to be swept when run.
-class ImmediateSweepWeakCacheTask : public GCParallelTask
+class ImmediateSweepWeakCacheTask : public GCParallelTaskHelper<ImmediateSweepWeakCacheTask>
 {
     JS::detail::WeakCacheBase& cache;
 
     ImmediateSweepWeakCacheTask(const ImmediateSweepWeakCacheTask&) = delete;
 
   public:
     ImmediateSweepWeakCacheTask(JSRuntime* rt, JS::detail::WeakCacheBase& wc)
-      : GCParallelTask(rt), cache(wc)
+      : GCParallelTaskHelper(rt), cache(wc)
     {}
 
     ImmediateSweepWeakCacheTask(ImmediateSweepWeakCacheTask&& other)
-      : GCParallelTask(Move(other)), cache(other.cache)
+      : GCParallelTaskHelper(Move(other)), cache(other.cache)
     {}
 
-    void run() override {
+    void run() {
         cache.sweep();
     }
 };
 
 static void
-UpdateAtomsBitmap(JSRuntime* runtime)
-{
+UpdateAtomsBitmap(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
+
     DenseBitmap marked;
     if (runtime->gc.atomMarking.computeBitmapFromChunkMarkBits(runtime, marked)) {
         for (GCZonesIter zone(runtime); !zone.done(); zone.next())
             runtime->gc.atomMarking.updateZoneBitmap(zone, marked);
     } else {
         // Ignore OOM in computeBitmapFromChunkMarkBits. The updateZoneBitmap
         // call can only remove atoms from the zone bitmap, so it is
         // conservative to just not call it.
@@ -5349,74 +5343,81 @@ UpdateAtomsBitmap(JSRuntime* runtime)
     // For convenience sweep these tables non-incrementally as part of bitmap
     // sweeping; they are likely to be much smaller than the main atoms table.
     runtime->unsafeSymbolRegistry().sweep();
     for (CompartmentsIter comp(runtime, SkipAtoms); !comp.done(); comp.next())
         comp->sweepVarNames();
 }
 
 static void
-SweepCCWrappers(JSRuntime* runtime)
-{
+SweepCCWrappers(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
     for (SweepGroupCompartmentsIter c(runtime); !c.done(); c.next())
         c->sweepCrossCompartmentWrappers();
 }
 
 static void
-SweepObjectGroups(JSRuntime* runtime)
-{
+SweepObjectGroups(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
     for (SweepGroupCompartmentsIter c(runtime); !c.done(); c.next())
         c->objectGroups.sweep();
 }
 
 static void
-SweepRegExps(JSRuntime* runtime)
-{
+SweepRegExps(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
     for (SweepGroupCompartmentsIter c(runtime); !c.done(); c.next())
         c->sweepRegExps();
 }
 
 static void
-SweepMisc(JSRuntime* runtime)
-{
+SweepMisc(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
     for (SweepGroupCompartmentsIter c(runtime); !c.done(); c.next()) {
         c->sweepGlobalObject();
         c->sweepTemplateObjects();
         c->sweepSavedStacks();
         c->sweepSelfHostingScriptSource();
         c->sweepNativeIterators();
     }
 }
 
 static void
-SweepCompressionTasks(JSRuntime* runtime)
-{
+SweepCompressionTasks(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
+
     AutoLockHelperThreadState lock;
 
     // Attach finished compression tasks.
     auto& finished = HelperThreadState().compressionFinishedList(lock);
     for (size_t i = 0; i < finished.length(); i++) {
         if (finished[i]->runtimeMatches(runtime)) {
-            UniquePtr<SourceCompressionTask> task(Move(finished[i]));
+            UniquePtr<SourceCompressionTask> compressionTask(Move(finished[i]));
             HelperThreadState().remove(finished, &i);
-            task->complete();
+            compressionTask->complete();
         }
     }
 
     // Sweep pending tasks that are holding onto should-be-dead ScriptSources.
     auto& pending = HelperThreadState().compressionPendingList(lock);
     for (size_t i = 0; i < pending.length(); i++) {
         if (pending[i]->shouldCancel())
             HelperThreadState().remove(pending, &i);
     }
 }
 
 static void
-SweepWeakMaps(JSRuntime* runtime)
-{
+SweepWeakMaps(GCParallelTask* task)
+{
+    JSRuntime* runtime = task->runtime();
     for (SweepGroupZonesIter zone(runtime); !zone.done(); zone.next()) {
         /* Clear all weakrefs that point to unmarked things. */
         for (auto edge : zone->gcWeakRefs()) {
             /* Edges may be present multiple times, so may already be nulled. */
             if (*edge && IsAboutToBeFinalizedDuringSweep(**edge))
                 *edge = nullptr;
         }
         zone->gcWeakRefs().clear();
@@ -5426,34 +5427,36 @@ SweepWeakMaps(JSRuntime* runtime)
         if (!zone->gcWeakKeys().clear())
             oomUnsafe.crash("clearing weak keys in beginSweepingSweepGroup()");
 
         zone->sweepWeakMaps();
     }
 }
 
 static void
-SweepUniqueIds(JSRuntime* runtime)
-{
-    for (SweepGroupZonesIter zone(runtime); !zone.done(); zone.next())
+SweepUniqueIds(GCParallelTask* task)
+{
+    for (SweepGroupZonesIter zone(task->runtime()); !zone.done(); zone.next())
         zone->sweepUniqueIds();
 }
 
 void
-GCRuntime::startTask(GCParallelTask& task, gcstats::PhaseKind phase, AutoLockHelperThreadState& locked)
+GCRuntime::startTask(GCParallelTask& task, gcstats::PhaseKind phase,
+                     AutoLockHelperThreadState& locked)
 {
     if (!task.startWithLockHeld(locked)) {
         AutoUnlockHelperThreadState unlock(locked);
         gcstats::AutoPhase ap(stats(), phase);
         task.runFromActiveCooperatingThread(rt);
     }
 }
 
 void
-GCRuntime::joinTask(GCParallelTask& task, gcstats::PhaseKind phase, AutoLockHelperThreadState& locked)
+GCRuntime::joinTask(GCParallelTask& task, gcstats::PhaseKind phase,
+                    AutoLockHelperThreadState& locked)
 {
     {
         gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::JOIN_PARALLEL_TASKS);
         task.joinWithLockHeld(locked);
     }
     stats().recordParallelPhase(phase, task.duration());
 }
 
@@ -6025,39 +6028,38 @@ class js::gc::WeakCacheSweepIterator
 
   private:
     void checkState() {
         MOZ_ASSERT((!sweepZone && !sweepCache) ||
                    (sweepCache && sweepCache->needsIncrementalBarrier()));
     }
 };
 
-class IncrementalSweepWeakCacheTask : public GCParallelTask
+class IncrementalSweepWeakCacheTask : public GCParallelTaskHelper<IncrementalSweepWeakCacheTask>
 {
     WeakCacheSweepIterator& work_;
     SliceBudget& budget_;
     AutoLockHelperThreadState& lock_;
     JS::detail::WeakCacheBase* cache_;
 
   public:
     IncrementalSweepWeakCacheTask(JSRuntime* rt, WeakCacheSweepIterator& work, SliceBudget& budget,
                                   AutoLockHelperThreadState& lock)
-      : GCParallelTask(rt), work_(work), budget_(budget), lock_(lock),
+      : GCParallelTaskHelper(rt), work_(work), budget_(budget), lock_(lock),
         cache_(work.next(lock))
     {
         MOZ_ASSERT(cache_);
         runtime()->gc.startTask(*this, gcstats::PhaseKind::SWEEP_WEAK_CACHES, lock_);
     }
 
     ~IncrementalSweepWeakCacheTask() {
         runtime()->gc.joinTask(*this, gcstats::PhaseKind::SWEEP_WEAK_CACHES, lock_);
     }
 
-  private:
-    void run() override {
+    void run() {
         do {
             MOZ_ASSERT(cache_->needsIncrementalBarrier());
             size_t steps = cache_->sweep();
             cache_->setNeedsIncrementalBarrier(false);
 
             AutoLockHelperThreadState lock;
             budget_.step(steps);
             if (budget_.isOverBudget())
--- a/js/src/gc/GCParallelTask.h
+++ b/js/src/gc/GCParallelTask.h
@@ -2,27 +2,37 @@
  * vim: set ts=8 sts=4 et sw=4 tw=99:
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef gc_GCParallelTask_h
 #define gc_GCParallelTask_h
 
+#include "mozilla/Move.h"
+
 #include "js/TypeDecls.h"
 #include "threading/ProtectedData.h"
 
 namespace js {
 
 // A generic task used to dispatch work to the helper thread system.
-// Users should derive from GCParallelTask add what data they need and
-// override |run|.
+// Users supply a function pointer to call.
+//
+// Note that we don't use virtual functions here because destructors can write
+// the vtable pointer on entry, which can causes races if synchronization
+// happens there.
 class GCParallelTask
 {
+  public:
+    using TaskFunc = void (*)(GCParallelTask*);
+
+  private:
     JSRuntime* const runtime_;
+    TaskFunc func_;
 
     // The state of the parallel computation.
     enum TaskState {
         NotStarted,
         Dispatched,
         Finished,
     };
     UnprotectedData<TaskState> state;
@@ -31,30 +41,36 @@ class GCParallelTask
     ActiveThreadOrGCTaskData<mozilla::TimeDuration> duration_;
 
     explicit GCParallelTask(const GCParallelTask&) = delete;
 
   protected:
     // A flag to signal a request for early completion of the off-thread task.
     mozilla::Atomic<bool> cancel_;
 
-    virtual void run() = 0;
+  public:
+    explicit GCParallelTask(JSRuntime* runtime, TaskFunc func)
+      : runtime_(runtime),
+        func_(func),
+        state(NotStarted),
+        duration_(nullptr),
+        cancel_(false)
+    {}
 
-  public:
-    explicit GCParallelTask(JSRuntime* runtime) : runtime_(runtime), state(NotStarted), duration_(nullptr) {}
     GCParallelTask(GCParallelTask&& other)
       : runtime_(other.runtime_),
+        func_(other.func_),
         state(other.state),
         duration_(nullptr),
         cancel_(false)
     {}
 
     // Derived classes must override this to ensure that join() gets called
     // before members get destructed.
-    virtual ~GCParallelTask();
+    ~GCParallelTask();
 
     JSRuntime* runtime() { return runtime_; }
 
     // Time spent in the most recent invocation of this task.
     mozilla::TimeDuration duration() const { return duration_; }
 
     // The simple interface to a parallel task works exactly like pthreads.
     bool start();
@@ -75,16 +91,38 @@ class GCParallelTask
         if (mode == CancelAndWait)
             join();
     }
 
     // Check if a task is actively running.
     bool isRunningWithLockHeld(const AutoLockHelperThreadState& locked) const;
     bool isRunning() const;
 
+    void runTask() {
+        func_(this);
+    }
+
     // This should be friended to HelperThread, but cannot be because it
     // would introduce several circular dependencies.
   public:
     void runFromHelperThread(AutoLockHelperThreadState& locked);
 };
 
+// CRTP template to handle cast to derived type when calling run().
+template <typename Derived>
+class GCParallelTaskHelper : public GCParallelTask
+{
+  public:
+    explicit GCParallelTaskHelper(JSRuntime* runtime)
+      : GCParallelTask(runtime, &runTaskTyped)
+    {}
+    GCParallelTaskHelper(GCParallelTaskHelper&& other)
+      : GCParallelTask(mozilla::Move(other))
+    {}
+
+  private:
+    static void runTaskTyped(GCParallelTask* task) {
+        static_cast<Derived*>(task)->run();
+    }
+};
+
 } /* namespace js */
 #endif /* gc_GCParallelTask_h */
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -101,42 +101,40 @@ class ChunkPool
         Chunk* operator->() const { return get(); }
       private:
         Chunk* current_;
     };
 };
 
 // Performs extra allocation off thread so that when memory is required on the
 // active thread it will already be available and waiting.
-class BackgroundAllocTask : public GCParallelTask
+class BackgroundAllocTask : public GCParallelTaskHelper<BackgroundAllocTask>
 {
     // Guarded by the GC lock.
     GCLockData<ChunkPool&> chunkPool_;
 
     const bool enabled_;
 
   public:
     BackgroundAllocTask(JSRuntime* rt, ChunkPool& pool);
     bool enabled() const { return enabled_; }
 
-  protected:
-    void run() override;
+    void run();
 };
 
 // Search the provided Chunks for free arenas and decommit them.
-class BackgroundDecommitTask : public GCParallelTask
+class BackgroundDecommitTask : public GCParallelTaskHelper<BackgroundDecommitTask>
 {
   public:
     using ChunkVector = mozilla::Vector<Chunk*>;
 
-    explicit BackgroundDecommitTask(JSRuntime *rt) : GCParallelTask(rt) {}
+    explicit BackgroundDecommitTask(JSRuntime *rt) : GCParallelTaskHelper(rt) {}
     void setChunksToScan(ChunkVector &chunks);
 
-  protected:
-    void run() override;
+    void run();
 
   private:
     ActiveThreadOrGCTaskData<ChunkVector> toDecommit;
 };
 
 template<typename F>
 struct Callback {
     ActiveThreadOrGCTaskData<F> op;
@@ -477,18 +475,20 @@ class GCRuntime
     JSString* tryNewNurseryString(JSContext* cx, size_t thingSize, AllocKind kind);
     static TenuredCell* refillFreeListInGC(Zone* zone, AllocKind thingKind);
 
     void bufferGrayRoots();
 
     /*
      * Concurrent sweep infrastructure.
      */
-    void startTask(GCParallelTask& task, gcstats::PhaseKind phase, AutoLockHelperThreadState& locked);
-    void joinTask(GCParallelTask& task, gcstats::PhaseKind phase, AutoLockHelperThreadState& locked);
+    void startTask(GCParallelTask& task, gcstats::PhaseKind phase,
+                   AutoLockHelperThreadState& locked);
+    void joinTask(GCParallelTask& task, gcstats::PhaseKind phase,
+                  AutoLockHelperThreadState& locked);
 
     void mergeCompartments(JSCompartment* source, JSCompartment* target);
 
   private:
     enum IncrementalResult
     {
         Reset = 0,
         Ok
--- a/js/src/gc/Nursery.cpp
+++ b/js/src/gc/Nursery.cpp
@@ -38,29 +38,31 @@ using namespace gc;
 
 using mozilla::DebugOnly;
 using mozilla::PodCopy;
 using mozilla::TimeDuration;
 using mozilla::TimeStamp;
 
 constexpr uintptr_t CanaryMagicValue = 0xDEADB15D;
 
-struct js::Nursery::FreeMallocedBuffersTask : public GCParallelTask
+struct js::Nursery::FreeMallocedBuffersTask : public GCParallelTaskHelper<FreeMallocedBuffersTask>
 {
-    explicit FreeMallocedBuffersTask(FreeOp* fop) : GCParallelTask(fop->runtime()), fop_(fop) {}
+    explicit FreeMallocedBuffersTask(FreeOp* fop)
+      : GCParallelTaskHelper(fop->runtime()),
+        fop_(fop) {}
     bool init() { return buffers_.init(); }
     void transferBuffersToFree(MallocedBuffersSet& buffersToFree,
                                const AutoLockHelperThreadState& lock);
-    ~FreeMallocedBuffersTask() override { join(); }
+    ~FreeMallocedBuffersTask() { join(); }
+
+    void run();
 
   private:
     FreeOp* fop_;
     MallocedBuffersSet buffers_;
-
-    virtual void run() override;
 };
 
 #ifdef JS_GC_ZEAL
 struct js::Nursery::Canary
 {
     uintptr_t magicValue;
     Canary* next;
 };
--- a/js/src/gc/Statistics.h
+++ b/js/src/gc/Statistics.h
@@ -22,19 +22,16 @@
 #include "js/SliceBudget.h"
 #include "js/UniquePtr.h"
 #include "js/Vector.h"
 #include "vm/JSONPrinter.h"
 
 using mozilla::Maybe;
 
 namespace js {
-
-class GCParallelTask;
-
 namespace gcstats {
 
 // Phase data is generated by a script. If you need to add phases, edit
 // js/src/gc/GenerateStatsPhases.py
 
 #include "gc/StatsPhasesGenerated.h"
 
 enum Stat {
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -1436,31 +1436,31 @@ TimeSince(TimeStamp prev)
 }
 
 void
 js::GCParallelTask::runFromActiveCooperatingThread(JSRuntime* rt)
 {
     MOZ_ASSERT(state == NotStarted);
     MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(rt));
     TimeStamp timeStart = TimeStamp::Now();
-    run();
+    runTask();
     duration_ = TimeSince(timeStart);
 }
 
 void
 js::GCParallelTask::runFromHelperThread(AutoLockHelperThreadState& locked)
 {
     AutoSetContextRuntime ascr(runtime());
     gc::AutoSetThreadIsPerformingGC performingGC;
 
     {
         AutoUnlockHelperThreadState parallelSection(locked);
         TimeStamp timeStart = TimeStamp::Now();
         TlsContext.get()->heapState = JS::HeapState::MajorCollecting;
-        run();
+        runTask();
         TlsContext.get()->heapState = JS::HeapState::Idle;
         duration_ = TimeSince(timeStart);
     }
 
     state = Finished;
     HelperThreadState().notifyAll(GlobalHelperThreadState::CONSUMER, locked);
 }