From d0802e7ec174a956d94748861ccbd5bb1fdb0e76 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Mon, 1 Aug 2016 18:39:44 -0700 Subject: [PATCH] Bug 1290287 - Make js::HelperThread::thread a js::Thread instead of a PRThread; r=terrence This ended up boiling more of the ocean than I'd hoped for, but the resulting code is much nicer. --- js/src/asmjs/WasmSignalHandlers.cpp | 1 + js/src/threading/Thread.h | 24 +++++-- js/src/vm/HelperThreads.cpp | 97 +++++++++++++++++------------ js/src/vm/HelperThreads.h | 5 +- js/src/vm/PosixNSPR.cpp | 2 +- 5 files changed, 80 insertions(+), 49 deletions(-) diff --git a/js/src/asmjs/WasmSignalHandlers.cpp b/js/src/asmjs/WasmSignalHandlers.cpp index 8e65260821d9..3ed0f9fd670a 100644 --- a/js/src/asmjs/WasmSignalHandlers.cpp +++ b/js/src/asmjs/WasmSignalHandlers.cpp @@ -1030,6 +1030,7 @@ MachExceptionHandlerThread(JSRuntime* rt) MachExceptionHandler::MachExceptionHandler() : installed_(false), + thread_(), port_(MACH_PORT_NULL) {} diff --git a/js/src/threading/Thread.h b/js/src/threading/Thread.h index 5cfb996f4220..09e5a4a7865c 100644 --- a/js/src/threading/Thread.h +++ b/js/src/threading/Thread.h @@ -15,6 +15,8 @@ #include +#include "js/Utility.h" + #ifdef XP_WIN # define THREAD_RETURN_TYPE unsigned int # define THREAD_CALL_API __stdcall @@ -69,7 +71,17 @@ public: // Create a Thread in an initially unjoinable state. A thread of execution can // be created for this Thread by calling |init|. Some of the thread's // properties may be controlled by passing options to this constructor. - explicit Thread(const Options& options = Options()) : id_(Id()), options_(options) {} + template ::Type, + typename DerefO = typename mozilla::RemoveReference::Type, + typename = typename mozilla::EnableIf::value, + void*>::Type> + explicit Thread(O&& options = Options()) + : id_(Id()) + , options_(mozilla::Forward(options)) + { } // Start a thread of execution at functor |f| with parameters |args|. Note // that the arguments must be either POD or rvalue references (mozilla::Move). @@ -89,9 +101,11 @@ public: MOZ_MUST_USE bool init(F&& f, Args&&... args) { MOZ_RELEASE_ASSERT(!joinable()); using Trampoline = detail::ThreadTrampoline; - auto trampoline = new Trampoline(mozilla::Forward(f), - mozilla::Forward(args)...); - MOZ_RELEASE_ASSERT(trampoline); + AutoEnterOOMUnsafeRegion oom; + auto trampoline = js_new(mozilla::Forward(f), + mozilla::Forward(args)...); + if (!trampoline) + oom.crash("js::Thread::init"); return create(Trampoline::Start, trampoline); } @@ -197,7 +211,7 @@ public: static THREAD_RETURN_TYPE THREAD_CALL_API Start(void* aPack) { auto* pack = static_cast*>(aPack); pack->callMain(typename mozilla::IndexSequenceFor::Type()); - delete pack; + js_delete(pack); return 0; } diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp index 56344ca6a973..7dd0ed203d97 100644 --- a/js/src/vm/HelperThreads.cpp +++ b/js/src/vm/HelperThreads.cpp @@ -153,8 +153,7 @@ js::CancelOffThreadIonCompile(JSCompartment* compartment, JSScript* script, bool } /* Wait for in progress entries to finish up. */ - for (size_t i = 0; i < HelperThreadState().threadCount; i++) { - HelperThread& helper = HelperThreadState().threads[i]; + for (auto& helper : *HelperThreadState().threads) { while (helper.ionBuilder() && CompiledScriptMatches(compartment, script, helper.ionBuilder()->script())) { @@ -327,8 +326,8 @@ js::CancelOffThreadParses(JSRuntime* rt) } if (!pending) { bool inProgress = false; - for (size_t i = 0; i < HelperThreadState().threadCount; i++) { - ParseTask* task = HelperThreadState().threads[i].parseTask(); + for (auto& thread : *HelperThreadState().threads) { + ParseTask* task = thread.parseTask(); if (task && task->runtimeMatches(rt)) inProgress = true; } @@ -615,20 +614,30 @@ GlobalHelperThreadState::ensureInitialized() if (threads) return true; - threads = js_pod_calloc(threadCount); - if (!threads) + threads = js::UniquePtr(js_new()); + if (!threads || !threads->initCapacity(threadCount)) return false; for (size_t i = 0; i < threadCount; i++) { - HelperThread& helper = threads[i]; + threads->infallibleEmplaceBack(); + HelperThread& helper = (*threads)[i]; + helper.threadData.emplace(static_cast(nullptr)); - helper.thread = PR_CreateThread(PR_USER_THREAD, - HelperThread::ThreadMain, &helper, - PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD, PR_JOINABLE_THREAD, HELPER_STACK_SIZE); - if (!helper.thread || !helper.threadData->init()) { - finishThreads(); - return false; - } + if (!helper.threadData->init()) + goto error; + + helper.thread = mozilla::Some(Thread(Thread::Options().setStackSize(HELPER_STACK_SIZE))); + if (!helper.thread->init(HelperThread::ThreadMain, &helper)) + goto error; + + continue; + + error: + // Ensure that we do not leave uninitialized threads in the `threads` + // vector. + threads->popBack(); + finishThreads(); + return false; } return true; @@ -660,10 +669,9 @@ GlobalHelperThreadState::finishThreads() return; MOZ_ASSERT(CanUseExtraThreads()); - for (size_t i = 0; i < threadCount; i++) - threads[i].destroy(); - js_free(threads); - threads = nullptr; + for (auto& thread : *threads) + thread.destroy(); + threads.reset(nullptr); } void @@ -704,8 +712,8 @@ GlobalHelperThreadState::hasActiveThreads(const AutoLockHelperThreadState&) if (!threads) return false; - for (size_t i = 0; i < threadCount; i++) { - if (!threads[i].idle()) + for (auto& thread : *threads) { + if (!thread.idle()) return true; } @@ -730,8 +738,8 @@ GlobalHelperThreadState::checkTaskThreadLimit(size_t maxThreads) const return true; size_t count = 0; - for (size_t i = 0; i < threadCount; i++) { - if (threads[i].currentTask.isSome() && threads[i].currentTask->is()) + for (auto& thread : *threads) { + if (thread.currentTask.isSome() && thread.currentTask->is()) count++; if (count >= maxThreads) return false; @@ -883,11 +891,14 @@ GlobalHelperThreadState::lowestPriorityUnpausedIonCompileAtThreshold( // such builders permitted. size_t numBuilderThreads = 0; HelperThread* thread = nullptr; - for (size_t i = 0; i < threadCount; i++) { - if (threads[i].ionBuilder() && !threads[i].pause) { + for (auto& thisThread : *threads) { + if (thisThread.ionBuilder() && !thisThread.pause) { numBuilderThreads++; - if (!thread || IonBuilderHasHigherPriority(thread->ionBuilder(), threads[i].ionBuilder())) - thread = &threads[i]; + if (!thread || + IonBuilderHasHigherPriority(thread->ionBuilder(), thisThread.ionBuilder())) + { + thread = &thisThread; + } } } if (numBuilderThreads < maxUnpausedIonCompilationThreads()) @@ -901,12 +912,15 @@ GlobalHelperThreadState::highestPriorityPausedIonCompile(const AutoLockHelperThr // Get the highest priority IonBuilder which has started compilation but // which was subsequently paused. HelperThread* thread = nullptr; - for (size_t i = 0; i < threadCount; i++) { - if (threads[i].pause) { + for (auto& thisThread : *threads) { + if (thisThread.pause) { // Currently, only threads with IonBuilders can be paused. - MOZ_ASSERT(threads[i].ionBuilder()); - if (!thread || IonBuilderHasHigherPriority(threads[i].ionBuilder(), thread->ionBuilder())) - thread = &threads[i]; + MOZ_ASSERT(thisThread.ionBuilder()); + if (!thread || + IonBuilderHasHigherPriority(thisThread.ionBuilder(), thread->ionBuilder())) + { + thread = &thisThread; + } } } return thread; @@ -1266,7 +1280,7 @@ GlobalHelperThreadState::mergeParseTaskCompartment(JSContext* cx, ParseTask* par void HelperThread::destroy() { - if (thread) { + if (thread.isSome()) { { AutoLockHelperThreadState lock; terminate = true; @@ -1275,7 +1289,8 @@ HelperThread::destroy() HelperThreadState().notifyAll(GlobalHelperThreadState::PRODUCER, lock); } - PR_JoinThread(thread); + thread->join(); + thread.reset(); } threadData.reset(); @@ -1420,11 +1435,11 @@ HelperThread::handleIonWorkload(AutoLockHelperThreadState& locked) static HelperThread* CurrentHelperThread() { - PRThread* prThread = PR_GetCurrentThread(); + auto threadId = ThisThread::GetId(); HelperThread* thread = nullptr; - for (size_t i = 0; i < HelperThreadState().threadCount; i++) { - if (prThread == HelperThreadState().threads[i].thread) { - thread = &HelperThreadState().threads[i]; + for (auto& thisThread : *HelperThreadState().threads) { + if (thisThread.thread.isSome() && threadId == thisThread.thread->get_id()) { + thread = &thisThread; break; } } @@ -1562,8 +1577,8 @@ GlobalHelperThreadState::compressionInProgress(SourceCompressionTask* task, if (compressionWorklist(lock)[i] == task) return true; } - for (size_t i = 0; i < threadCount; i++) { - if (threads[i].compressionTask() == task) + for (auto& thread : *threads) { + if (thread.compressionTask() == task) return true; } return false; @@ -1604,8 +1619,8 @@ GlobalHelperThreadState::compressionTaskForSource(ScriptSource* ss, if (task->source() == ss) return task; } - for (size_t i = 0; i < threadCount; i++) { - SourceCompressionTask* task = threads[i].compressionTask(); + for (auto& thread : *threads) { + SourceCompressionTask* task = thread.compressionTask(); if (task && task->source() == ss) return task; } diff --git a/js/src/vm/HelperThreads.h b/js/src/vm/HelperThreads.h index 0251647d2e2c..1f565abddfe7 100644 --- a/js/src/vm/HelperThreads.h +++ b/js/src/vm/HelperThreads.h @@ -69,7 +69,8 @@ class GlobalHelperThreadState typedef Vector GCParallelTaskVector; // List of available threads, or null if the thread state has not been initialized. - HelperThread* threads; + using HelperThreadVector = Vector; + UniquePtr threads; private: // The lists below are all protected by |lock|. @@ -274,7 +275,7 @@ HelperThreadState() struct HelperThread { mozilla::Maybe threadData; - PRThread* thread; + mozilla::Maybe thread; /* * Indicate to a thread that it should terminate itself. This is only read diff --git a/js/src/vm/PosixNSPR.cpp b/js/src/vm/PosixNSPR.cpp index 7141a1559567..5a227726d144 100644 --- a/js/src/vm/PosixNSPR.cpp +++ b/js/src/vm/PosixNSPR.cpp @@ -145,7 +145,7 @@ PR_GetCurrentThread() if (!thread) { thread = js_new(nullptr, nullptr, false); if (!thread) - MOZ_CRASH(); + return nullptr; pthread_setspecific(gSelfThreadIndex, thread); } return thread;