From dd357608a238b773bd0fae71a7628e77cd58fd9a Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Wed, 6 Sep 2017 09:07:09 +0100 Subject: [PATCH] Bug 1395366 - Extend zone group's state to cover those intended for future use by helper threads and disallow GC of such groups r=sfink --- js/src/gc/Verifier.cpp | 2 +- js/src/gc/Zone.cpp | 6 ++- js/src/gc/Zone.h | 6 +-- js/src/gc/ZoneGroup.cpp | 5 +- js/src/gc/ZoneGroup.h | 33 ++++++++++++- js/src/threading/ProtectedData.cpp | 2 +- js/src/vm/HelperThreads.cpp | 74 ++++++++++++++++++++++-------- js/src/vm/Runtime.cpp | 14 +++--- js/src/vm/Runtime.h | 4 +- 9 files changed, 108 insertions(+), 38 deletions(-) diff --git a/js/src/gc/Verifier.cpp b/js/src/gc/Verifier.cpp index a652cedda7ca..7f5bc747fe8d 100644 --- a/js/src/gc/Verifier.cpp +++ b/js/src/gc/Verifier.cpp @@ -525,7 +525,7 @@ HeapCheckTracerBase::onChild(const JS::GCCellPtr& thing) // Don't trace into GC in zones being used by helper threads. Zone* zone = thing.is() ? thing.as().zone() : cell->asTenured().zone(); - if (zone->group() && zone->group()->usedByHelperThread) + if (zone->group() && zone->group()->usedByHelperThread()) return; WorkItem item(thing, contextName(), parentIndex); diff --git a/js/src/gc/Zone.cpp b/js/src/gc/Zone.cpp index 725677cf73c5..e5c948e24ab7 100644 --- a/js/src/gc/Zone.cpp +++ b/js/src/gc/Zone.cpp @@ -298,9 +298,11 @@ Zone::hasMarkedCompartments() bool Zone::canCollect() { - // Zones cannot be collected while in use by other threads. - if (usedByHelperThread()) + // Zones that will be or are currently used by other threads cannot be + // collected. + if (!isAtomsZone() && group()->createdForHelperThread()) return false; + JSRuntime* rt = runtimeFromAnyThread(); if (isAtomsZone() && rt->hasHelperThreadZones()) return false; diff --git a/js/src/gc/Zone.h b/js/src/gc/Zone.h index 737573265423..7a1f0aa9caf4 100644 --- a/js/src/gc/Zone.h +++ b/js/src/gc/Zone.h @@ -533,7 +533,7 @@ struct Zone : public JS::shadow::Zone, js::ZoneGroupData isSystem; bool usedByHelperThread() { - return !isAtomsZone() && group()->usedByHelperThread; + return !isAtomsZone() && group()->usedByHelperThread(); } #ifdef DEBUG @@ -693,7 +693,7 @@ class ZoneGroupsIter it = rt->gc.groups().begin(); end = rt->gc.groups().end(); - if (!done() && (*it)->usedByHelperThread) + if (!done() && (*it)->createdForHelperThread()) next(); } @@ -703,7 +703,7 @@ class ZoneGroupsIter MOZ_ASSERT(!done()); do { it++; - } while (!done() && (*it)->usedByHelperThread); + } while (!done() && (*it)->createdForHelperThread()); } ZoneGroup* get() const { diff --git a/js/src/gc/ZoneGroup.cpp b/js/src/gc/ZoneGroup.cpp index 9cfd2128cd79..b45c38e1024e 100644 --- a/js/src/gc/ZoneGroup.cpp +++ b/js/src/gc/ZoneGroup.cpp @@ -20,7 +20,7 @@ ZoneGroup::ZoneGroup(JSRuntime* runtime) ownerContext_(TlsContext.get()), enterCount(1), zones_(this), - usedByHelperThread(false), + helperThreadUse(HelperThreadUse::None), #ifdef DEBUG ionBailAfter_(this, 0), #endif @@ -45,6 +45,7 @@ ZoneGroup::init() ZoneGroup::~ZoneGroup() { #ifdef DEBUG + MOZ_ASSERT(helperThreadUse == HelperThreadUse::None); { AutoLockHelperThreadState lock; MOZ_ASSERT(ionLazyLinkListSize_ == 0); @@ -65,7 +66,7 @@ ZoneGroup::enter(JSContext* cx) MOZ_ASSERT(enterCount); } else { if (useExclusiveLocking) { - MOZ_ASSERT(!usedByHelperThread); + MOZ_ASSERT(!usedByHelperThread()); while (ownerContext().context() != nullptr) { cx->yieldToEmbedding(); } diff --git a/js/src/gc/ZoneGroup.h b/js/src/gc/ZoneGroup.h index 7901793d7973..98a8d0e4bb4c 100644 --- a/js/src/gc/ZoneGroup.h +++ b/js/src/gc/ZoneGroup.h @@ -59,8 +59,37 @@ class ZoneGroup public: ZoneVector& zones() { return zones_.ref(); } - // Whether a zone in this group is in use by a helper thread. - mozilla::Atomic usedByHelperThread; + private: + enum class HelperThreadUse : uint32_t + { + None, + Pending, + Active + }; + + mozilla::Atomic helperThreadUse; + + public: + // Whether a zone in this group was created for use by a helper thread. + bool createdForHelperThread() const { + return helperThreadUse != HelperThreadUse::None; + } + // Whether a zone in this group is currently in use by a helper thread. + bool usedByHelperThread() const { + return helperThreadUse == HelperThreadUse::Active; + } + void setCreatedForHelperThread() { + MOZ_ASSERT(helperThreadUse == HelperThreadUse::None); + helperThreadUse = HelperThreadUse::Pending; + } + void setUsedByHelperThread() { + MOZ_ASSERT(helperThreadUse == HelperThreadUse::Pending); + helperThreadUse = HelperThreadUse::Active; + } + void clearUsedByHelperThread() { + MOZ_ASSERT(helperThreadUse != HelperThreadUse::None); + helperThreadUse = HelperThreadUse::None; + } explicit ZoneGroup(JSRuntime* runtime); ~ZoneGroup(); diff --git a/js/src/threading/ProtectedData.cpp b/js/src/threading/ProtectedData.cpp index 3b0bdaeae83d..157fbe3f9121 100644 --- a/js/src/threading/ProtectedData.cpp +++ b/js/src/threading/ProtectedData.cpp @@ -79,7 +79,7 @@ CheckZoneGroup::check() const JSContext* cx = TlsContext.get(); if (group) { - if (group->usedByHelperThread) { + if (group->usedByHelperThread()) { MOZ_ASSERT(group->ownedByCurrentThread()); } else { // This check is disabled on windows for the same reason as in diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp index 36ffc4ce3f20..23244f846c01 100644 --- a/js/src/vm/HelperThreads.cpp +++ b/js/src/vm/HelperThreads.cpp @@ -36,6 +36,7 @@ using namespace js; using mozilla::ArrayLength; using mozilla::DebugOnly; +using mozilla::Maybe; using mozilla::Unused; using mozilla::TimeDuration; using mozilla::TimeStamp; @@ -646,6 +647,14 @@ js::CancelOffThreadParses(JSRuntime* rt) if (!found) break; } + +#ifdef DEBUG + GlobalHelperThreadState::ParseTaskVector& worklist = HelperThreadState().parseWorklist(lock); + for (size_t i = 0; i < worklist.length(); i++) { + ParseTask* task = worklist[i]; + MOZ_ASSERT(!task->runtimeMatches(rt)); + } +#endif } bool @@ -696,8 +705,29 @@ EnsureParserCreatedClasses(JSContext* cx, ParseTaskKind kind) return true; } +class AutoClearUsedByHelperThread +{ + ZoneGroup* group; + + public: + AutoClearUsedByHelperThread(JSObject* global) + : group(global->zone()->group()) + {} + + void forget() { + group = nullptr; + } + + ~AutoClearUsedByHelperThread() { + if (group) + group->clearUsedByHelperThread(); + } +}; + static JSObject* -CreateGlobalForOffThreadParse(JSContext* cx, ParseTaskKind kind, const gc::AutoSuppressGC& nogc) +CreateGlobalForOffThreadParse(JSContext* cx, ParseTaskKind kind, + Maybe& clearUseGuard, + const gc::AutoSuppressGC& nogc) { JSCompartment* currentCompartment = cx->compartment(); @@ -720,11 +750,18 @@ CreateGlobalForOffThreadParse(JSContext* cx, ParseTaskKind kind, const gc::AutoS JS_SetCompartmentPrincipals(global->compartment(), currentCompartment->principals()); + // Mark this zone group as created for a helper thread. This prevents it + // from being collected until clearUsedByHelperThread() is called. + ZoneGroup* group = global->zone()->group(); + group->setCreatedForHelperThread(); + clearUseGuard.emplace(global); + // Initialize all classes required for parsing while still on the active // thread, for both the target and the new global so that prototype // pointers can be changed infallibly after parsing finishes. if (!EnsureParserCreatedClasses(cx, kind)) return nullptr; + { AutoCompartment ac(cx, global); if (!EnsureParserCreatedClasses(cx, kind)) @@ -737,19 +774,18 @@ CreateGlobalForOffThreadParse(JSContext* cx, ParseTaskKind kind, const gc::AutoS static bool QueueOffThreadParseTask(JSContext* cx, ParseTask* task) { - if (OffThreadParsingMustWaitForGC(cx->runtime())) { - AutoLockHelperThreadState lock; - if (!HelperThreadState().parseWaitingOnGC(lock).append(task)) { - ReportOutOfMemory(cx); - return false; - } - } else { - AutoLockHelperThreadState lock; - if (!HelperThreadState().parseWorklist(lock).append(task)) { - ReportOutOfMemory(cx); - return false; - } + AutoLockHelperThreadState lock; + bool mustWait = OffThreadParsingMustWaitForGC(cx->runtime()); + + auto& queue = mustWait ? HelperThreadState().parseWaitingOnGC(lock) + : HelperThreadState().parseWorklist(lock); + if (!queue.append(task)) { + ReportOutOfMemory(cx); + return false; + } + + if (!mustWait) { task->activate(cx->runtime()); HelperThreadState().notifyOne(GlobalHelperThreadState::PRODUCER, lock); } @@ -768,7 +804,8 @@ StartOffThreadParseTask(JSContext* cx, const ReadOnlyCompileOptions& options, gc::AutoAssertNoNurseryAlloc noNurseryAlloc; AutoSuppressAllocationMetadataBuilder suppressMetadata(cx); - JSObject* global = CreateGlobalForOffThreadParse(cx, kind, nogc); + Maybe clearUseGuard; + JSObject* global = CreateGlobalForOffThreadParse(cx, kind, clearUseGuard, nogc); if (!global) return false; @@ -780,6 +817,7 @@ StartOffThreadParseTask(JSContext* cx, const ReadOnlyCompileOptions& options, return false; task.forget(); + clearUseGuard->forget(); return true; } @@ -844,7 +882,7 @@ js::EnqueuePendingParseTasksAfterGC(JSRuntime* rt) for (size_t i = 0; i < waiting.length(); i++) { ParseTask* task = waiting[i]; - if (task->runtimeMatches(rt) && !task->parseGlobal->zone()->wasGCStarted()) { + if (task->runtimeMatches(rt)) { AutoEnterOOMUnsafeRegion oomUnsafe; if (!newTasks.append(task)) oomUnsafe.crash("EnqueuePendingParseTasksAfterGC"); @@ -856,8 +894,8 @@ js::EnqueuePendingParseTasksAfterGC(JSRuntime* rt) if (newTasks.empty()) return; - // This logic should mirror the contents of the !activeGCInAtomsZone() - // branch in StartOffThreadParseScript: + // This logic should mirror the contents of the + // !OffThreadParsingMustWaitForGC() branch in QueueOffThreadParseTask: for (size_t i = 0; i < newTasks.length(); i++) newTasks[i]->activate(rt); @@ -1429,7 +1467,7 @@ js::GCParallelTask::~GCParallelTask() // base class can't ensure that the task is done using the members. All we // can do now is check that someone has previously stopped the task. #ifdef DEBUG - mozilla::Maybe helperLock; + Maybe helperLock; if (!HelperThreadState().isLockedByCurrentThread()) helperLock.emplace(); MOZ_ASSERT(state == NotStarted); diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index f94bc8d9e941..e4b28ddf13cd 100644 --- a/js/src/vm/Runtime.cpp +++ b/js/src/vm/Runtime.cpp @@ -136,7 +136,7 @@ JSRuntime::JSRuntime(JSRuntime* parentRuntime) #ifdef DEBUG activeThreadHasExclusiveAccess(false), #endif - numHelperThreadZones(0), + numActiveHelperThreadZones(0), numCompartments(0), localeCallbacks(nullptr), defaultLocale(nullptr), @@ -892,18 +892,18 @@ JSRuntime::destroyAtomsAddedWhileSweepingTable() void JSRuntime::setUsedByHelperThread(Zone* zone) { - MOZ_ASSERT(!zone->group()->usedByHelperThread); + MOZ_ASSERT(!zone->group()->usedByHelperThread()); MOZ_ASSERT(!zone->wasGCStarted()); - zone->group()->usedByHelperThread = true; - numHelperThreadZones++; + zone->group()->setUsedByHelperThread(); + numActiveHelperThreadZones++; } void JSRuntime::clearUsedByHelperThread(Zone* zone) { - MOZ_ASSERT(zone->group()->usedByHelperThread); - zone->group()->usedByHelperThread = false; - numHelperThreadZones--; + MOZ_ASSERT(zone->group()->usedByHelperThread()); + zone->group()->clearUsedByHelperThread(); + numActiveHelperThreadZones--; JSContext* cx = TlsContext.get(); if (gc.fullGCForAtomsRequested() && cx->canCollectAtoms()) gc.triggerFullGCForAtoms(cx); diff --git a/js/src/vm/Runtime.h b/js/src/vm/Runtime.h index fc0ecbced083..fb629b0c2307 100644 --- a/js/src/vm/Runtime.h +++ b/js/src/vm/Runtime.h @@ -624,7 +624,7 @@ struct JSRuntime : public js::MallocProvider #endif /* Number of zones which may be operated on by non-cooperating helper threads. */ - js::UnprotectedData numHelperThreadZones; + js::UnprotectedData numActiveHelperThreadZones; friend class js::AutoLockForExclusiveAccess; @@ -633,7 +633,7 @@ struct JSRuntime : public js::MallocProvider void clearUsedByHelperThread(JS::Zone* zone); bool hasHelperThreadZones() const { - return numHelperThreadZones > 0; + return numActiveHelperThreadZones > 0; } #ifdef DEBUG