Attachment 350197 Details for Bug 189558 – Patch

[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:

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)

Flags:

mark.lam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189558: 350197