From c951c985c1738a951a0e851710cf6c355671afd1 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Thu, 10 May 2018 10:09:31 +0100 Subject: Bug 1465108 - Use function pointers rather than virtual run method for GC parallel tasks r=sfink a=abillings a=RyanVM --- js/src/gc/GCRuntime.h | 18 ++++++++-------- js/src/gc/Nursery.cpp | 8 +++---- js/src/gc/Statistics.h | 3 --- js/src/jsgc.cpp | 25 +++++++++++----------- js/src/jsgc.h | 51 ++++++++++++++++++++++++++++++++++++++------- js/src/vm/HelperThreads.cpp | 4 ++-- 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 { // 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 { public: using ChunkVector = mozilla::Vector; @@ -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 { 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 { // 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 +class GCSweepTask : public GCParallelTaskHelper { 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(mozilla::Move(other)), runtime(other.runtime) {} }; // Causes the given WeakCache to be swept when run. -class SweepWeakCacheTask : public GCSweepTask +class SweepWeakCacheTask : public GCSweepTask { JS::WeakCache& 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 { \ 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 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 +class GCParallelTaskHelper : public GCParallelTask +{ + public: + GCParallelTaskHelper() + : GCParallelTask(&runTaskTyped) + {} + GCParallelTaskHelper(GCParallelTaskHelper&& other) + : GCParallelTask(mozilla::Move(other)) + {} + + private: + static void runTaskTyped(GCParallelTask* task) { + static_cast(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; } -- cgit v1.2.3