Attachment 362102 Details for Bug 194693 – Patch

[patch] Patch

bug-194693-20190215003345.patch (text/plain), 14.72 KB, created by Yusuke Suzuki on 2019-02-15 00:33:46 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-02-15 00:33:46 PST

Size: 14.72 KB

patch

obsolete

>Subversion Revision: 241577
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 24db8ff6f92a5f239c9e3d08c29e35e1c5add641..498a71a2016995b7d1daba4cb0e3c5a7404c723a 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,47 @@
>+2019-02-15  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Do not even allocate JIT worklists in non-JIT mode
>+        https://bugs.webkit.org/show_bug.cgi?id=194693
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Heap always allocates JIT worklists for Baseline, DFG, and FTL. While they do not have actual threads, Worklist itself already allocates some memory.
>+        And we do not perform any GC operations that are only meaningful in JIT environment.
>+
>+        1. We add VM::canUseJIT() check in Heap's ensureXXXWorklist things to prevent them from being allocated.
>+        2. We remove DFG marking constraint in non-JIT mode.
>+        3. We do not gather conservative roots from scratch buffers under the non-JIT mode (BTW, # of scratch buffers are always zero in non-JIT mode)
>+        4. We do not visit JITStubRoutineSet.
>+        5. Align JITWorklist function names to the other worklists.
>+
>+        * dfg/DFGOSRExitPreparation.cpp:
>+        (JSC::DFG::prepareCodeOriginForOSRExit):
>+        * dfg/DFGPlan.h:
>+        * dfg/DFGWorklist.cpp:
>+        (JSC::DFG::markCodeBlocks): Deleted.
>+        * dfg/DFGWorklist.h:
>+        * heap/Heap.cpp:
>+        (JSC::Heap::completeAllJITPlans):
>+        (JSC::Heap::iterateExecutingAndCompilingCodeBlocks):
>+        (JSC::Heap::gatherScratchBufferRoots):
>+        (JSC::Heap::removeDeadCompilerWorklistEntries):
>+        (JSC::Heap::stopThePeriphery):
>+        (JSC::Heap::suspendCompilerThreads):
>+        (JSC::Heap::resumeCompilerThreads):
>+        (JSC::Heap::addCoreConstraints):
>+        * jit/JITWorklist.cpp:
>+        (JSC::JITWorklist::existingGlobalWorklistOrNull):
>+        (JSC::JITWorklist::ensureGlobalWorklist):
>+        (JSC::JITWorklist::instance): Deleted.
>+        * jit/JITWorklist.h:
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::jitCompileAndSetHeuristics):
>+        * runtime/VM.cpp:
>+        (JSC::VM::~VM):
>+        (JSC::VM::gatherScratchBufferRoots):
>+        (JSC::VM::gatherConservativeRoots): Deleted.
>+        * runtime/VM.h:
>+
> 2019-02-14  Saam barati  <sbarati@apple.com>
> 
>         lowerStackArgs should lower Lea32/64 on ARM64 to Add
>diff --git a/Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp b/Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp
>index 646c7e405dec6dba9efb5d499112a17abdebe11b..a94170a4fe9139fd3bd43fd88724b40afce1aadb 100644
>--- a/Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOSRExitPreparation.cpp
>@@ -43,7 +43,7 @@ void prepareCodeOriginForOSRExit(ExecState* exec, CodeOrigin codeOrigin)
>     
>     for (; codeOrigin.inlineCallFrame; codeOrigin = codeOrigin.inlineCallFrame->directCaller) {
>         CodeBlock* codeBlock = codeOrigin.inlineCallFrame->baselineCodeBlock.get();
>-        JITWorklist::instance()->compileNow(codeBlock);
>+        JITWorklist::ensureGlobalWorklist().compileNow(codeBlock);
>     }
> }
> 
>diff --git a/Source/JavaScriptCore/dfg/DFGPlan.h b/Source/JavaScriptCore/dfg/DFGPlan.h
>index 6d5f6683ce8f0cb97cf9c5df6d2028e7fc54d8f7..9afd45860a45bbe8267205455997a8cac7877b3d 100644
>--- a/Source/JavaScriptCore/dfg/DFGPlan.h
>+++ b/Source/JavaScriptCore/dfg/DFGPlan.h
>@@ -70,7 +70,6 @@ class Plan : public ThreadSafeRefCounted<Plan> {
>     
>     CompilationKey key();
>     
>-    void markCodeBlocks(SlotVisitor&);
>     template<typename Func>
>     void iterateCodeBlocksForGC(const Func&);
>     void checkLivenessAndVisitChildren(SlotVisitor&);
>diff --git a/Source/JavaScriptCore/dfg/DFGWorklist.cpp b/Source/JavaScriptCore/dfg/DFGWorklist.cpp
>index dece70e37071d4892fb4cd410422eaa6a73918ac..8fd16efe783a55d0303ca06201745fe4ecf0efe8 100644
>--- a/Source/JavaScriptCore/dfg/DFGWorklist.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGWorklist.cpp
>@@ -661,10 +661,6 @@ void completeAllPlansForVM(VM&)
> {
> }
> 
>-void markCodeBlocks(VM&, SlotVisitor&)
>-{
>-}
>-
> #endif // ENABLE(DFG_JIT)
> 
> } } // namespace JSC::DFG
>diff --git a/Source/JavaScriptCore/dfg/DFGWorklist.h b/Source/JavaScriptCore/dfg/DFGWorklist.h
>index 8a5de186fb2c2f73442a2f579fb30faee0d92c17..396ebb8af2d9ec569db3d26f6c0d3b07211a9024 100644
>--- a/Source/JavaScriptCore/dfg/DFGWorklist.h
>+++ b/Source/JavaScriptCore/dfg/DFGWorklist.h
>@@ -144,7 +144,6 @@ Worklist& existingWorklistForIndex(unsigned index);
> #endif // ENABLE(DFG_JIT)
> 
> void completeAllPlansForVM(VM&);
>-void markCodeBlocks(VM&, SlotVisitor&);
> 
> template<typename Func>
> void iterateCodeBlocksForGC(VM&, const Func&);
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index 5f2bc83f4767fdcaa23234d5fbb10e130a7f8c7f..c0d238396a91b0f6c6a9fb6f54012571e6650420 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -599,8 +599,10 @@ void Heap::didFinishIterating()
> 
> void Heap::completeAllJITPlans()
> {
>+    if (!VM::canUseJIT())
>+        return;
> #if ENABLE(JIT)
>-    JITWorklist::instance()->completeAllForVM(*m_vm);
>+    JITWorklist::ensureGlobalWorklist().completeAllForVM(*m_vm);
> #endif // ENABLE(JIT)
>     DFG::completeAllPlansForVM(*m_vm);
> }
>@@ -609,7 +611,8 @@ template<typename Func>
> void Heap::iterateExecutingAndCompilingCodeBlocks(const Func& func)
> {
>     m_codeBlocks->iterateCurrentlyExecuting(func);
>-    DFG::iterateCodeBlocksForGC(*m_vm, func);
>+    if (VM::canUseJIT())
>+        DFG::iterateCodeBlocksForGC(*m_vm, func);
> }
> 
> template<typename Func>
>@@ -667,7 +670,9 @@ void Heap::gatherJSStackRoots(ConservativeRoots& roots)
> void Heap::gatherScratchBufferRoots(ConservativeRoots& roots)
> {
> #if ENABLE(DFG_JIT)
>-    m_vm->gatherConservativeRoots(roots);
>+    if (!VM::canUseJIT())
>+        return;
>+    m_vm->gatherScratchBufferRoots(roots);
> #else
>     UNUSED_PARAM(roots);
> #endif
>@@ -684,6 +689,8 @@ void Heap::beginMarking()
> void Heap::removeDeadCompilerWorklistEntries()
> {
> #if ENABLE(DFG_JIT)
>+    if (!VM::canUseJIT())
>+        return;
>     for (unsigned i = DFG::numberOfWorklists(); i--;)
>         DFG::existingWorklistForIndex(i).removeDeadPlans(*m_vm);
> #endif
>@@ -1586,9 +1593,9 @@ void Heap::stopThePeriphery(GCConductor conn)
>         });
> 
> #if ENABLE(JIT)
>-    {
>+    if (VM::canUseJIT()) {
>         DeferGCForAWhile awhile(*this);
>-        if (JITWorklist::instance()->completeAllForVM(*m_vm)
>+        if (JITWorklist::ensureGlobalWorklist().completeAllForVM(*m_vm)
>             && conn == GCConductor::Collector)
>             setGCDidJIT();
>     }
>@@ -2093,6 +2100,8 @@ void Heap::suspendCompilerThreads()
>     // We ensure the worklists so that it's not possible for the mutator to start a new worklist
>     // after we have suspended the ones that he had started before. That's not very expensive since
>     // the worklists use AutomaticThreads anyway.
>+    if (!VM::canUseJIT())
>+        return;
>     for (unsigned i = DFG::numberOfWorklists(); i--;)
>         DFG::ensureWorklistForIndex(i).suspendAllThreads();
> #endif
>@@ -2310,6 +2319,8 @@ void Heap::didFinishCollection()
> void Heap::resumeCompilerThreads()
> {
> #if ENABLE(DFG_JIT)
>+    if (!VM::canUseJIT())
>+        return;
>     for (unsigned i = DFG::numberOfWorklists(); i--;)
>         DFG::existingWorklistForIndex(i).resumeAllThreads();
> #endif
>@@ -2640,7 +2651,7 @@ void Heap::addCoreConstraints()
>                 SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::ConservativeScan);
>                 slotVisitor.append(conservativeRoots);
>             }
>-            {
>+            if (VM::canUseJIT()) {
>                 // JITStubRoutines must be visited after scanning ConservativeRoots since JITStubRoutines depend on the hook executed during gathering ConservativeRoots.
>                 SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::JITStubRoutines);
>                 m_jitStubRoutines->traceMarkedStubRoutines(slotVisitor);
>@@ -2744,26 +2755,28 @@ void Heap::addCoreConstraints()
>         ConstraintParallelism::Parallel);
>     
> #if ENABLE(DFG_JIT)
>-    m_constraintSet->add(
>-        "Dw", "DFG Worklists",
>-        [this] (SlotVisitor& slotVisitor) {
>-            SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::DFGWorkLists);
>-
>-            for (unsigned i = DFG::numberOfWorklists(); i--;)
>-                DFG::existingWorklistForIndex(i).visitWeakReferences(slotVisitor);
>-            
>-            // FIXME: This is almost certainly unnecessary.
>-            // https://bugs.webkit.org/show_bug.cgi?id=166829
>-            DFG::iterateCodeBlocksForGC(
>-                *m_vm,
>-                [&] (CodeBlock* codeBlock) {
>-                    slotVisitor.appendUnbarriered(codeBlock);
>-                });
>-            
>-            if (Options::logGC() == GCLogging::Verbose)
>-                dataLog("DFG Worklists:\n", slotVisitor);
>-        },
>-        ConstraintVolatility::GreyedByMarking);
>+    if (VM::canUseJIT()) {
>+        m_constraintSet->add(
>+            "Dw", "DFG Worklists",
>+            [this] (SlotVisitor& slotVisitor) {
>+                SetRootMarkReasonScope rootScope(slotVisitor, SlotVisitor::RootMarkReason::DFGWorkLists);
>+
>+                for (unsigned i = DFG::numberOfWorklists(); i--;)
>+                    DFG::existingWorklistForIndex(i).visitWeakReferences(slotVisitor);
>+                
>+                // FIXME: This is almost certainly unnecessary.
>+                // https://bugs.webkit.org/show_bug.cgi?id=166829
>+                DFG::iterateCodeBlocksForGC(
>+                    *m_vm,
>+                    [&] (CodeBlock* codeBlock) {
>+                        slotVisitor.appendUnbarriered(codeBlock);
>+                    });
>+                
>+                if (Options::logGC() == GCLogging::Verbose)
>+                    dataLog("DFG Worklists:\n", slotVisitor);
>+            },
>+            ConstraintVolatility::GreyedByMarking);
>+    }
> #endif
>     
>     m_constraintSet->add(
>diff --git a/Source/JavaScriptCore/jit/JITWorklist.cpp b/Source/JavaScriptCore/jit/JITWorklist.cpp
>index 4c6ef4fb5780a5a0738a18208839a5cba0913642..1f369fcd66084e9ac43cbd5dc942ae319d9533c4 100644
>--- a/Source/JavaScriptCore/jit/JITWorklist.cpp
>+++ b/Source/JavaScriptCore/jit/JITWorklist.cpp
>@@ -322,16 +322,24 @@ void JITWorklist::finalizePlans(Plans& myPlans)
>     }
> }
> 
>-JITWorklist* JITWorklist::instance()
>+static JITWorklist* theGlobalJITWorklist { nullptr };
>+
>+JITWorklist* JITWorklist::existingGlobalWorklistOrNull()
>+{
>+    return theGlobalJITWorklist;
>+}
>+
>+JITWorklist& JITWorklist::ensureGlobalWorklist()
> {
>-    static JITWorklist* worklist;
>     static std::once_flag once;
>     std::call_once(
>         once,
>         [] {
>-            worklist = new JITWorklist();
>+            auto* worklist = new JITWorklist();
>+            WTF::storeStoreFence();
>+            theGlobalJITWorklist = worklist;
>         });
>-    return worklist;
>+    return *theGlobalJITWorklist;
> }
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/jit/JITWorklist.h b/Source/JavaScriptCore/jit/JITWorklist.h
>index adf9fb9bb3fec53d61c7f8c1c8699e98d715e3c3..eb6f5515df70397e730c1c27999ffb382f6b2b61 100644
>--- a/Source/JavaScriptCore/jit/JITWorklist.h
>+++ b/Source/JavaScriptCore/jit/JITWorklist.h
>@@ -57,7 +57,8 @@ class JITWorklist {
>     
>     void compileNow(CodeBlock*, unsigned loopOSREntryBytecodeOffset = 0);
>     
>-    static JITWorklist* instance();
>+    static JITWorklist& ensureGlobalWorklist();
>+    static JITWorklist* existingGlobalWorklistOrNull();
>     
> private:
>     JITWorklist();
>diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>index a9a85507d21f37d23ff82c56016564a910e2fc35..0406c92a763bc8ad5a07f13e5f9a719d7b759817 100644
>--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>@@ -369,6 +369,7 @@ inline bool jitCompileAndSetHeuristics(CodeBlock* codeBlock, ExecState* exec, un
> {
>     VM& vm = exec->vm();
>     DeferGCForAWhile deferGC(vm.heap); // My callers don't set top callframe, so we don't want to GC here at all.
>+    ASSERT(VM::canUseJIT());
>     
>     codeBlock->updateAllValueProfilePredictions();
> 
>@@ -379,7 +380,7 @@ inline bool jitCompileAndSetHeuristics(CodeBlock* codeBlock, ExecState* exec, un
>         return false;
>     }
>     
>-    JITWorklist::instance()->poll(vm);
>+    JITWorklist::ensureGlobalWorklist().poll(vm);
>     
>     switch (codeBlock->jitType()) {
>     case JITCode::BaselineJIT: {
>@@ -389,7 +390,7 @@ inline bool jitCompileAndSetHeuristics(CodeBlock* codeBlock, ExecState* exec, un
>         return true;
>     }
>     case JITCode::InterpreterThunk: {
>-        JITWorklist::instance()->compileLater(codeBlock, loopOSREntryBytecodeOffset);
>+        JITWorklist::ensureGlobalWorklist().compileLater(codeBlock, loopOSREntryBytecodeOffset);
>         return codeBlock->jitType() == JITCode::BaselineJIT;
>     }
>     default:
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index 3d1f724c4fd23aa65b33aa71e35eb4d75b40d26c..79022d62181168273e0e688b9b19a645b4baec69 100644
>--- a/Source/JavaScriptCore/runtime/VM.cpp
>+++ b/Source/JavaScriptCore/runtime/VM.cpp
>@@ -497,8 +497,8 @@ VM::~VM()
>     Gigacage::removePrimitiveDisableCallback(primitiveGigacageDisabledCallback, this);
>     promiseDeferredTimer->stopRunningTasks();
> #if ENABLE(WEBASSEMBLY)
>-    if (Wasm::existingWorklistOrNull())
>-        Wasm::ensureWorklist().stopAllPlansForContext(wasmContext);
>+    if (Wasm::Worklist* worklist = Wasm::existingWorklistOrNull())
>+        worklist->stopAllPlansForContext(wasmContext);
> #endif
>     if (UNLIKELY(m_watchdog))
>         m_watchdog->willDestroyVM(this);
>@@ -516,7 +516,8 @@ VM::~VM()
> #endif // ENABLE(SAMPLING_PROFILER)
>     
> #if ENABLE(JIT)
>-    JITWorklist::instance()->completeAllForVM(*this);
>+    if (JITWorklist* worklist = JITWorklist::existingGlobalWorklistOrNull())
>+        worklist->completeAllForVM(*this);
> #endif // ENABLE(JIT)
> 
> #if ENABLE(DFG_JIT)
>@@ -921,7 +922,7 @@ inline void VM::updateStackLimits()
> }
> 
> #if ENABLE(DFG_JIT)
>-void VM::gatherConservativeRoots(ConservativeRoots& conservativeRoots)
>+void VM::gatherScratchBufferRoots(ConservativeRoots& conservativeRoots)
> {
>     auto lock = holdLock(m_scratchBufferLock);
>     for (auto* scratchBuffer : m_scratchBuffers) {
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index bb9366aa66f65cd35cef7b86f897643e5d33b779..5ab352ff61c572f87436614ffd20555b33976e0c 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -740,7 +740,7 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
>         return m_exceptionFuzzBuffer.get();
>     }
> 
>-    void gatherConservativeRoots(ConservativeRoots&);
>+    void gatherScratchBufferRoots(ConservativeRoots&);
> 
>     VMEntryScope* entryScope;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 194693: 362102