Attachment 369228 Details for Bug 197645 – Patch

[patch] Patch

bug-197645-20190506214502.patch (text/plain), 15.62 KB, created by Yusuke Suzuki on 2019-05-06 21:45:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-06 21:45:03 PDT

Size: 15.62 KB

patch

obsolete

>Subversion Revision: 244999
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index a8db599525f650cc60a9d914af7a2f63448960c1..c2538f64e0dfc90c67d42465d8f38763cadfdade 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>
>+        https://bugs.webkit.org/show_bug.cgi?id=197645
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We are using HashMap<std::tuple<Structure*, const Instruction*>, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> for LLIntPrototypeLoadAdaptiveStructureWatchpoint,
>+        but this has several memory inefficiency.
>+
>+        1. Structure* and Instruction* are too large. We can just use StructureID and bytecodeOffsert (unsigned).
>+        2. While we are using Bag<>, we do not add a new LLIntPrototypeLoadAdaptiveStructureWatchpoint after constructing this Bag first. So we can
>+           use Vector<LLIntPrototypeLoadAdaptiveStructureWatchpoint> instead. We ensure that new entry won't be added to this Vector by making Watchpoint
>+           non-movable.
>+        3. Instead of having OpGetById::Metadata&, we just hold `unsigned` bytecodeOffset, and get Metadata& from the owner CodeBlock when needed.
>+
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::finalizeLLIntInlineCaches):
>+        * bytecode/CodeBlock.h:
>+        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
>+        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::LLIntPrototypeLoadAdaptiveStructureWatchpoint):
>+        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
>+        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h:
>+        * bytecode/Watchpoint.h:
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::setupGetByIdPrototypeCache):
>+
> 2019-05-06  Keith Miller  <keith_miller@apple.com>
> 
>         JSWrapperMap should check if existing prototype properties are wrappers when copying exported methods.
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index c6d49c940bb4441967e375afd1995bdb9b4653e6..6cb789f00cd278c89932c1b7fcc5675ef3ee0c6b 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] LLIntPrototypeLoadAdaptiveStructureWatchpoint does not require Bag<>
>+        https://bugs.webkit.org/show_bug.cgi?id=197645
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * WTF.xcodeproj/project.pbxproj:
>+        * wtf/CMakeLists.txt:
>+        * wtf/Nonmovable.h: Copied from Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h.
>+        * wtf/Vector.h:
>+        (WTF::minCapacity>::uncheckedConstructAndAppend):
>+
> 2019-05-06  Christopher Reid  <chris.reid@sony.com>
> 
>         [JSC] Respect already defined USE_LLINT_EMBEDDED_OPCODE_ID compiler variable.
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index 46f3ed72ac1193594622d59ab53b1bd49d0b7536..db28f67e2d17b30395500484e86945a7ce1e9d8a 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -1302,7 +1302,7 @@ void CodeBlock::finalizeLLIntInlineCaches()
>     // then cleared the cache without GCing in between.
>     m_llintGetByIdWatchpointMap.removeIf([&] (const StructureWatchpointMap::KeyValuePairType& pair) -> bool {
>         auto clear = [&] () {
>-            const Instruction* instruction = std::get<1>(pair.key);
>+            auto& instruction = instructions().at(std::get<1>(pair.key));
>             OpcodeID opcode = instruction->opcodeID();
>             if (opcode == op_get_by_id) {
>                 if (Options::verboseOSR())
>@@ -1312,11 +1312,11 @@ void CodeBlock::finalizeLLIntInlineCaches()
>             return true;
>         };
> 
>-        if (!vm.heap.isMarked(std::get<0>(pair.key)))
>+        if (!vm.heap.isMarked(vm.heap.structureIDTable().get(std::get<0>(pair.key))))
>             return clear();
> 
>-        for (const LLIntPrototypeLoadAdaptiveStructureWatchpoint* watchpoint : pair.value) {
>-            if (!watchpoint->key().isStillLive(vm))
>+        for (const LLIntPrototypeLoadAdaptiveStructureWatchpoint& watchpoint : pair.value) {
>+            if (!watchpoint.key().isStillLive(vm))
>                 return clear();
>         }
> 
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h
>index 406fca08a123df10d70f69e1368bfc32224b6626..a4c23bf8688111242918ba372deeccae739e8889 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.h
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h
>@@ -637,7 +637,7 @@ class CodeBlock : public JSCell {
>         return m_llintExecuteCounter;
>     }
> 
>-    typedef HashMap<std::tuple<Structure*, const Instruction*>, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
>+    typedef HashMap<std::tuple<StructureID, unsigned>, Vector<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
>     StructureWatchpointMap& llintGetByIdWatchpointMap() { return m_llintGetByIdWatchpointMap; }
> 
>     // Functions for controlling when tiered compilation kicks in. This
>diff --git a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
>index 1bebc48c0474f9550e7df2363f7d1cb8b6f67dd5..086b513b26e7d9ac7a9879505279b62fbf2bd4a9 100644
>--- a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
>+++ b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
>@@ -32,10 +32,10 @@
> 
> namespace JSC {
> 
>-LLIntPrototypeLoadAdaptiveStructureWatchpoint::LLIntPrototypeLoadAdaptiveStructureWatchpoint(CodeBlock* owner, const ObjectPropertyCondition& key, OpGetById::Metadata& getByIdMetadata)
>+LLIntPrototypeLoadAdaptiveStructureWatchpoint::LLIntPrototypeLoadAdaptiveStructureWatchpoint(CodeBlock* owner, const ObjectPropertyCondition& key, unsigned bytecodeOffset)
>     : m_owner(owner)
>     , m_key(key)
>-    , m_getByIdMetadata(getByIdMetadata)
>+    , m_bytecodeOffset(bytecodeOffset)
> {
>     RELEASE_ASSERT(key.watchingRequiresStructureTransitionWatchpoint());
>     RELEASE_ASSERT(!key.watchingRequiresReplacementWatchpoint());
>@@ -58,7 +58,8 @@ void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(VM& vm, const F
>         return;
>     }
> 
>-    clearLLIntGetByIdCache(m_getByIdMetadata);
>+    auto& instruction = m_owner->instructions().at(m_bytecodeOffset);
>+    clearLLIntGetByIdCache(instruction->as<OpGetById>().metadata(m_owner));
> }
> 
> void LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(OpGetById::Metadata& metadata)
>diff --git a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>index 9e11808f3f48674f0e1af89a11373286aa2223e6..a9b1fcd2587ae86270923e44e613b6775d5a8ff2 100644
>--- a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>+++ b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>@@ -33,7 +33,7 @@ namespace JSC {
> 
> class LLIntPrototypeLoadAdaptiveStructureWatchpoint final : public Watchpoint {
> public:
>-    LLIntPrototypeLoadAdaptiveStructureWatchpoint(CodeBlock*, const ObjectPropertyCondition&, OpGetById::Metadata&);
>+    LLIntPrototypeLoadAdaptiveStructureWatchpoint(CodeBlock*, const ObjectPropertyCondition&, unsigned bytecodeOffset);
> 
>     void install(VM&);
> 
>@@ -47,7 +47,7 @@ class LLIntPrototypeLoadAdaptiveStructureWatchpoint final : public Watchpoint {
> private:
>     CodeBlock* m_owner;
>     ObjectPropertyCondition m_key;
>-    OpGetById::Metadata& m_getByIdMetadata;
>+    unsigned m_bytecodeOffset;
> };
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/bytecode/Watchpoint.h b/Source/JavaScriptCore/bytecode/Watchpoint.h
>index 3720cc71ffd9c790b22fb3364aecd66ad7669eaa..77fba3efa9a5a45791b2b3ece232bf1d221a706b 100644
>--- a/Source/JavaScriptCore/bytecode/Watchpoint.h
>+++ b/Source/JavaScriptCore/bytecode/Watchpoint.h
>@@ -28,6 +28,7 @@
> #include <wtf/Atomics.h>
> #include <wtf/FastMalloc.h>
> #include <wtf/Noncopyable.h>
>+#include <wtf/Nonmovable.h>
> #include <wtf/PrintStream.h>
> #include <wtf/ScopedLambda.h>
> #include <wtf/SentinelLinkedList.h>
>@@ -91,6 +92,7 @@ class WatchpointSet;
> 
> class Watchpoint : public BasicRawSentinelNode<Watchpoint> {
>     WTF_MAKE_NONCOPYABLE(Watchpoint);
>+    WTF_MAKE_NONMOVABLE(Watchpoint);
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>     Watchpoint() = default;
>diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>index 7c832954dfa04e146617609544eecb7d96759de4..2e90c701da1ec75b1822aa6d7b3be7c7426f25b7 100644
>--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>@@ -719,19 +719,22 @@ static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, const Instructio
>     if (!conditions.isValid())
>         return;
> 
>+    unsigned bytecodeOffset = codeBlock->bytecodeOffset(pc);
>     PropertyOffset offset = invalidOffset;
>     CodeBlock::StructureWatchpointMap& watchpointMap = codeBlock->llintGetByIdWatchpointMap();
>-    Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint> watchpoints;
>+    Vector<LLIntPrototypeLoadAdaptiveStructureWatchpoint> watchpoints;
>+    watchpoints.reserveInitialCapacity(conditions.size());
>     for (ObjectPropertyCondition condition : conditions) {
>         if (!condition.isWatchable())
>             return;
>         if (condition.condition().kind() == PropertyCondition::Presence)
>             offset = condition.condition().offset();
>-        watchpoints.add(codeBlock, condition, metadata)->install(vm);
>+        watchpoints.uncheckedConstructAndAppend(codeBlock, condition, bytecodeOffset);
>+        watchpoints.last().install(vm);
>     }
> 
>     ASSERT((offset == invalidOffset) == slot.isUnset());
>-    auto result = watchpointMap.add(std::make_tuple(structure, pc), WTFMove(watchpoints));
>+    auto result = watchpointMap.add(std::make_tuple(structure->id(), bytecodeOffset), WTFMove(watchpoints));
>     ASSERT_UNUSED(result, result.isNewEntry);
> 
>     ConcurrentJSLocker locker(codeBlock->m_lock);
>diff --git a/Source/WTF/WTF.xcodeproj/project.pbxproj b/Source/WTF/WTF.xcodeproj/project.pbxproj
>index 89c91b096d1c32a6796c487d38cdc165588bc8be..0ba9840394e7a50810e9aca9b03e2658a120f814 100644
>--- a/Source/WTF/WTF.xcodeproj/project.pbxproj
>+++ b/Source/WTF/WTF.xcodeproj/project.pbxproj
>@@ -669,6 +669,7 @@
> 		E3A32BC31FC830E2007D7E76 /* JSValueMalloc.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSValueMalloc.h; sourceTree = "<group>"; };
> 		E3CF76902115D6BA0091DE48 /* CompactPointerTuple.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompactPointerTuple.h; sourceTree = "<group>"; };
> 		E3E158251EADA53C004A079D /* SystemFree.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SystemFree.h; sourceTree = "<group>"; };
>+		E3E64F0B22813428001E55B4 /* Nonmovable.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = Nonmovable.h; sourceTree = "<group>"; };
> 		E431CC4A21187ADB000C8A07 /* DispatchSPI.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DispatchSPI.h; sourceTree = "<group>"; };
> 		E4A0AD371A96245500536DF6 /* WorkQueue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WorkQueue.cpp; sourceTree = "<group>"; };
> 		E4A0AD381A96245500536DF6 /* WorkQueue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WorkQueue.h; sourceTree = "<group>"; };
>@@ -1039,6 +1040,7 @@
> 				1A3F6BE6174ADA2100B2EEA7 /* NeverDestroyed.h */,
> 				0F0D85B317234CB100338210 /* NoLock.h */,
> 				A8A472D0151A825B004123FF /* Noncopyable.h */,
>+				E3E64F0B22813428001E55B4 /* Nonmovable.h */,
> 				526AEC911F6B4E5C00695F5D /* NoTailCalls.h */,
> 				7CEAE5AC1EA6E10F00DB6890 /* NotFound.h */,
> 				A8A472D5151A825B004123FF /* NumberOfCores.cpp */,
>diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt
>index 5a8bb51b94ac9f5ec35ef844d97eca202fb4f47b..1990ca94916b3fa93220e917eab1b5b830ebc209 100644
>--- a/Source/WTF/wtf/CMakeLists.txt
>+++ b/Source/WTF/wtf/CMakeLists.txt
>@@ -139,6 +139,7 @@ set(WTF_PUBLIC_HEADERS
>     NoLock.h
>     NoTailCalls.h
>     Noncopyable.h
>+    Nonmovable.h
>     NotFound.h
>     NumberOfCores.h
>     OSAllocator.h
>diff --git a/Source/WTF/wtf/Nonmovable.h b/Source/WTF/wtf/Nonmovable.h
>new file mode 100644
>index 0000000000000000000000000000000000000000..4c1034b2b72f6508bda1b06877a1747c7b500d1d
>--- /dev/null
>+++ b/Source/WTF/wtf/Nonmovable.h
>@@ -0,0 +1,32 @@
>+/*
>+ * Copyright (C) 2019 Apple Inc. All Rights Reserved.
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *    notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *    notice, this list of conditions and the following disclaimer in the
>+ *    documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+ */
>+
>+#pragma once
>+
>+#define WTF_MAKE_NONMOVABLE(ClassName) \
>+    private: \
>+        ClassName(ClassName&&) = delete; \
>+        ClassName& operator=(ClassName&&) = delete; \
>+
>diff --git a/Source/WTF/wtf/Vector.h b/Source/WTF/wtf/Vector.h
>index 05ff1a82e0403950a316690dc2ecd2353005876e..e4e1b6288541157a24bdb3b06b88ab565c94e412 100644
>--- a/Source/WTF/wtf/Vector.h
>+++ b/Source/WTF/wtf/Vector.h
>@@ -775,6 +775,7 @@ class Vector : private VectorBuffer<T, inlineCapacity> {
> 
>     void uncheckedAppend(ValueType&& value) { uncheckedAppend<ValueType>(std::forward<ValueType>(value)); }
>     template<typename U> void uncheckedAppend(U&&);
>+    template<typename... Args> void uncheckedConstructAndAppend(Args&&...);
> 
>     template<typename U> void append(const U*, size_t);
>     template<typename U, size_t otherCapacity> void appendVector(const Vector<U, otherCapacity>&);
>@@ -1391,6 +1392,18 @@ ALWAYS_INLINE void Vector<T, inlineCapacity, OverflowHandler, minCapacity>::unch
>     ++m_size;
> }
> 
>+template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t minCapacity>
>+template<typename... Args>
>+ALWAYS_INLINE void Vector<T, inlineCapacity, OverflowHandler, minCapacity>::uncheckedConstructAndAppend(Args&&... args)
>+{
>+    ASSERT(size() < capacity());
>+
>+    asanBufferSizeWillChangeTo(m_size + 1);
>+
>+    new (NotNull, end()) T(std::forward<Args>(args)...);
>+    ++m_size;
>+}
>+
> template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t minCapacity>
> template<typename U, size_t otherCapacity>
> inline void Vector<T, inlineCapacity, OverflowHandler, minCapacity>::appendVector(const Vector<U, otherCapacity>& val)

Flags:

saam: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 197645: 369228 | 369258