Attachment 362702 Details for Bug 194706 – Patch for landing

[patch] Patch for landing

bug-194706-20190222093017.patch (text/plain), 14.25 KB, created by Tadeu Zagallo on 2019-02-22 00:30:57 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-02-22 00:30:57 PST

Size: 14.25 KB

patch

obsolete

>Subversion Revision: 241937
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index d72d8518ee5e210a1ef000e77921663f640a3f7b..716beccc4c15d333cad04d69aa3a2bd92e341530 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-02-19  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        Cache CompactVariableMap::Handle instead of VariableEnvironment for UnlinkedFunctionExecutable
>+        https://bugs.webkit.org/show_bug.cgi?id=194706
>+
>+        Reviewed by Saam Barati.
>+
>+        In https://bugs.webkit.org/show_bug.cgi?id=194583 we started using a
>+        CompactVariableMap::Handle instead of VariableEnvironment for
>+        UnlinkedFunctionExecutables, but we were creating the full environment
>+        to encode the executable in the bytecode cache. This patch changes it so
>+        that we cache the handle instead of the environment. This avoids duplicating
>+        the VariableEnvironment whenever we have to cache two handles that point
>+        to the environment.
>+
>+        * bytecode/UnlinkedFunctionExecutable.h:
>+        * parser/VariableEnvironment.cpp:
>+        (JSC::CompactVariableMap::get):
>+        * parser/VariableEnvironment.h:
>+        * runtime/CachedTypes.cpp:
>+        (JSC::CachedCompactVariableEnvironment::encode):
>+        (JSC::CachedCompactVariableEnvironment::decode const):
>+        (JSC::CachedCompactVariableMapHandle::encode):
>+        (JSC::CachedCompactVariableMapHandle::decode const):
>+        (JSC::CachedFunctionExecutable::encode):
>+        (JSC::CachedFunctionExecutable::decode const):
>+        (JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable):
>+
> 2019-02-21  Saam Barati  <sbarati@apple.com>
> 
>         Update JSScript SPI based on feedback
>diff --git a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>index defd2f08b87181e6b7f4ff60156907fdb6eb397f..b0c305e2f5b9d0b338581abd69f08de87753fe04 100644
>--- a/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>+++ b/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>@@ -191,7 +191,7 @@ public:
> 
> private:
>     UnlinkedFunctionExecutable(VM*, Structure*, const SourceCode&, FunctionMetadataNode*, UnlinkedFunctionKind, ConstructAbility, JSParserScriptMode, CompactVariableMap::Handle,  JSC::DerivedContextType, bool isBuiltinDefaultClassConstructor);
>-    UnlinkedFunctionExecutable(Decoder&, VariableEnvironment&, const CachedFunctionExecutable&);
>+    UnlinkedFunctionExecutable(Decoder&, CompactVariableMap::Handle, const CachedFunctionExecutable&);
> 
>     unsigned m_firstLineOffset;
>     unsigned m_lineCount;
>diff --git a/Source/JavaScriptCore/parser/VariableEnvironment.cpp b/Source/JavaScriptCore/parser/VariableEnvironment.cpp
>index a93ae7adbdfe6686c93b7fc1850737d9e1f0f1bb..faddd14302d1715ed581fbec5d58b74065bd8a6a 100644
>--- a/Source/JavaScriptCore/parser/VariableEnvironment.cpp
>+++ b/Source/JavaScriptCore/parser/VariableEnvironment.cpp
>@@ -154,12 +154,21 @@ VariableEnvironment CompactVariableEnvironment::toVariableEnvironment() const
> CompactVariableMap::Handle CompactVariableMap::get(const VariableEnvironment& env)
> {
>     auto* environment = new CompactVariableEnvironment(env);
>+    bool isNewEntry;
>+    auto handle = get(environment, isNewEntry);
>+    if (!isNewEntry)
>+        delete environment;
>+    return handle;
>+}
>+
>+CompactVariableMap::Handle CompactVariableMap::get(CompactVariableEnvironment* environment, bool& isNewEntry)
>+{
>     CompactVariableMapKey key { *environment };
>     auto addResult = m_map.add(key, 1);
>+    isNewEntry = addResult.isNewEntry;
>     if (addResult.isNewEntry)
>         return CompactVariableMap::Handle(*environment, *this);
> 
>-    delete environment;
>     ++addResult.iterator->value;
>     return CompactVariableMap::Handle(addResult.iterator->key.environment(), *this);
> }
>diff --git a/Source/JavaScriptCore/parser/VariableEnvironment.h b/Source/JavaScriptCore/parser/VariableEnvironment.h
>index ee6cce6e786ca56aa00081d095f0264bc3cbee5a..9f90450480376ed294827154bc3a515b914ab74a 100644
>--- a/Source/JavaScriptCore/parser/VariableEnvironment.h
>+++ b/Source/JavaScriptCore/parser/VariableEnvironment.h
>@@ -125,6 +125,9 @@ private:
> class CompactVariableEnvironment {
>     WTF_MAKE_FAST_ALLOCATED;
>     WTF_MAKE_NONCOPYABLE(CompactVariableEnvironment);
>+
>+    friend class CachedCompactVariableEnvironment;
>+
> public:
>     CompactVariableEnvironment(const VariableEnvironment&);
>     VariableEnvironment toVariableEnvironment() const;
>@@ -133,6 +136,8 @@ public:
>     unsigned hash() const { return m_hash; }
> 
> private:
>+    CompactVariableEnvironment() = default;
>+
>     Vector<RefPtr<UniquedStringImpl>> m_variables;
>     Vector<VariableEnvironmentEntry> m_variableMetadata;
>     unsigned m_hash;
>@@ -204,6 +209,8 @@ namespace JSC {
> class CompactVariableMap : public RefCounted<CompactVariableMap> {
> public:
>     class Handle {
>+        friend class CachedCompactVariableMapHandle;
>+
>     public:
>         Handle() = default;
> 
>@@ -241,6 +248,10 @@ public:
> 
> private:
>     friend class Handle;
>+    friend class CachedCompactVariableMapHandle;
>+
>+    Handle get(CompactVariableEnvironment*, bool& isNewEntry);
>+
>     HashMap<CompactVariableMapKey, unsigned> m_map;
> };
> 
>diff --git a/Source/JavaScriptCore/runtime/CachedTypes.cpp b/Source/JavaScriptCore/runtime/CachedTypes.cpp
>index acd95dfa9650ed01346215da43799285cf4662f8..063efa9d0eabef2d62804100d8091c64f55a14dd 100644
>--- a/Source/JavaScriptCore/runtime/CachedTypes.cpp
>+++ b/Source/JavaScriptCore/runtime/CachedTypes.cpp
>@@ -127,7 +127,7 @@ public:
>         m_ptrToOffsetMap.add(ptr, offset);
>     }
> 
>-    WTF::Optional<ptrdiff_t> offsetForPtr(const void* ptr)
>+    WTF::Optional<ptrdiff_t> cachedOffsetForPtr(const void* ptr)
>     {
>         auto it = m_ptrToOffsetMap.find(ptr);
>         if (it == m_ptrToOffsetMap.end())
>@@ -227,8 +227,8 @@ public:
> 
>     ~Decoder()
>     {
>-        for (auto& pair : m_finalizers)
>-            pair.value();
>+        for (auto& finalizer : m_finalizers)
>+            finalizer();
>     }
> 
>     VM& vm() { return m_vm; }
>@@ -245,7 +245,7 @@ public:
>         m_offsetToPtrMap.add(offset, ptr);
>     }
> 
>-    WTF::Optional<void*> ptrForOffset(ptrdiff_t offset)
>+    WTF::Optional<void*> cachedPtrForOffset(ptrdiff_t offset)
>     {
>         auto it = m_offsetToPtrMap.find(offset);
>         if (it == m_offsetToPtrMap.end())
>@@ -254,9 +254,9 @@ public:
>     }
> 
>     template<typename Functor>
>-    void addFinalizer(ptrdiff_t offset, const Functor& fn)
>+    void addFinalizer(const Functor& finalizer)
>     {
>-        m_finalizers.add(offset, fn);
>+        m_finalizers.append(finalizer);
>     }
> 
> private:
>@@ -266,7 +266,7 @@ private:
>     size_t m_size;
> #endif
>     HashMap<ptrdiff_t, void*> m_offsetToPtrMap;
>-    HashMap<ptrdiff_t, std::function<void()>> m_finalizers;
>+    Vector<std::function<void()>> m_finalizers;
> };
> 
> template<typename T>
>@@ -377,7 +377,7 @@ public:
>         if (m_isEmpty)
>             return;
> 
>-        if (WTF::Optional<ptrdiff_t> offset = encoder.offsetForPtr(src)) {
>+        if (WTF::Optional<ptrdiff_t> offset = encoder.cachedOffsetForPtr(src)) {
>             this->m_offset = *offset - encoder.offsetOf(&this->m_offset);
>             return;
>         }
>@@ -388,20 +388,32 @@ public:
>     }
> 
>     template<typename... Args>
>-    Source* decode(Decoder& decoder, Args... args) const
>+    Source* decode(Decoder& decoder, bool& isNewAllocation, Args&&... args) const
>     {
>-        if (m_isEmpty)
>+        if (m_isEmpty) {
>+            isNewAllocation = false;
>             return nullptr;
>+        }
> 
>         ptrdiff_t bufferOffset = decoder.offsetOf(this->buffer());
>-        if (WTF::Optional<void*> ptr = decoder.ptrForOffset(bufferOffset))
>+        if (WTF::Optional<void*> ptr = decoder.cachedPtrForOffset(bufferOffset)) {
>+            isNewAllocation = false;
>             return reinterpret_cast<Source*>(*ptr);
>+        }
> 
>-        Source* ptr = get()->decode(decoder, args...);
>+        isNewAllocation = true;
>+        Source* ptr = get()->decode(decoder, std::forward<Args>(args)...);
>         decoder.cacheOffset(bufferOffset, ptr);
>         return ptr;
>     }
> 
>+    template<typename... Args>
>+    Source* decode(Decoder& decoder, Args&&... args) const
>+    {
>+        bool unusedIsNewAllocation;
>+        return decode(decoder, unusedIsNewAllocation, std::forward<Args>(args)...);
>+    }
>+
>     const T* operator->() const { return get(); }
> 
> private:
>@@ -431,10 +443,15 @@ public:
> 
>     RefPtr<Source> decode(Decoder& decoder) const
>     {
>-        Source* decodedPtr = m_ptr.decode(decoder);
>+        bool isNewAllocation;
>+        Source* decodedPtr = m_ptr.decode(decoder, isNewAllocation);
>         if (!decodedPtr)
>             return nullptr;
>-        decoder.addFinalizer(decoder.offsetOf(m_ptr.buffer()), [=] { derefIfNotNull(decodedPtr); });
>+        if (isNewAllocation) {
>+            decoder.addFinalizer([=] {
>+                derefIfNotNull(decodedPtr);
>+            });
>+        }
>         refIfNotNull(decodedPtr);
>         return adoptRef(decodedPtr);
>     }
>@@ -878,6 +895,65 @@ private:
>     CachedHashMap<CachedRefPtr<CachedUniquedStringImpl>, VariableEnvironmentEntry, IdentifierRepHash, HashTraits<RefPtr<UniquedStringImpl>>, VariableEnvironmentEntryHashTraits> m_map;
> };
> 
>+class CachedCompactVariableEnvironment : public CachedObject<CompactVariableEnvironment> {
>+public:
>+    void encode(Encoder& encoder, const CompactVariableEnvironment& env)
>+    {
>+        m_variables.encode(encoder, env.m_variables);
>+        m_variableMetadata.encode(encoder, env.m_variableMetadata);
>+        m_hash = env.m_hash;
>+        m_isEverythingCaptured = env.m_isEverythingCaptured;
>+    }
>+
>+    void decode(Decoder& decoder, CompactVariableEnvironment& env) const
>+    {
>+        m_variables.decode(decoder, env.m_variables);
>+        m_variableMetadata.decode(decoder, env.m_variableMetadata);
>+        env.m_hash = m_hash;
>+        env.m_isEverythingCaptured = m_isEverythingCaptured;
>+    }
>+
>+    CompactVariableEnvironment* decode(Decoder& decoder) const
>+    {
>+        CompactVariableEnvironment* env = new CompactVariableEnvironment;
>+        decode(decoder, *env);
>+        return env;
>+    }
>+
>+private:
>+    CachedVector<CachedRefPtr<CachedUniquedStringImpl>> m_variables;
>+    CachedVector<VariableEnvironmentEntry> m_variableMetadata;
>+    unsigned m_hash;
>+    bool m_isEverythingCaptured;
>+};
>+
>+class CachedCompactVariableMapHandle : public CachedObject<CompactVariableMap::Handle> {
>+public:
>+    void encode(Encoder& encoder, const CompactVariableMap::Handle& handle)
>+    {
>+        m_environment.encode(encoder, handle.m_environment);
>+    }
>+
>+    CompactVariableMap::Handle decode(Decoder& decoder) const
>+    {
>+        bool isNewAllocation;
>+        CompactVariableEnvironment* environment = m_environment.decode(decoder, isNewAllocation);
>+        bool isNewEntry;
>+        CompactVariableMap::Handle handle = decoder.vm().m_compactVariableMap->get(environment, isNewEntry);
>+        if (!isNewAllocation)
>+            ASSERT(!isNewEntry);
>+        else if (!isNewEntry) {
>+            decoder.addFinalizer([=] {
>+                delete environment;
>+            });
>+        }
>+        return handle;
>+    }
>+
>+private:
>+    CachedPtr<CachedCompactVariableEnvironment> m_environment;
>+};
>+
> template<typename T, typename Source = SourceType<T>>
> class CachedArray : public VariableLengthObject<Source*> {
> public:
>@@ -1517,7 +1593,7 @@ private:
>     CachedIdentifier m_ecmaName;
>     CachedIdentifier m_inferredName;
> 
>-    CachedVariableEnvironment m_parentScopeTDZVariables;
>+    CachedCompactVariableMapHandle m_parentScopeTDZVariables;
> 
>     CachedWriteBarrier<CachedFunctionCodeBlock, UnlinkedFunctionCodeBlock> m_unlinkedCodeBlockForCall;
>     CachedWriteBarrier<CachedFunctionCodeBlock, UnlinkedFunctionCodeBlock> m_unlinkedCodeBlockForConstruct;
>@@ -1852,7 +1928,7 @@ ALWAYS_INLINE void CachedFunctionExecutable::encode(Encoder& encoder, const Unli
>     m_ecmaName.encode(encoder, executable.ecmaName());
>     m_inferredName.encode(encoder, executable.inferredName());
> 
>-    m_parentScopeTDZVariables.encode(encoder, executable.parentScopeTDZVariables());
>+    m_parentScopeTDZVariables.encode(encoder, executable.m_parentScopeTDZVariables);
> 
>     m_unlinkedCodeBlockForCall.encode(encoder, executable.m_unlinkedCodeBlockForCall);
>     m_unlinkedCodeBlockForConstruct.encode(encoder, executable.m_unlinkedCodeBlockForConstruct);
>@@ -1860,10 +1936,8 @@ ALWAYS_INLINE void CachedFunctionExecutable::encode(Encoder& encoder, const Unli
> 
> ALWAYS_INLINE UnlinkedFunctionExecutable* CachedFunctionExecutable::decode(Decoder& decoder) const
> {
>-    VariableEnvironment env;
>-    m_parentScopeTDZVariables.decode(decoder, env);
>-
>-    UnlinkedFunctionExecutable* executable = new (NotNull, allocateCell<UnlinkedFunctionExecutable>(decoder.vm().heap)) UnlinkedFunctionExecutable(decoder, env, *this);
>+    CompactVariableMap::Handle env = m_parentScopeTDZVariables.decode(decoder);
>+    UnlinkedFunctionExecutable* executable = new (NotNull, allocateCell<UnlinkedFunctionExecutable>(decoder.vm().heap)) UnlinkedFunctionExecutable(decoder, WTFMove(env), *this);
>     executable->finishCreation(decoder.vm());
> 
>     m_unlinkedCodeBlockForCall.decode(decoder, executable->m_unlinkedCodeBlockForCall, executable);
>@@ -1872,7 +1946,7 @@ ALWAYS_INLINE UnlinkedFunctionExecutable* CachedFunctionExecutable::decode(Decod
>     return executable;
> }
> 
>-ALWAYS_INLINE UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(Decoder& decoder, VariableEnvironment& parentScopeTDZVariables, const CachedFunctionExecutable& cachedExecutable)
>+ALWAYS_INLINE UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(Decoder& decoder, CompactVariableMap::Handle parentScopeTDZVariables, const CachedFunctionExecutable& cachedExecutable)
>     : Base(decoder.vm(), decoder.vm().unlinkedFunctionExecutableStructure.get())
>     , m_firstLineOffset(cachedExecutable.firstLineOffset())
>     , m_lineCount(cachedExecutable.lineCount())
>@@ -1902,7 +1976,7 @@ ALWAYS_INLINE UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(Decoder& de
>     , m_ecmaName(cachedExecutable.ecmaName(decoder))
>     , m_inferredName(cachedExecutable.inferredName(decoder))
> 
>-    , m_parentScopeTDZVariables(decoder.vm().m_compactVariableMap->get(parentScopeTDZVariables))
>+    , m_parentScopeTDZVariables(WTFMove(parentScopeTDZVariables))
> 
>     , m_rareData(cachedExecutable.rareData(decoder))
> {

Actions: View | Formatted Diff | Diff

Attachments on bug 194706: 362118 | 362435 | 362485 | 362498 | 362700 | 362702