Cache CompactVariableMap::Handle instead of VariableEnvironment for UnlinkedFunctionE...
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 09:08:35 +0000 (09:08 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 09:08:35 +0000 (09:08 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241938 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
Source/JavaScriptCore/parser/VariableEnvironment.cpp
Source/JavaScriptCore/parser/VariableEnvironment.h
Source/JavaScriptCore/runtime/CachedTypes.cpp

index d72d851..fc61627 100644 (file)
@@ -1,3 +1,31 @@
+2019-02-22  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
index defd2f0..b0c305e 100644 (file)
@@ -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;
index a93ae7a..faddd14 100644 (file)
@@ -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);
 }
index ee6cce6..9f90450 100644 (file)
@@ -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;
 };
 
index acd95df..063efa9 100644 (file)
@@ -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))
 {