Bug 1465108 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings a=RyanVM

6 files changed, 72 insertions, 37 deletions

diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
index 19737c9ee..5c2576efd 100644
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -73,7 +73,7 @@ class ChunkPool
 
 // Performs extra allocation off the main 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.
     JSRuntime* runtime;
@@ -85,12 +85,11 @@ class BackgroundAllocTask : public GCParallelTask
     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
+// Search the provided Chunks for free arenas and recommit them.
+class BackgroundDecommitTask : public GCParallelTaskHelper<BackgroundDecommitTask>
 {
   public:
     using ChunkVector = mozilla::Vector<Chunk*>;
@@ -98,8 +97,7 @@ class BackgroundDecommitTask : public GCParallelTask
     explicit BackgroundDecommitTask(JSRuntime *rt) : runtime(rt) {}
     void setChunksToScan(ChunkVector &chunks);
 
-  protected:
-    void run() override;
+    void run();
 
   private:
     JSRuntime* runtime;
@@ -1171,8 +1169,10 @@ class GCRuntime
     /*
      * Concurrent sweep infrastructure.
      */
-    void startTask(GCParallelTask& task, gcstats::Phase phase, AutoLockHelperThreadState& locked);
-    void joinTask(GCParallelTask& task, gcstats::Phase phase, AutoLockHelperThreadState& locked);
+    void startTask(GCParallelTask& task, gcstats::Phase phase,
+                   AutoLockHelperThreadState& locked);
+    void joinTask(GCParallelTask& task, gcstats::Phase phase,
+                  AutoLockHelperThreadState& locked);
 
     /*
      * List head of arenas allocated during the sweep phase.
diff --git a/js/src/gc/Nursery.cpp b/js/src/gc/Nursery.cpp
index aa50bf29e..55ca5a059 100644
--- a/js/src/gc/Nursery.cpp
+++ b/js/src/gc/Nursery.cpp
@@ -43,19 +43,19 @@ using mozilla::PodZero;
 
 static const uintptr_t CanaryMagicValue = 0xDEADB15D;
 
-struct js::Nursery::FreeMallocedBuffersTask : public GCParallelTask
+struct js::Nursery::FreeMallocedBuffersTask : public GCParallelTaskHelper<FreeMallocedBuffersTask>
 {
     explicit FreeMallocedBuffersTask(FreeOp* fop) : 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;
 };
 
 struct js::Nursery::SweepAction
diff --git a/js/src/gc/Statistics.h b/js/src/gc/Statistics.h
index c9e5871e3..ca1969b2c 100644
--- a/js/src/gc/Statistics.h
+++ b/js/src/gc/Statistics.h
@@ -22,9 +22,6 @@
 using mozilla::Maybe;
 
 namespace js {
-
-class GCParallelTask;
-
 namespace gcstats {
 
 enum Phase : uint8_t {
diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
index fb10797d5..3d4dae9bb 100644
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -2156,7 +2156,7 @@ ArenasToUpdate::getArenasToUpdate(AutoLockHelperThreadState& lock, unsigned maxL
     return { begin, last->next };
 }
 
-struct UpdatePointersTask : public GCParallelTask
+struct UpdatePointersTask : public GCParallelTaskHelper<UpdatePointersTask>
 {
     // Maximum number of arenas to update in one block.
 #ifdef DEBUG
@@ -2172,14 +2172,13 @@ struct UpdatePointersTask : public GCParallelTask
         arenas_.end = nullptr;
     }
 
-    ~UpdatePointersTask() override { join(); }
+    void run();
 
   private:
     JSRuntime* rt_;
     ArenasToUpdate* source_;
     ArenaListSegment arenas_;
 
-    virtual void run() override;
     bool getArenasToUpdate();
     void updateArenas();
 };
@@ -2985,7 +2984,6 @@ 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) {
@@ -4359,7 +4357,8 @@ GCRuntime::endMarkingZoneGroup()
     marker.setMarkColorBlack();
 }
 
-class GCSweepTask : public GCParallelTask
+template <typename Derived>
+class GCSweepTask : public GCParallelTaskHelper<Derived>
 {
     GCSweepTask(const GCSweepTask&) = delete;
 
@@ -4369,13 +4368,13 @@ class GCSweepTask : public GCParallelTask
   public:
     explicit GCSweepTask(JSRuntime* rt) : runtime(rt) {}
     GCSweepTask(GCSweepTask&& other)
-      : GCParallelTask(mozilla::Move(other)),
+      : GCParallelTaskHelper<Derived>(mozilla::Move(other)),
         runtime(other.runtime)
     {}
 };
 
 // Causes the given WeakCache to be swept when run.
-class SweepWeakCacheTask : public GCSweepTask
+class SweepWeakCacheTask : public GCSweepTask<SweepWeakCacheTask>
 {
     JS::WeakCache<void*>& cache;
 
@@ -4387,15 +4386,15 @@ class SweepWeakCacheTask : public GCSweepTask
       : GCSweepTask(mozilla::Move(other)), cache(other.cache)
     {}
 
-    void run() override {
+    void run() {
         cache.sweep();
     }
 };
 
 #define MAKE_GC_SWEEP_TASK(name)                                              \
-    class name : public GCSweepTask {                                         \
-        void run() override;                                                  \
+    class name : public GCSweepTask<name> {                                   \
       public:                                                                 \
+        void run();                                                           \
         explicit name (JSRuntime* rt) : GCSweepTask(rt) {}                    \
     }
 MAKE_GC_SWEEP_TASK(SweepAtomsTask);
@@ -4447,7 +4446,8 @@ SweepMiscTask::run()
 }
 
 void
-GCRuntime::startTask(GCParallelTask& task, gcstats::Phase phase, AutoLockHelperThreadState& locked)
+GCRuntime::startTask(GCParallelTask& task, gcstats::Phase phase,
+                     AutoLockHelperThreadState& locked)
 {
     if (!task.startWithLockHeld(locked)) {
         AutoUnlockHelperThreadState unlock(locked);
@@ -4457,7 +4457,8 @@ GCRuntime::startTask(GCParallelTask& task, gcstats::Phase phase, AutoLockHelperT
 }
 
 void
-GCRuntime::joinTask(GCParallelTask& task, gcstats::Phase phase, AutoLockHelperThreadState& locked)
+GCRuntime::joinTask(GCParallelTask& task, gcstats::Phase phase,
+                    AutoLockHelperThreadState& locked)
 {
     gcstats::AutoPhase ap(stats, task, phase);
     task.joinWithLockHeld(locked);
diff --git a/js/src/jsgc.h b/js/src/jsgc.h
index 7ad176d84..d3cf31fe7 100644
--- a/js/src/jsgc.h
+++ b/js/src/jsgc.h
@@ -12,6 +12,7 @@
 #include "mozilla/Atomics.h"
 #include "mozilla/EnumeratedArray.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/Move.h"
 #include "mozilla/TypeTraits.h"
 
 #include "js/GCAPI.h"
@@ -936,10 +937,19 @@ class GCHelperState
 };
 
 // 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:
+    TaskFunc func_;
+
     // The state of the parallel computation.
     enum TaskState {
         NotStarted,
@@ -956,19 +966,24 @@ class GCParallelTask
     // A flag to signal a request for early completion of the off-thread task.
     mozilla::Atomic<bool> cancel_;
 
-    virtual void run() = 0;
-
   public:
-    GCParallelTask() : state(NotStarted), duration_(0) {}
+    explicit GCParallelTask(TaskFunc func)
+      : func_(func),
+        state(NotStarted),
+        duration_(0),
+        cancel_(false)
+    {}
+
     GCParallelTask(GCParallelTask&& other)
-      : state(other.state),
+      : func_(other.func_),
+        state(other.state),
         duration_(0),
         cancel_(false)
     {}
 
     // Derived classes must override this to ensure that join() gets called
     // before members get destructed.
-    virtual ~GCParallelTask();
+    ~GCParallelTask();
 
     // Time spent in the most recent invocation of this task.
     int64_t duration() const { return duration_; }
@@ -997,12 +1012,34 @@ class GCParallelTask
     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:
+    GCParallelTaskHelper()
+      : GCParallelTask(&runTaskTyped)
+    {}
+    GCParallelTaskHelper(GCParallelTaskHelper&& other)
+      : GCParallelTask(mozilla::Move(other))
+    {}
+
+  private:
+    static void runTaskTyped(GCParallelTask* task) {
+        static_cast<Derived*>(task)->run();
+    }
+};
+
 typedef void (*IterateChunkCallback)(JSRuntime* rt, void* data, gc::Chunk* chunk);
 typedef void (*IterateZoneCallback)(JSRuntime* rt, void* data, JS::Zone* zone);
 typedef void (*IterateArenaCallback)(JSRuntime* rt, void* data, gc::Arena* arena,
diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp
index 7381a97b5..bd29d0c79 100644
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -1144,7 +1144,7 @@ js::GCParallelTask::runFromMainThread(JSRuntime* rt)
     MOZ_ASSERT(state == NotStarted);
     MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(rt));
     uint64_t timeStart = PRMJ_Now();
-    run();
+    runTask();
     duration_ = PRMJ_Now() - timeStart;
 }
 
@@ -1155,7 +1155,7 @@ js::GCParallelTask::runFromHelperThread(AutoLockHelperThreadState& locked)
         AutoUnlockHelperThreadState parallelSection(locked);
         gc::AutoSetThreadIsPerformingGC performingGC;
         uint64_t timeStart = PRMJ_Now();
-        run();
+        runTask();
         duration_ = PRMJ_Now() - timeStart;
     }