WebKit Bugzilla
Attachment 350197 Details for
Bug 189558
: [JSC] Heap::reportExtraMemoryVisited shows contention if we have many JSString
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-189558-20180920230319.patch (text/plain), 9.86 KB, created by
Yusuke Suzuki
on 2018-09-20 07:03:20 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2018-09-20 07:03:20 PDT
Size:
9.86 KB
patch
obsolete
>Subversion Revision: 236254 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 21f59091c556577f2c8d6292c2e6bdd7e2f07350..809f20eb91eff8bc590354288e60c4032c708819 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,41 @@ >+2018-09-20 Yusuke Suzuki <yusukesuzuki@slowstart.org> >+ >+ [JSC] Heap::reportExtraMemoryVisited shows contention if we have many JSString >+ https://bugs.webkit.org/show_bug.cgi?id=189558 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ When running web-tooling-benchmark postcss test on Linux JSCOnly port, we get the following result in `perf report`. >+ >+ 10.95% AutomaticThread libJavaScriptCore.so.1.0.0 [.] JSC::Heap::reportExtraMemoryVisited >+ >+ This is because postcss produces bunch of JSString, which require reportExtraMemoryVisited calls in JSString::visitChildren. >+ And since reportExtraMemoryVisited attempts to update atomic counter, if we have bunch of marking threads, it becomes super contended. >+ >+ This patch reduces the frequency of updating the atomic counter. Each SlotVisitor has per-SlotVisitor m_extraMemorySize counter. >+ And we propagate this value to the global atomic counter when rebalance happens. >+ >+ We also reduce HeapCell::heap() access by using `vm.heap`. >+ >+ * heap/SlotVisitor.cpp: >+ (JSC::SlotVisitor::didStartMarking): >+ (JSC::SlotVisitor::propagateExternalMemoryVisitedIfNecessary): >+ (JSC::SlotVisitor::drain): >+ (JSC::SlotVisitor::performIncrementOfDraining): >+ (JSC::SlotVisitor::drainInParallelPassively): >+ * heap/SlotVisitor.h: >+ * heap/SlotVisitorInlines.h: >+ (JSC::SlotVisitor::reportExtraMemoryVisited): >+ * runtime/JSString.cpp: >+ (JSC::JSRopeString::resolveRopeToAtomicString const): >+ (JSC::JSRopeString::resolveRope const): >+ * runtime/JSString.h: >+ (JSC::JSString::finishCreation): >+ * wasm/js/JSWebAssemblyInstance.cpp: >+ (JSC::JSWebAssemblyInstance::finishCreation): >+ * wasm/js/JSWebAssemblyMemory.cpp: >+ (JSC::JSWebAssemblyMemory::finishCreation): >+ > 2018-09-20 Fujii Hironori <Hironori.Fujii@sony.com> > > [Win][Clang] JITMathIC.h: error: missing 'template' keyword prior to dependent template name 'retagged' >diff --git a/Source/JavaScriptCore/heap/SlotVisitor.cpp b/Source/JavaScriptCore/heap/SlotVisitor.cpp >index 33c37fc6e89b28b6347869dbe0dac77db1eab357..6cade0c7ba6b07de9ccb009a1caaee8bf529abb5 100644 >--- a/Source/JavaScriptCore/heap/SlotVisitor.cpp >+++ b/Source/JavaScriptCore/heap/SlotVisitor.cpp >@@ -99,8 +99,17 @@ SlotVisitor::~SlotVisitor() > > void SlotVisitor::didStartMarking() > { >- if (heap()->collectionScope() == CollectionScope::Eden) >- reset(); >+ auto scope = heap()->collectionScope(); >+ if (scope) { >+ switch (*scope) { >+ case CollectionScope::Eden: >+ reset(); >+ break; >+ case CollectionScope::Full: >+ m_extraMemorySize = 0; >+ break; >+ } >+ } > > if (HeapProfiler* heapProfiler = vm().heapProfiler()) > m_heapSnapshotBuilder = heapProfiler->activeSnapshotBuilder(); >@@ -397,6 +406,14 @@ void SlotVisitor::visitAsConstraint(const JSCell* cell) > visitChildren(cell); > } > >+inline void SlotVisitor::propagateExternalMemoryVisitedIfNecessary() >+{ >+ if (m_isFirstVisit && m_extraMemorySize) { >+ heap()->reportExtraMemoryVisited(m_extraMemorySize); >+ m_extraMemorySize = 0; >+ } >+} >+ > void SlotVisitor::donateKnownParallel(MarkStackArray& from, MarkStackArray& to) > { > // NOTE: Because we re-try often, we can afford to be conservative, and >@@ -483,6 +500,7 @@ NEVER_INLINE void SlotVisitor::drain(MonotonicTime timeout) > visitChildren(stack.removeLast()); > return IterationStatus::Done; > }); >+ propagateExternalMemoryVisitedIfNecessary(); > if (status == IterationStatus::Continue) > break; > >@@ -539,6 +557,7 @@ size_t SlotVisitor::performIncrementOfDraining(size_t bytesRequested) > } > return IterationStatus::Done; > }); >+ propagateExternalMemoryVisitedIfNecessary(); > if (status == IterationStatus::Continue) > break; > m_rightToRun.safepoint(); >@@ -696,7 +715,7 @@ SlotVisitor::SharedDrainResult SlotVisitor::drainInParallelPassively(MonotonicTi > return drainInParallel(timeout); > } > >- donateAll(holdLock(m_heap.m_markingMutex)); >+ donateAll(); > return waitForTermination(timeout); > } > >diff --git a/Source/JavaScriptCore/heap/SlotVisitor.h b/Source/JavaScriptCore/heap/SlotVisitor.h >index ee590efa057957a5998be1dc69aa3d051b73137c..6df7a652cddf70bd45417fbd20bb9445fc484f26 100644 >--- a/Source/JavaScriptCore/heap/SlotVisitor.h >+++ b/Source/JavaScriptCore/heap/SlotVisitor.h >@@ -216,6 +216,8 @@ class SlotVisitor { > void noteLiveAuxiliaryCell(HeapCell*); > > void visitChildren(const JSCell*); >+ >+ void propagateExternalMemoryVisitedIfNecessary(); > > void donateKnownParallel(); > void donateKnownParallel(MarkStackArray& from, MarkStackArray& to); >@@ -237,6 +239,7 @@ class SlotVisitor { > size_t m_bytesVisited; > size_t m_visitCount; > size_t m_nonCellVisitCount { 0 }; // Used for incremental draining, ignored otherwise. >+ size_t m_extraMemorySize { 0 }; > bool m_isInParallelMode; > > HeapVersion m_markingVersion; >diff --git a/Source/JavaScriptCore/heap/SlotVisitorInlines.h b/Source/JavaScriptCore/heap/SlotVisitorInlines.h >index f36d17b47282fad492d82216427383a73060fe38..84a558e5122a42528f9e9613c7dfdd1ea98e74d6 100644 >--- a/Source/JavaScriptCore/heap/SlotVisitorInlines.h >+++ b/Source/JavaScriptCore/heap/SlotVisitorInlines.h >@@ -154,8 +154,12 @@ inline bool SlotVisitor::containsOpaqueRoot(void* ptr) const > inline void SlotVisitor::reportExtraMemoryVisited(size_t size) > { > if (m_isFirstVisit) { >- heap()->reportExtraMemoryVisited(size); > m_nonCellVisitCount += size; >+ // FIXME: Change this to use SaturatedArithmetic when available. >+ // https://bugs.webkit.org/show_bug.cgi?id=170411 >+ Checked<size_t, RecordOverflow> checkedNewSize = m_extraMemorySize; >+ checkedNewSize += size; >+ m_extraMemorySize = UNLIKELY(checkedNewSize.hasOverflowed()) ? std::numeric_limits<size_t>::max() : checkedNewSize.unsafeGet(); > } > } > >diff --git a/Source/JavaScriptCore/runtime/JSString.cpp b/Source/JavaScriptCore/runtime/JSString.cpp >index d9c4234f10069ea23967aa41693645f86f2cc973..d31ca6b6a4cac77516030d8f4ce8a2802bcf84f4 100644 >--- a/Source/JavaScriptCore/runtime/JSString.cpp >+++ b/Source/JavaScriptCore/runtime/JSString.cpp >@@ -205,7 +205,7 @@ void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const > > // If we resolved a string that didn't previously exist, notify the heap that we've grown. > if (m_value.impl()->hasOneRef()) >- Heap::heap(this)->reportExtraMemoryAllocated(m_value.impl()->cost()); >+ vm.heap.reportExtraMemoryAllocated(m_value.impl()->cost()); > } > > void JSRopeString::clearFibers() const >@@ -264,7 +264,7 @@ void JSRopeString::resolveRope(ExecState* exec) const > if (is8Bit()) { > LChar* buffer; > if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) { >- Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost()); >+ exec->vm().heap.reportExtraMemoryAllocated(newImpl->cost()); > m_value = WTFMove(newImpl); > } else { > outOfMemory(exec); >@@ -278,7 +278,7 @@ void JSRopeString::resolveRope(ExecState* exec) const > > UChar* buffer; > if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) { >- Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost()); >+ exec->vm().heap.reportExtraMemoryAllocated(newImpl->cost()); > m_value = WTFMove(newImpl); > } else { > outOfMemory(exec); >diff --git a/Source/JavaScriptCore/runtime/JSString.h b/Source/JavaScriptCore/runtime/JSString.h >index 7c769094729370c0652a7be08656633f96d6016d..290c8c7478d45800bbb5ebedd55fb1282298389a 100644 >--- a/Source/JavaScriptCore/runtime/JSString.h >+++ b/Source/JavaScriptCore/runtime/JSString.h >@@ -123,7 +123,7 @@ class JSString : public JSCell { > Base::finishCreation(vm); > setLength(length); > setIs8Bit(m_value.impl()->is8Bit()); >- Heap::heap(this)->reportExtraMemoryAllocated(cost); >+ vm.heap.reportExtraMemoryAllocated(cost); > } > > protected: >diff --git a/Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp b/Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp >index e0b8e0c4eeaf5dba18790dd112a0b6989e5760c6..abc7fae48ef78bae7ef34cbe4f356abf84ea03a3 100644 >--- a/Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp >+++ b/Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp >@@ -66,7 +66,7 @@ void JSWebAssemblyInstance::finishCreation(VM& vm, JSWebAssemblyModule* module, > m_moduleNamespaceObject.set(vm, this, moduleNamespaceObject); > m_callee.set(vm, this, module->callee()); > >- heap()->reportExtraMemoryAllocated(m_instance->extraMemoryAllocated()); >+ vm.heap.reportExtraMemoryAllocated(m_instance->extraMemoryAllocated()); > } > > void JSWebAssemblyInstance::destroy(JSCell* cell) >diff --git a/Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp b/Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp >index c90ac93a247a3fc4476d57e8a6bee617cf59b33a..56562526cfe7178b2738e9265d7b285a3e47646f 100644 >--- a/Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp >+++ b/Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp >@@ -133,7 +133,7 @@ void JSWebAssemblyMemory::finishCreation(VM& vm) > { > Base::finishCreation(vm); > ASSERT(inherits(vm, info())); >- heap()->reportExtraMemoryAllocated(memory().size()); >+ vm.heap.reportExtraMemoryAllocated(memory().size()); > } > > void JSWebAssemblyMemory::destroy(JSCell* cell)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
mark.lam
:
review+
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 189558
: 350197