Attachment 371474 Details for Bug 198023 – Patch

[patch] Patch

bug-198023-20190605221523.patch (text/plain), 19.41 KB, created by Yusuke Suzuki on 2019-06-05 22:15:23 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-05 22:15:23 PDT

Size: 19.41 KB

patch

obsolete

>Subversion Revision: 246084
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 521baaaa39e8ed45945f2be0f4398681e8473d70..2b6892431fdcfd840d1d8dffbf55b131ef50ef9c 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,75 @@
>+2019-06-05  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] UnlinkedCodeBlock should be eventually jettisoned in VM mini mode
>+        https://bugs.webkit.org/show_bug.cgi?id=198023
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        While CodeBlock is periodically jettisoned, UnlinkedCodeBlock and UnlinkedFunctionExecutable can be retained almost forever in certain type of applications.
>+        When we execute a program, which has UnlinkedProgramCodeBlock retained in CodeCache. And UnlinkedProgramCodeBlock holds array of UnlinkedFunctionExecutable.
>+        And UnlinkedFunctionExecutables hold UnlinkedFunctionCodeBlocks once it is generated. So eventually, this tree gets larger and larger until we purge
>+        UnlinkedProgramCodeBlock from CodeCache. This is OK in the browser case. We navigate to various other pages, and UnlinkedProgramCodeBlocks should eventually
>+        be pruned from CodeCache with the new ones. So this tree won't be retained forever. But the behavior is different in the other applications that do not have
>+        navigations. If they only have one program which holds all, we basically retain this tree during executing this application. The same thing can happen in
>+        web applications which does not have navigation and keeps alive for a long time. Once we hit CodeCache limit by periodically executing a new script, we will
>+        hit the uppermost of memory footprint. But until that, we increase our memory footprint.
>+
>+        However, destroying these UnlinkedCodeBlocks and UnlinkedFunctionExecutables causes a tricky problem. In the browser environment, navigation can happen at any
>+        time. So even if the given UnlinkedCodeBlock seems unused in the current page, it can be used when navigating to a new page which is under the same domain.
>+        One example is initializing function in a script. It is only executed once per page. So once it is executed, it seems that this UnlinkedCodeBlock is unused.
>+        But this will be used when we navigate to a new page. Pruning code blocks based on usage could cause performance regression.
>+
>+        But if our VM is mini VM mode, the story is different. In mini VM mode, we focus on memory footprint rather than performance e.g. daemons. The daemon never
>+        reuse these CodeCache since we do not have the navigation.
>+
>+        This patch logically makes UnlinkedFunctionExecutable -> UnlinkedCodeBlock reference weak when VM is mini mode. If UnlinkedCodeBlock is used in previous GC
>+        cycle, we retain it. But if it is not used, and if UnlinkedFunctionExecutable is only the cell keeping UnlinkedCodeBlock alive, we destroy it. It is a
>+        heuristic. In a super pathological case, it could increase memory footprint. Consider the following example.
>+
>+            UnlinkedFunctionExecutable(A1) -> UnlinkedCodeBlock(B1) -> UnlinkedFunctionExecutable(C1) -> UnlinkedCodeBlock(D1)
>+                                                                                                             ^
>+                                                                                                         CodeBlock(E1)
>+
>+        We could delete A1, B1, and C1 while keeping D1. But if we eventually re-execute the same code corresponding to A1, B1, C1, they will be newly created, and
>+        we will create duplicate UnlinkedCodeBlock and instructions stream for D1.
>+
>+                                                                                                         UnlinkedCodeBlock(D1)
>+                                                                                                             ^
>+                                                                                                         CodeBlock(E1)
>+
>+            UnlinkedFunctionExecutable(A2) -> UnlinkedCodeBlock(B2) -> UnlinkedFunctionExecutable(C2) -> UnlinkedCodeBlock(D2)
>+
>+        But this does not happen in practice and even it happens, we eventually discard D1 and D2 since CodeBlock E1 will be jettisoned anyway. So in practice, we do
>+        not see memory footprint increase. We tested it in Gmail and the target application, but both said memory footprint reduction (30 MB / 400 MB and 1 MB /6 MB).
>+        While this affects on performance much on tests which has navigation (1-3 % regression in Speedometer2, note that JetStream2 does not show regression), we do
>+        not apply this to non mini mode VM until we come up with a good strategy to fasten performance of re-generation. Personally I think flushing destroyed
>+        UnlinkedCodeBlock to the disk is promising.
>+
>+        If UnlinkedCodeBlock is generated from bytecode cache, we do not make UnlinkedFunctionExecutable -> UnlinkedCodeBlock link weak because the decoder of the bytecode
>+        cache assumes that generated JSCells won't be destroyed while the parent cells of that cell are live. This is true in the current implementation, and this assumption
>+        will be broken with this patch. So, for now, we do not make this link weak. Currently, our target application does not use bytecode cache so it is OK.
>+
>+        Since we do not have an automated script right now so it is a bit difficult to measure memory footprint precisely. But manual testing shows that tpatch improves memory
>+        footprint of our target application from about 6 MB to about 5 MB.
>+
>+        * bytecode/UnlinkedCodeBlock.cpp:
>+        (JSC::UnlinkedCodeBlock::UnlinkedCodeBlock):
>+        * bytecode/UnlinkedCodeBlock.h:
>+        (JSC::UnlinkedCodeBlock::usedInPreviousCycle const):
>+        (JSC::UnlinkedCodeBlock::setUsedInPreviousCycle):
>+        * bytecode/UnlinkedFunctionExecutable.cpp:
>+        (JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable):
>+        (JSC::UnlinkedFunctionExecutable::visitChildren):
>+        (JSC::UnlinkedFunctionExecutable::unlinkedCodeBlockFor):
>+        (JSC::UnlinkedFunctionExecutable::decodeCachedCodeBlocks):
>+        (JSC::UnlinkedFunctionExecutable::finalizeUnconditionally):
>+        * bytecode/UnlinkedFunctionExecutable.h:
>+        * heap/Heap.cpp:
>+        (JSC::Heap::finalizeUnconditionalFinalizers):
>+        * runtime/CachedTypes.cpp:
>+        (JSC::UnlinkedCodeBlock::UnlinkedCodeBlock):
>+        (JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable):
>+
> 2019-06-04  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         Unreviewed, update exception scope for putByIndexBeyondVectorLength
>diff --git a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
>index f9c3ba9d71647d423db18a14419804d34f30f441..437b3a823252a427cf3ab9a1eca2ec98d4ed84c7 100644
>--- a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
>@@ -71,6 +71,7 @@ UnlinkedCodeBlock::UnlinkedCodeBlock(VM* vm, Structure* structure, CodeType code
>     , m_evalContextType(static_cast<unsigned>(info.evalContextType()))
>     , m_codeType(static_cast<unsigned>(codeType))
>     , m_didOptimize(static_cast<unsigned>(MixedTriState))
>+    , m_usedInPreviousCycle(false)
>     , m_parseMode(info.parseMode())
>     , m_codeGenerationMode(codeGenerationMode)
>     , m_metadata(UnlinkedMetadataTable::create())
>diff --git a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h
>index 2a350a1f11cf2131f72c2b4bd809018fe712eff2..61a76744033f35aa9c6c81363086054661549b8a 100644
>--- a/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h
>+++ b/Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h
>@@ -370,6 +370,12 @@ class UnlinkedCodeBlock : public JSCell {
>         return m_metadata->sizeInBytes();
>     }
> 
>+    bool usedInPreviousCycle() const { return m_usedInPreviousCycle; }
>+
>+    void setUsedInPreviousCycle(bool used)
>+    {
>+        m_usedInPreviousCycle = used;
>+    }
> 
> protected:
>     UnlinkedCodeBlock(VM*, Structure*, CodeType, const ExecutableInfo&, OptionSet<CodeGenerationMode>);
>@@ -424,6 +430,7 @@ class UnlinkedCodeBlock : public JSCell {
>     unsigned m_evalContextType : 2;
>     unsigned m_codeType : 2;
>     unsigned m_didOptimize : 2;
>+    unsigned m_usedInPreviousCycle : 1;
> public:
>     ConcurrentJSLock m_lock;
> private:
>diff --git a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
>index 14a069c5e07608d309182293656eb2187fd97fd6..0312d59ec214ab8469ab69aa2f8460b458f64f6c 100644
>--- a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
>+++ b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
>@@ -107,6 +107,7 @@ UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* struct
>     , m_constructorKind(static_cast<unsigned>(node->constructorKind()))
>     , m_functionMode(static_cast<unsigned>(node->functionMode()))
>     , m_derivedContextType(static_cast<unsigned>(derivedContextType))
>+    , m_isGeneratedFromCache(false)
>     , m_unlinkedCodeBlockForCall()
>     , m_unlinkedCodeBlockForConstruct()
>     , m_name(node->ident())
>@@ -142,7 +143,21 @@ void UnlinkedFunctionExecutable::visitChildren(JSCell* cell, SlotVisitor& visito
>     UnlinkedFunctionExecutable* thisObject = jsCast<UnlinkedFunctionExecutable*>(cell);
>     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
>     Base::visitChildren(thisObject, visitor);
>-    if (!thisObject->m_isCached) {
>+
>+    // Currently, bytecode cache assumes that the tree of UnlinkedFunctionExecutable and UnlinkedCodeBlock will not be destroyed while the parent is live.
>+    // Bytecode cache uses this asumption to avoid duplicate materialization by bookkeeping the heap cells in the offste-to-pointer map.
>+    if (VM::isInMiniMode() && !thisObject->m_isGeneratedFromCache) {
>+        auto markIfProfitable = [&] (WriteBarrier<UnlinkedFunctionCodeBlock>& unlinkedCodeBlock) {
>+            if (!unlinkedCodeBlock)
>+                return;
>+            if (unlinkedCodeBlock->didOptimize() == TrueTriState)
>+                visitor.append(unlinkedCodeBlock);
>+            else if (unlinkedCodeBlock->usedInPreviousCycle())
>+                visitor.append(unlinkedCodeBlock);
>+        };
>+        markIfProfitable(thisObject->m_unlinkedCodeBlockForCall);
>+        markIfProfitable(thisObject->m_unlinkedCodeBlockForConstruct);
>+    } else if (!thisObject->m_isCached) {
>         visitor.append(thisObject->m_unlinkedCodeBlockForCall);
>         visitor.append(thisObject->m_unlinkedCodeBlockForConstruct);
>     }
>@@ -197,56 +212,49 @@ UnlinkedFunctionExecutable* UnlinkedFunctionExecutable::fromGlobalCode(
>     return executable;
> }
> 
>-UnlinkedFunctionCodeBlock* UnlinkedFunctionExecutable::unlinkedCodeBlockFor(CodeSpecializationKind specializationKind)
>-{
>-    switch (specializationKind) {
>-    case CodeForCall:
>-        return m_unlinkedCodeBlockForCall.get();
>-    case CodeForConstruct:
>-        return m_unlinkedCodeBlockForConstruct.get();
>-    }
>-    ASSERT_NOT_REACHED();
>-    return nullptr;
>-}
>-
> UnlinkedFunctionCodeBlock* UnlinkedFunctionExecutable::unlinkedCodeBlockFor(
>     VM& vm, const SourceCode& source, CodeSpecializationKind specializationKind, 
>     OptionSet<CodeGenerationMode> codeGenerationMode, ParserError& error, SourceParseMode parseMode)
> {
>-    if (m_isCached)
>-        decodeCachedCodeBlocks();
>-    switch (specializationKind) {
>-    case CodeForCall:
>-        if (UnlinkedFunctionCodeBlock* codeBlock = m_unlinkedCodeBlockForCall.get())
>-            return codeBlock;
>-        break;
>-    case CodeForConstruct:
>-        if (UnlinkedFunctionCodeBlock* codeBlock = m_unlinkedCodeBlockForConstruct.get())
>-            return codeBlock;
>-        break;
>-    }
>-
>-    UnlinkedFunctionCodeBlock* result = generateUnlinkedFunctionCodeBlock(
>-        vm, this, source, specializationKind, codeGenerationMode, 
>-        isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, 
>-        error, parseMode);
>-    
>-    if (error.isValid())
>-        return nullptr;
>-
>-    switch (specializationKind) {
>-    case CodeForCall:
>-        m_unlinkedCodeBlockForCall.set(vm, this, result);
>-        break;
>-    case CodeForConstruct:
>-        m_unlinkedCodeBlockForConstruct.set(vm, this, result);
>-        break;
>-    }
>-    vm.unlinkedFunctionExecutableSpace.set.add(this);
>-    return result;
>+    UnlinkedFunctionCodeBlock* unlinkedCodeBlock = ([&] () -> UnlinkedFunctionCodeBlock* {
>+        if (m_isCached)
>+            decodeCachedCodeBlocks(vm);
>+        switch (specializationKind) {
>+        case CodeForCall:
>+            if (UnlinkedFunctionCodeBlock* codeBlock = m_unlinkedCodeBlockForCall.get())
>+                return codeBlock;
>+            break;
>+        case CodeForConstruct:
>+            if (UnlinkedFunctionCodeBlock* codeBlock = m_unlinkedCodeBlockForConstruct.get())
>+                return codeBlock;
>+            break;
>+        }
>+
>+        UnlinkedFunctionCodeBlock* result = generateUnlinkedFunctionCodeBlock(
>+            vm, this, source, specializationKind, codeGenerationMode, 
>+            isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, 
>+            error, parseMode);
>+        
>+        if (error.isValid())
>+            return nullptr;
>+
>+        switch (specializationKind) {
>+        case CodeForCall:
>+            m_unlinkedCodeBlockForCall.set(vm, this, result);
>+            break;
>+        case CodeForConstruct:
>+            m_unlinkedCodeBlockForConstruct.set(vm, this, result);
>+            break;
>+        }
>+        vm.unlinkedFunctionExecutableSpace.set.add(this);
>+        return result;
>+    })();
>+    if (unlinkedCodeBlock)
>+        unlinkedCodeBlock->setUsedInPreviousCycle(true);
>+    return unlinkedCodeBlock;
> }
> 
>-void UnlinkedFunctionExecutable::decodeCachedCodeBlocks()
>+void UnlinkedFunctionExecutable::decodeCachedCodeBlocks(VM& vm)
> {
>     ASSERT(m_isCached);
>     ASSERT(m_decoder);
>@@ -256,7 +264,7 @@ void UnlinkedFunctionExecutable::decodeCachedCodeBlocks()
>     int32_t cachedCodeBlockForCallOffset = m_cachedCodeBlockForCallOffset;
>     int32_t cachedCodeBlockForConstructOffset = m_cachedCodeBlockForConstructOffset;
> 
>-    DeferGC deferGC(decoder->vm().heap);
>+    DeferGC deferGC(vm.heap);
> 
>     // No need to clear m_unlinkedCodeBlockForCall here, since we moved the decoder out of the same slot
>     if (cachedCodeBlockForCallOffset)
>@@ -268,7 +276,7 @@ void UnlinkedFunctionExecutable::decodeCachedCodeBlocks()
> 
>     WTF::storeStoreFence();
>     m_isCached = false;
>-    decoder->vm().heap.writeBarrier(this);
>+    vm.heap.writeBarrier(this);
> }
> 
> UnlinkedFunctionExecutable::RareData& UnlinkedFunctionExecutable::ensureRareDataSlow()
>@@ -284,4 +292,27 @@ void UnlinkedFunctionExecutable::setInvalidTypeProfilingOffsets()
>     m_typeProfilingEndOffset = std::numeric_limits<unsigned>::max();
> }
> 
>+void UnlinkedFunctionExecutable::finalizeUnconditionally(VM& vm)
>+{
>+    if (VM::isInMiniMode() && !m_isGeneratedFromCache) {
>+        bool isCleared = false;
>+        bool isStillValid = false;
>+        auto clearIfDead = [&] (WriteBarrier<UnlinkedFunctionCodeBlock>& unlinkedCodeBlock) {
>+            if (!unlinkedCodeBlock)
>+                return;
>+            if (!vm.heap.isMarked(unlinkedCodeBlock.get())) {
>+                unlinkedCodeBlock.clear();
>+                isCleared = true;
>+            } else {
>+                unlinkedCodeBlock->setUsedInPreviousCycle(false);
>+                isStillValid = true;
>+            }
>+        };
>+        clearIfDead(m_unlinkedCodeBlockForCall);
>+        clearIfDead(m_unlinkedCodeBlockForConstruct);
>+        if (isCleared && !isStillValid)
>+            vm.unlinkedFunctionExecutableSpace.set.remove(this);
>+    }
>+}
>+
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>index 0c7f89cd4eb024fcf80d78836c3f43c0a7cd23c3..bed95d309e7f16dcd69792ffed44a0d1e2dc8f11 100644
>--- a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>+++ b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>@@ -114,8 +114,6 @@ class UnlinkedFunctionExecutable final : public JSCell {
>     unsigned typeProfilingEndOffset() const { return m_typeProfilingEndOffset; }
>     void setInvalidTypeProfilingOffsets();
> 
>-    UnlinkedFunctionCodeBlock* unlinkedCodeBlockFor(CodeSpecializationKind);
>-
>     UnlinkedFunctionCodeBlock* unlinkedCodeBlockFor(
>         VM&, const SourceCode&, CodeSpecializationKind, OptionSet<CodeGenerationMode>,
>         ParserError&, SourceParseMode);
>@@ -189,6 +187,8 @@ class UnlinkedFunctionExecutable final : public JSCell {
>         ensureRareData().m_sourceMappingURLDirective = sourceMappingURL;
>     }
> 
>+    void finalizeUnconditionally(VM&);
>+
>     struct RareData {
>         WTF_MAKE_STRUCT_FAST_ALLOCATED;
> 
>@@ -202,7 +202,7 @@ class UnlinkedFunctionExecutable final : public JSCell {
>     UnlinkedFunctionExecutable(VM*, Structure*, const SourceCode&, FunctionMetadataNode*, UnlinkedFunctionKind, ConstructAbility, JSParserScriptMode, Optional<CompactVariableMap::Handle>,  JSC::DerivedContextType, bool isBuiltinDefaultClassConstructor);
>     UnlinkedFunctionExecutable(Decoder&, const CachedFunctionExecutable&);
> 
>-    void decodeCachedCodeBlocks();
>+    void decodeCachedCodeBlocks(VM&);
> 
>     unsigned m_firstLineOffset : 31;
>     unsigned m_isInStrictContext : 1;
>@@ -228,6 +228,7 @@ class UnlinkedFunctionExecutable final : public JSCell {
>     unsigned m_constructorKind : 2;
>     unsigned m_functionMode : 2; // FunctionMode
>     unsigned m_derivedContextType: 2;
>+    unsigned m_isGeneratedFromCache : 1;
> 
>     union {
>         WriteBarrier<UnlinkedFunctionCodeBlock> m_unlinkedCodeBlockForCall;
>diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
>index 6663b4f0d39ebbef9d270de03d2b5fc5e33d6482..a85207f12cf603f047881d164cc678274483ffee 100644
>--- a/Source/JavaScriptCore/heap/Heap.cpp
>+++ b/Source/JavaScriptCore/heap/Heap.cpp
>@@ -606,6 +606,7 @@ void Heap::finalizeUnconditionalFinalizers()
>         });
>     finalizeMarkedUnconditionalFinalizers<ExecutableToCodeBlockEdge>(vm()->executableToCodeBlockEdgesWithFinalizers);
>     finalizeMarkedUnconditionalFinalizers<StructureRareData>(vm()->structureRareDataSpace);
>+    finalizeMarkedUnconditionalFinalizers<UnlinkedFunctionExecutable>(vm()->unlinkedFunctionExecutableSpace.set);
>     if (vm()->m_weakSetSpace)
>         finalizeMarkedUnconditionalFinalizers<JSWeakSet>(*vm()->m_weakSetSpace);
>     if (vm()->m_weakMapSpace)
>diff --git a/Source/JavaScriptCore/runtime/CachedTypes.cpp b/Source/JavaScriptCore/runtime/CachedTypes.cpp
>index d461e569329f1d9e8b86ecbc357b9ae7699d718a..b7bc2a724c36ef81f8a7245cd476391091cb3130 100644
>--- a/Source/JavaScriptCore/runtime/CachedTypes.cpp
>+++ b/Source/JavaScriptCore/runtime/CachedTypes.cpp
>@@ -2032,6 +2032,8 @@ ALWAYS_INLINE UnlinkedCodeBlock::UnlinkedCodeBlock(Decoder& decoder, Structure*
> 
>     , m_didOptimize(static_cast<unsigned>(MixedTriState))
> 
>+    , m_usedInPreviousCycle(false)
>+
>     , m_features(cachedCodeBlock.features())
>     , m_parseMode(cachedCodeBlock.parseMode())
>     , m_codeGenerationMode(cachedCodeBlock.codeGenerationMode())
>@@ -2158,6 +2160,7 @@ ALWAYS_INLINE UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(Decoder& de
>     , m_constructorKind(cachedExecutable.constructorKind())
>     , m_functionMode(cachedExecutable.functionMode())
>     , m_derivedContextType(cachedExecutable.derivedContextType())
>+    , m_isGeneratedFromCache(true)
>     , m_unlinkedCodeBlockForCall()
>     , m_unlinkedCodeBlockForConstruct()
>

Actions: View | Formatted Diff | Diff

Attachments on bug 198023: 370283 | 371361 | 371455 | 371472 | 371473 | 371474 | 371560 | 371564 | 371569 | 371624