author | Luke Wagner <luke@mozilla.com> |
Thu, 23 Aug 2018 11:23:52 -0500 | |
changeset 433143 | e4a245dc212abd00595bb4e4489096c6daefc73c |
parent 433142 | 5ff39f587a0de8a75ab92bc16b58c759cffdb42c |
child 433144 | 49b70f7e6817131d79876ab88667bc6c2833e4a0 |
child 433194 | 62bf59351409430561fb8f8fcef08bdb1a7208cd |
push id | 34499 |
push user | csabou@mozilla.com |
push date | Thu, 23 Aug 2018 21:40:51 +0000 |
treeherder | mozilla-central@49b70f7e6817 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | lth, bbouvier |
bugs | 1480998 |
milestone | 63.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
|
--- a/js/src/wasm/WasmProcess.cpp +++ b/js/src/wasm/WasmProcess.cpp @@ -38,37 +38,38 @@ using mozilla::BinarySearchIf; // wasm compilation may be tiered, and the second tier doesn't have access to // any JSContext/JS::Compartment/etc lying around, we have to use a process-wide // map instead. typedef Vector<const CodeSegment*, 0, SystemAllocPolicy> CodeSegmentVector; Atomic<bool> wasm::CodeExists(false); +// Because of profiling, the thread running wasm might need to know to which +// CodeSegment the current PC belongs, during a call to lookup(). A lookup +// is a read-only operation, and we don't want to take a lock then +// (otherwise, we could have a deadlock situation if an async lookup +// happened on a given thread that was holding mutatorsMutex_ while getting +// sampled). Since the writer could be modifying the data that is getting +// looked up, the writer functions use spin-locks to know if there are any +// observers (i.e. calls to lookup()) of the atomic data. + +static Atomic<size_t> sNumObservers(0); +static Atomic<bool> sShuttingDown(false); + class ProcessCodeSegmentMap { // Since writes (insertions or removals) can happen on any background // thread at the same time, we need a lock here. Mutex mutatorsMutex_; CodeSegmentVector segments1_; CodeSegmentVector segments2_; - // Because of profiling, the thread running wasm might need to know to which - // CodeSegment the current PC belongs, during a call to lookup(). A lookup - // is a read-only operation, and we don't want to take a lock then - // (otherwise, we could have a deadlock situation if an async lookup - // happened on a given thread that was holding mutatorsMutex_ while getting - // sampled). Since the writer could be modifying the data that is getting - // looked up, the writer functions use spin-locks to know if there are any - // observers (i.e. calls to lookup()) of the atomic data. - - Atomic<size_t> observers_; - // Except during swapAndWait(), there are no lookup() observers of the // vector pointed to by mutableCodeSegments_ CodeSegmentVector* mutableCodeSegments_; Atomic<const CodeSegmentVector*> readonlyCodeSegments_; struct CodeSegmentPC { @@ -79,17 +80,17 @@ class ProcessCodeSegmentMap return 0; if (pc < cs->base()) return -1; return 1; } }; void swapAndWait() { - // Both vectors are consistent for look up at this point, although their + // Both vectors are consistent for lookup at this point although their // contents are different: there is no way for the looked up PC to be // in the code segment that is getting registered, because the code // segment is not even fully created yet. // If a lookup happens before this instruction, then the // soon-to-become-former read-only pointer is used during the lookup, // which is valid. @@ -105,23 +106,22 @@ class ProcessCodeSegmentMap // - in case of removal, it means the new vector contains one less // entry, but it's fine since unregistering means the code segment // isn't used by any live instance anymore, thus PC can't be in the // to-be-removed code segment's range. // A lookup could have happened on any of the two vectors. Wait for // observers to be done using any vector before mutating. - while (observers_); + while (sNumObservers > 0) {} } public: ProcessCodeSegmentMap() : mutatorsMutex_(mutexid::WasmCodeSegmentMap), - observers_(0), mutableCodeSegments_(&segments1_), readonlyCodeSegments_(&segments2_) { } ~ProcessCodeSegmentMap() { MOZ_ASSERT(segments1_.empty()); @@ -187,65 +187,79 @@ class ProcessCodeSegmentMap CodeSegmentPC(cs->base()), &otherIndex)); MOZ_ASSERT(index == otherIndex); #endif mutableCodeSegments_->erase(mutableCodeSegments_->begin() + index); } const CodeSegment* lookup(const void* pc) { - auto decObserver = mozilla::MakeScopeExit([&] { - observers_--; - }); - observers_++; - - // Once atomically-read, the readonly vector is valid as long as - // observers_ has been incremented (see swapAndWait()). const CodeSegmentVector* readonly = readonlyCodeSegments_; size_t index; if (!BinarySearchIf(*readonly, 0, readonly->length(), CodeSegmentPC(pc), &index)) return nullptr; // It is fine returning a raw CodeSegment*, because we assume we are // looking up a live PC in code which is on the stack, keeping the // CodeSegment alive. return (*readonly)[index]; } }; -static ProcessCodeSegmentMap processCodeSegmentMap; +static ProcessCodeSegmentMap sProcessCodeSegmentMap; bool wasm::RegisterCodeSegment(const CodeSegment* cs) { MOZ_ASSERT(cs->codeTier().code().initialized()); - return processCodeSegmentMap.insert(cs); + return sProcessCodeSegmentMap.insert(cs); } void wasm::UnregisterCodeSegment(const CodeSegment* cs) { - processCodeSegmentMap.remove(cs); + sProcessCodeSegmentMap.remove(cs); } const CodeSegment* wasm::LookupCodeSegment(const void* pc, const CodeRange** codeRange /*= nullptr */) { - if (const CodeSegment* found = processCodeSegmentMap.lookup(pc)) { + // Avoid accessing an uninitialized sProcessCodeSegmentMap if there is a + // crash early in startup. Returning null will allow the crash to propagate + // properly to breakpad. + if (!CodeExists) + return nullptr; + + // Ensure the observer count is above 0 throughout the entire lookup to + // ensure swapAndWait() waits for the lookup to complete. + auto decObserver = mozilla::MakeScopeExit([&] { + MOZ_ASSERT(sNumObservers > 0); + sNumObservers--; + }); + sNumObservers++; + + // Check sShuttingDown with sNumObservers > 0 to ensure the spinloop in + // wasm::ShutDown() is effective. + if (sShuttingDown) + return nullptr; + + if (const CodeSegment* found = sProcessCodeSegmentMap.lookup(pc)) { if (codeRange) { *codeRange = found->isModule() ? found->asModule()->lookupRange(pc) : found->asLazyStub()->lookupRange(pc); } return found; } + if (codeRange) *codeRange = nullptr; + return nullptr; } const Code* wasm::LookupCode(const void* pc, const CodeRange** codeRange /* = nullptr */) { const CodeSegment* found = LookupCodeSegment(pc, codeRange); MOZ_ASSERT_IF(!found && codeRange, !*codeRange); @@ -256,11 +270,15 @@ void wasm::ShutDown() { // If there are live runtimes then we are already pretty much leaking the // world, so to avoid spurious assertions (which are valid and valuable when // there are not live JSRuntimes), don't bother releasing anything here. if (JSRuntime::hasLiveRuntimes()) return; + // After signalling shutdown, wait for currently-active observers to finish. + sShuttingDown = true; + while (sNumObservers > 0) {} + ReleaseBuiltinThunks(); - processCodeSegmentMap.freeAll(); + sProcessCodeSegmentMap.freeAll(); }