Attachment 368426 Details for Bug 186740 – Patch

[patch] Patch

bug-186740-20190428021941.patch (text/plain), 11.31 KB, created by Yusuke Suzuki on 2019-04-28 02:19:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-04-28 02:19:41 PDT

Size: 11.31 KB

patch

obsolete

>Subversion Revision: 244723
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 28aeca460ca65580a9ae60e98de7e15fada6d41b..1647cab53dfffcfdaf9d774dd1709653e849d01c 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-04-28  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        CodeBlockSet wastes 190KB of HashTable capacity on nytimes.com
>+        https://bugs.webkit.org/show_bug.cgi?id=186740
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::visitChildren):
>+        (JSC::CodeBlock::finalizeUnconditionally):
>+        * heap/CodeBlockSet.cpp:
>+        (JSC::CodeBlockSet::dump const):
>+        (JSC::CodeBlockSet::clearCurrentlyExecuting): Deleted.
>+        * heap/CodeBlockSet.h:
>+        * heap/CodeBlockSetInlines.h:
>+        (JSC::CodeBlockSet::mark):
>+        (JSC::CodeBlockSet::unmark):
>+        (JSC::CodeBlockSet::iterateViaSubspaces):
>+        (JSC::CodeBlockSet::iterateCurrentlyExecuting):
>+        * heap/ConservativeRoots.cpp:
>+        (JSC::CompositeMarkHook::CompositeMarkHook):
>+        (JSC::CompositeMarkHook::markKnownJSCell):
>+        (JSC::ConservativeRoots::add):
>+        * heap/Heap.cpp:
>+        (JSC::Heap::finalizeUnconditionalFinalizers):
>+        (JSC::Heap::iterateExecutingAndCompilingCodeBlocks):
>+        (JSC::Heap::deleteUnmarkedCompiledCode):
>+        (JSC::Heap::runEndPhase):
>+        * runtime/VM.h:
>+        (JSC::VM::forEachCodeBlockSpace): Deleted.
>+
> 2019-04-26  Keith Rollin  <krollin@apple.com>
> 
>         Enable new build rule for post-processing headers when using XCBuild
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index 0ab756cbb8fa9142c40f5e57aee4255c6353c88b..2d1fbd915cd4c894aec57a724d0191e169c50b93 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -974,8 +974,6 @@ void CodeBlock::visitChildren(SlotVisitor& visitor)
> 
>     stronglyVisitStrongReferences(locker, visitor);
>     stronglyVisitWeakReferences(locker, visitor);
>-    
>-    VM::SpaceAndSet::setFor(*subspace()).add(this);
> }
> 
> bool CodeBlock::shouldVisitStrongly(const ConcurrentJSLocker& locker)
>@@ -1374,8 +1372,6 @@ void CodeBlock::finalizeUnconditionally(VM& vm)
>         dfgCommon->recordedStatuses.finalize(vm);
>     }
> #endif // ENABLE(DFG_JIT)
>-
>-    VM::SpaceAndSet::setFor(*subspace()).remove(this);
> }
> 
> void CodeBlock::destroy(JSCell* cell)
>diff --git a/Source/JavaScriptCore/heap/CodeBlockSet.cpp b/Source/JavaScriptCore/heap/CodeBlockSet.cpp
>index 49fcd84f4ad97aae0aaf3f193c86ea0613f6d8b7..eac38048b3e4f89c96f93b85342def46985942e6 100644
>--- a/Source/JavaScriptCore/heap/CodeBlockSet.cpp
>+++ b/Source/JavaScriptCore/heap/CodeBlockSet.cpp
>@@ -50,21 +50,12 @@ bool CodeBlockSet::contains(const AbstractLocker&, void* candidateCodeBlock)
>     return m_codeBlocks.contains(codeBlock);
> }
> 
>-void CodeBlockSet::clearCurrentlyExecuting()
>-{
>-    m_currentlyExecuting.clear();
>-}
>-
> void CodeBlockSet::dump(PrintStream& out) const
> {
>     CommaPrinter comma;
>     out.print("{codeBlocks = [");
>     for (CodeBlock* codeBlock : m_codeBlocks)
>         out.print(comma, pointerDump(codeBlock));
>-    out.print("], currentlyExecuting = [");
>-    comma = CommaPrinter();
>-    for (CodeBlock* codeBlock : m_currentlyExecuting)
>-        out.print(comma, pointerDump(codeBlock));
>     out.print("]}");
> }
> 
>diff --git a/Source/JavaScriptCore/heap/CodeBlockSet.h b/Source/JavaScriptCore/heap/CodeBlockSet.h
>index c4c58274fcd0eb752bb614f60f680931edc79cbb..ab8494e9f0c9d6ee79105657bb28e0ec581a23f6 100644
>--- a/Source/JavaScriptCore/heap/CodeBlockSet.h
>+++ b/Source/JavaScriptCore/heap/CodeBlockSet.h
>@@ -49,10 +49,9 @@ class CodeBlockSet {
>     CodeBlockSet();
>     ~CodeBlockSet();
> 
>-    void mark(const AbstractLocker&, CodeBlock* candidateCodeBlock);
>+    void mark(CodeBlock* candidateCodeBlock);
>+    void unmark(CodeBlock*);
>     
>-    void clearCurrentlyExecuting();
>-
>     bool contains(const AbstractLocker&, void* candidateCodeBlock);
>     Lock& getLock() { return m_lock; }
> 
>@@ -64,7 +63,7 @@ class CodeBlockSet {
> 
>     template<typename Functor> void iterateViaSubspaces(VM&, const Functor&);
>     
>-    template<typename Functor> void iterateCurrentlyExecuting(const Functor&);
>+    template<typename Functor> void iterateCurrentlyExecuting(VM&, const Functor&);
>     
>     void dump(PrintStream&) const;
>     
>@@ -73,7 +72,6 @@ class CodeBlockSet {
> 
> private:
>     HashSet<CodeBlock*> m_codeBlocks;
>-    HashSet<CodeBlock*> m_currentlyExecuting;
>     Lock m_lock;
> };
> 
>diff --git a/Source/JavaScriptCore/heap/CodeBlockSetInlines.h b/Source/JavaScriptCore/heap/CodeBlockSetInlines.h
>index 5ddbf494ab23e8080b9932a0d2f9e3cdecad5276..4e2d892315b178bead134e3a50314f21b8baee11 100644
>--- a/Source/JavaScriptCore/heap/CodeBlockSetInlines.h
>+++ b/Source/JavaScriptCore/heap/CodeBlockSetInlines.h
>@@ -30,15 +30,20 @@
> 
> #include "CodeBlock.h"
> #include "CodeBlockSet.h"
>+#include "IsoCellSetInlines.h"
> 
> namespace JSC {
> 
>-inline void CodeBlockSet::mark(const AbstractLocker&, CodeBlock* codeBlock)
>+inline void CodeBlockSet::mark(CodeBlock* codeBlock)
> {
>-    if (!codeBlock)
>-        return;
>+    ASSERT(codeBlock);
>+    codeBlock->vm()->codeBlockSpace.set.add(codeBlock);
>+}
> 
>-    m_currentlyExecuting.add(codeBlock);
>+inline void CodeBlockSet::unmark(CodeBlock* codeBlock)
>+{
>+    ASSERT(codeBlock);
>+    codeBlock->vm()->codeBlockSpace.set.remove(codeBlock);
> }
> 
> template<typename Functor>
>@@ -58,21 +63,19 @@ void CodeBlockSet::iterate(const AbstractLocker&, const Functor& functor)
> template<typename Functor>
> void CodeBlockSet::iterateViaSubspaces(VM& vm, const Functor& functor)
> {
>-    vm.forEachCodeBlockSpace(
>-        [&] (auto& spaceAndSet) {
>-            spaceAndSet.space.forEachLiveCell(
>-                [&] (HeapCell* cell, HeapCell::Kind) {
>-                    functor(jsCast<CodeBlock*>(static_cast<JSCell*>(cell)));
>-                });
>+    vm.codeBlockSpace.space.forEachLiveCell(
>+        [&] (HeapCell* cell, HeapCell::Kind) {
>+            functor(jsCast<CodeBlock*>(static_cast<JSCell*>(cell)));
>         });
> }
> 
> template<typename Functor>
>-void CodeBlockSet::iterateCurrentlyExecuting(const Functor& functor)
>+void CodeBlockSet::iterateCurrentlyExecuting(VM& vm, const Functor& functor)
> {
>-    LockHolder locker(&m_lock);
>-    for (CodeBlock* codeBlock : m_currentlyExecuting)
>-        functor(codeBlock);
>+    vm.codeBlockSpace.set.forEachMarkedCell(
>+        [&] (HeapCell* cell, HeapCell::Kind) {
>+            functor(jsCast<CodeBlock*>(static_cast<JSCell*>(cell)));
>+        });
> }
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/heap/ConservativeRoots.cpp b/Source/JavaScriptCore/heap/ConservativeRoots.cpp
>index a420fe8e625c993ed594d0acf58aecb76c8c995e..38c90dbbd3e3e7d537dff84db80abd116e5e125c 100644
>--- a/Source/JavaScriptCore/heap/ConservativeRoots.cpp
>+++ b/Source/JavaScriptCore/heap/ConservativeRoots.cpp
>@@ -117,10 +117,9 @@ void ConservativeRoots::add(void* begin, void* end)
> 
> class CompositeMarkHook {
> public:
>-    CompositeMarkHook(JITStubRoutineSet& stubRoutines, CodeBlockSet& codeBlocks, const AbstractLocker& locker)
>+    CompositeMarkHook(JITStubRoutineSet& stubRoutines, CodeBlockSet& codeBlocks)
>         : m_stubRoutines(stubRoutines)
>         , m_codeBlocks(codeBlocks)
>-        , m_codeBlocksLocker(locker)
>     {
>     }
>     
>@@ -132,20 +131,18 @@ class CompositeMarkHook {
>     void markKnownJSCell(JSCell* cell)
>     {
>         if (cell->type() == CodeBlockType)
>-            m_codeBlocks.mark(m_codeBlocksLocker, jsCast<CodeBlock*>(cell));
>+            m_codeBlocks.mark(jsCast<CodeBlock*>(cell));
>     }
> 
> private:
>     JITStubRoutineSet& m_stubRoutines;
>     CodeBlockSet& m_codeBlocks;
>-    const AbstractLocker& m_codeBlocksLocker;
> };
> 
> void ConservativeRoots::add(
>     void* begin, void* end, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks)
> {
>-    LockHolder locker(codeBlocks.getLock());
>-    CompositeMarkHook markHook(jitStubRoutines, codeBlocks, locker);
>+    CompositeMarkHook markHook(jitStubRoutines, codeBlocks);
>     genericAddSpan(begin, end, markHook);
> }
> 
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index 4953ac259223f32a1a0ad6e96182ffbc16aaa522..7ab52d5d127654cab6179a2ce0cb270238f2d358 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -590,10 +590,7 @@ void Heap::finalizeUnconditionalFinalizers()
>     vm()->builtinExecutables()->finalizeUnconditionally();
>     if (vm()->m_inferredValueSpace)
>         finalizeMarkedUnconditionalFinalizers<InferredValue>(vm()->m_inferredValueSpace->space);
>-    vm()->forEachCodeBlockSpace(
>-        [&] (auto& space) {
>-            this->finalizeMarkedUnconditionalFinalizers<CodeBlock>(space.set);
>-        });
>+    finalizeMarkedUnconditionalFinalizers<CodeBlock>(vm()->codeBlockSpace.space);
>     finalizeMarkedUnconditionalFinalizers<ExecutableToCodeBlockEdge>(vm()->executableToCodeBlockEdgesWithFinalizers);
>     finalizeMarkedUnconditionalFinalizers<StructureRareData>(vm()->structureRareDataSpace);
>     if (vm()->m_weakSetSpace)
>@@ -632,7 +629,7 @@ void Heap::completeAllJITPlans()
> template<typename Func>
> void Heap::iterateExecutingAndCompilingCodeBlocks(const Func& func)
> {
>-    m_codeBlocks->iterateCurrentlyExecuting(func);
>+    m_codeBlocks->iterateCurrentlyExecuting(*m_vm, func);
>     if (VM::canUseJIT())
>         DFG::iterateCodeBlocksForGC(*m_vm, func);
> }
>@@ -962,7 +959,7 @@ void Heap::deleteAllUnlinkedCodeBlocks(DeleteAllCodeEffort effort)
> void Heap::deleteUnmarkedCompiledCode()
> {
>     vm()->forEachScriptExecutableSpace([] (auto& space) { space.space.sweep(); });
>-    vm()->forEachCodeBlockSpace([] (auto& space) { space.space.sweep(); }); // Sweeping must occur before deleting stubs, otherwise the stubs might still think they're alive as they get deleted.
>+    vm()->codeBlockSpace.space.sweep(); // Sweeping must occur before deleting stubs, otherwise the stubs might still think they're alive as they get deleted.
>     m_jitStubRoutines->deleteUnmarkedJettisonedStubRoutines();
> }
> 
>@@ -1506,10 +1503,11 @@ NEVER_INLINE bool Heap::runEndPhase(GCConductor conn)
>     notifyIncrementalSweeper();
>     
>     m_codeBlocks->iterateCurrentlyExecuting(
>+        *vm(),
>         [&] (CodeBlock* codeBlock) {
>             writeBarrier(codeBlock);
>+            m_codeBlocks->unmark(codeBlock);
>         });
>-    m_codeBlocks->clearCurrentlyExecuting();
>         
>     m_objectSpace.prepareForAllocation();
>     updateAllocationLimits();
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index b985d47ea4d95edddc9c0cf36689653376f7d1df..03bdef940717c33fc2b98511bbad184b518894c0 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -452,15 +452,6 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
>     
>     SpaceAndSet codeBlockSpace;
>     DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(inferredValueSpace)
>-
>-    template<typename Func>
>-    void forEachCodeBlockSpace(const Func& func)
>-    {
>-        // This should not include webAssemblyCodeBlockSpace because this is about subsclasses of
>-        // JSC::CodeBlock.
>-        func(codeBlockSpace);
>-    }
>-
>     DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(evalExecutableSpace)
>     DYNAMIC_SPACE_AND_SET_DEFINE_MEMBER(moduleProgramExecutableSpace)
>     SpaceAndSet functionExecutableSpace;

Actions: View | Formatted Diff | Diff

Attachments on bug 186740: 368426 | 368428 | 369261