Bug 1457703 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 10 May 2018 10:09:31 +0100
changeset 417706 ea7f34ae33bb1fea71339150df8bf9dd7126715d
parent 417705 9b7cc103ce95050f733244b0ac10f8870b6cb01f
child 417707 a3ebab26f0d9e962f1f892335838ee1b51335378
push id33977
push userncsoregi@mozilla.com
push dateThu, 10 May 2018 16:43:24 +0000
treeherdermozilla-central@17db33b6a124 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink, abillings
bugs1457703
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1457703 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings
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
@@ -2691,39 +2691,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;
@@ -3973,40 +3972,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));
@@ -4307,33 +4298,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
@@ -5365,39 +5357,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.
@@ -5408,68 +5402,74 @@ 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
-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();
         c->sweepRegExps();
     }
 }
 
 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();
@@ -5479,34 +5479,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.runFromMainThread(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());
 }
 
@@ -6086,39 +6088,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 class State {
         NotStarted,
         Dispatched,
         Finished
     };
     UnprotectedData<State> state_;
@@ -31,35 +41,35 @@ class GCParallelTask
     MainThreadOrGCTaskData<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, mozilla::MemoryOrdering::ReleaseAcquire> cancel_;
 
-    virtual void run() = 0;
-
   public:
-    explicit GCParallelTask(JSRuntime* runtime)
+    explicit GCParallelTask(JSRuntime* runtime, TaskFunc func)
       : runtime_(runtime),
+        func_(func),
         state_(State::NotStarted),
         duration_(nullptr),
         cancel_(false)
     {}
     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();
@@ -108,16 +118,38 @@ class GCParallelTask
         MOZ_ASSERT(state_ == State::Dispatched);
         state_ = State::Finished;
     }
     void setNotStarted(const AutoLockHelperThreadState& lock) {
         MOZ_ASSERT(state_ == State::Finished);
         state_ = State::NotStarted;
     }
 
+    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(GCParallelTask&& 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
 // main 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:
     MainThreadOrGCTaskData<ChunkVector> toDecommit;
 };
 
 template<typename F>
 struct Callback {
     MainThreadOrGCTaskData<F> op;
@@ -487,18 +485,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
@@ -20,19 +20,16 @@
 #include "gc/GCEnum.h"
 #include "js/AllocPolicy.h"
 #include "js/SliceBudget.h"
 #include "js/UniquePtr.h"
 #include "js/Vector.h"
 #include "vm/JSONPrinter.h"
 
 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
@@ -1516,33 +1516,33 @@ TimeSince(TimeStamp prev)
 }
 
 void
 js::GCParallelTask::runFromMainThread(JSRuntime* rt)
 {
     assertNotStarted();
     MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(rt));
     TimeStamp timeStart = TimeStamp::Now();
-    run();
+    runTask();
     duration_ = TimeSince(timeStart);
 }
 
 void
 js::GCParallelTask::runFromHelperThread(AutoLockHelperThreadState& lock)
 {
     MOZ_ASSERT(isDispatched(lock));
 
     AutoSetContextRuntime ascr(runtime());
     gc::AutoSetThreadIsPerformingGC performingGC;
 
     {
         AutoUnlockHelperThreadState parallelSection(lock);
         TimeStamp timeStart = TimeStamp::Now();
         TlsContext.get()->heapState = JS::HeapState::MajorCollecting;
-        run();
+        runTask();
         TlsContext.get()->heapState = JS::HeapState::Idle;
         duration_ = TimeSince(timeStart);
     }
 
     setFinished(lock);
     HelperThreadState().notifyAll(GlobalHelperThreadState::CONSUMER, lock);
 }