Attachment 341607 Details for Bug 186112 – Patch

[patch] Patch

bug-186112-20180530150921.patch (text/plain), 11.77 KB, created by Keith Miller on 2018-05-30 15:09:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2018-05-30 15:09:22 PDT

Size: 11.77 KB

patch

obsolete

>Subversion Revision: 232138
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 28a319a8b2dbf4cc82616e79b811be97a013974b..3aff857c6905a1b628f52ad2dd7a28b377adb188 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-05-30  Keith Miller  <keith_miller@apple.com>
>+
>+        LLInt get_by_id prototype caching doesn't properly handle changes
>+        https://bugs.webkit.org/show_bug.cgi?id=186112
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The caching would sometimes fail to track that a prototype had changed
>+        and wouldn't update its set of watchpoints.
>+
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::finalizeLLIntInlineCaches):
>+        * bytecode/CodeBlock.h:
>+        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h:
>+        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::key const):
>+        * bytecode/ObjectPropertyConditionSet.h:
>+        (JSC::ObjectPropertyConditionSet::size const):
>+        * bytecode/Watchpoint.h:
>+        (JSC::Watchpoint::Watchpoint): Deleted.
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::setupGetByIdPrototypeCache):
>+
> 2018-05-23  Keith Miller  <keith_miller@apple.com>
> 
>         Define length on CoW array should properly convert to writable
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 7bae582c6b028421e0c674dd929ba70cafb7230a..58a014c90a7f3440998fb70acaf5fa16164f43a0 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-05-30  Keith Miller  <keith_miller@apple.com>
>+
>+        LLInt get_by_id prototype caching doesn't properly handle changes
>+        https://bugs.webkit.org/show_bug.cgi?id=186112
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Mark some methods const.
>+
>+        * wtf/Bag.h:
>+        (WTF::Bag::begin const):
>+        (WTF::Bag::end const):
>+        (WTF::Bag::unwrappedHead const):
>+        (WTF::Bag::end): Deleted.
>+        (WTF::Bag::unwrappedHead): Deleted.
>+
> 2018-05-23  Eric Carlson  <eric.carlson@apple.com>
> 
>         Avoid loading AVFoundation to check supported MIME types if possible
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index 8bd7fe7af83de12d9e19d2df40dbc3c474d1c37c..fc5783a799e79073326e2e3563d1368bda5315e0 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -1259,9 +1259,7 @@ void CodeBlock::finalizeLLIntInlineCaches()
>     for (size_t size = propertyAccessInstructions.size(), i = 0; i < size; ++i) {
>         Instruction* curInstruction = &instructions()[propertyAccessInstructions[i]];
>         switch (Interpreter::getOpcodeID(curInstruction[0])) {
>-        case op_get_by_id:
>-        case op_get_by_id_proto_load:
>-        case op_get_by_id_unset: {
>+        case op_get_by_id: {
>             StructureID oldStructureID = curInstruction[4].u.structureID;
>             if (!oldStructureID || Heap::isMarked(vm.heap.structureIDTable().get(oldStructureID)))
>                 break;
>@@ -1300,6 +1298,8 @@ void CodeBlock::finalizeLLIntInlineCaches()
>         // We need to add optimizations for op_resolve_scope_for_hoisting_func_decl_in_eval to do link time scope resolution.
>         case op_resolve_scope_for_hoisting_func_decl_in_eval:
>             break;
>+        case op_get_by_id_proto_load:
>+        case op_get_by_id_unset:
>         case op_get_array_length:
>             break;
>         case op_to_this:
>@@ -1357,8 +1357,27 @@ void CodeBlock::finalizeLLIntInlineCaches()
> 
>     // We can't just remove all the sets when we clear the caches since we might have created a watchpoint set
>     // then cleared the cache without GCing in between.
>-    m_llintGetByIdWatchpointMap.removeIf([](const StructureWatchpointMap::KeyValuePairType& pair) -> bool {
>-        return !Heap::isMarked(pair.key);
>+    m_llintGetByIdWatchpointMap.removeIf([&] (const StructureWatchpointMap::KeyValuePairType& pair) -> bool {
>+        auto clear = [&] () {
>+            Instruction* instruction = std::get<1>(pair.key);
>+            OpcodeID opcode = Interpreter::getOpcodeID(*instruction);
>+            if (opcode == op_get_by_id_proto_load || opcode == op_get_by_id_unset) {
>+                if (Options::verboseOSR())
>+                    dataLogF("Clearing LLInt property access.\n");
>+                clearLLIntGetByIdCache(instruction);
>+            }
>+            return true;
>+        };
>+
>+        if (!Heap::isMarked(std::get<0>(pair.key)))
>+            return clear();
>+
>+        for (const LLIntPrototypeLoadAdaptiveStructureWatchpoint* watchpoint : pair.value) {
>+            if (!watchpoint->key().isStillLive())
>+                return clear();
>+        }
>+
>+        return false;
>     });
> 
>     for (unsigned i = 0; i < m_llintCallLinkInfos.size(); ++i) {
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h
>index fded8ea4b65cb05f1484d0996cc4d889b6bf13a2..0d4ed3af372ed46164d28916a836d385a2622a71 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.h
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h
>@@ -627,7 +627,7 @@ public:
>         return m_llintExecuteCounter;
>     }
> 
>-    typedef HashMap<Structure*, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
>+    typedef HashMap<std::tuple<Structure*, Instruction*>, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
>     StructureWatchpointMap& llintGetByIdWatchpointMap() { return m_llintGetByIdWatchpointMap; }
> 
>     // Functions for controlling when tiered compilation kicks in. This
>diff --git a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>index 8a73c6c79d7fd354658ec6a39caa40d1a043058e..66468f0ef845bc3b7f6549d0eddb35038d358eb0 100644
>--- a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>+++ b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
>@@ -33,16 +33,19 @@ namespace JSC {
> 
> class LLIntPrototypeLoadAdaptiveStructureWatchpoint : public Watchpoint {
> public:
>+    LLIntPrototypeLoadAdaptiveStructureWatchpoint() = default;
>     LLIntPrototypeLoadAdaptiveStructureWatchpoint(const ObjectPropertyCondition&, Instruction*);
> 
>     void install();
> 
>+    const ObjectPropertyCondition& key() const { return m_key; }
>+
> protected:
>     void fireInternal(const FireDetail&) override;
> 
> private:
>     ObjectPropertyCondition m_key;
>-    Instruction* m_getByIdInstruction;
>+    Instruction* m_getByIdInstruction { nullptr };
> };
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
>index 493e20c65211c2e0bf5f02ced21b9f73c1b76706..8e53a1b27b4723b314acc17055c78d14ad193176 100644
>--- a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
>+++ b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
>@@ -67,7 +67,8 @@ public:
>     }
> 
>     bool isValidAndWatchable() const;
>-    
>+
>+    size_t size() const { return m_data ? m_data->vector.size() : 0; }
>     bool isEmpty() const
>     {
>         return !m_data;
>diff --git a/Source/JavaScriptCore/bytecode/Watchpoint.h b/Source/JavaScriptCore/bytecode/Watchpoint.h
>index 4b373496fb97a70893284a02fe468deacd192888..4476e2a7b5135ea7dbcf2ec688c7397b72900e01 100644
>--- a/Source/JavaScriptCore/bytecode/Watchpoint.h
>+++ b/Source/JavaScriptCore/bytecode/Watchpoint.h
>@@ -68,9 +68,7 @@ class Watchpoint : public BasicRawSentinelNode<Watchpoint> {
>     WTF_MAKE_NONCOPYABLE(Watchpoint);
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    Watchpoint()
>-    {
>-    }
>+    Watchpoint() = default;
>     
>     virtual ~Watchpoint();
> 
>diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>index 5ccdcfe4146496b9aebe51010a977ca276e1da68..21212587296c57feb50617e71e84db949030ea9e 100644
>--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>@@ -670,15 +670,18 @@ static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, Instruction* pc,
> 
>     PropertyOffset offset = invalidOffset;
>     CodeBlock::StructureWatchpointMap& watchpointMap = codeBlock->llintGetByIdWatchpointMap();
>-    auto result = watchpointMap.add(structure, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>());
>+    Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint> watchpoints;
>     for (ObjectPropertyCondition condition : conditions) {
>         if (!condition.isWatchable())
>             return;
>         if (condition.condition().kind() == PropertyCondition::Presence)
>             offset = condition.condition().offset();
>-        result.iterator->value.add(condition, pc)->install();
>+        watchpoints.add(condition, pc)->install();
>     }
>+
>     ASSERT((offset == invalidOffset) == slot.isUnset());
>+    auto result = watchpointMap.add(std::make_tuple(structure, pc), WTFMove(watchpoints));
>+    ASSERT_UNUSED(result, result.isNewEntry);
> 
>     ConcurrentJSLocker locker(codeBlock->m_lock);
> 
>diff --git a/Source/WTF/wtf/Bag.h b/Source/WTF/wtf/Bag.h
>index feafbaa6eccc19ff6efe7d9876b7e85928eddc87..f66eed38d18cba59fb4b76f02ccf1c23e4571f46 100644
>--- a/Source/WTF/wtf/Bag.h
>+++ b/Source/WTF/wtf/Bag.h
>@@ -138,13 +138,21 @@ public:
>         result.m_node = unwrappedHead();
>         return result;
>     }
>-    
>-    iterator end() { return iterator(); }
>+
>+    const iterator begin() const
>+    {
>+        iterator result;
>+        result.m_node = unwrappedHead();
>+        return result;
>+    }
>+
>+
>+    iterator end() const { return iterator(); }
>     
>     bool isEmpty() const { return !m_head; }
>     
> private:
>-    Node* unwrappedHead() { return PtrTraits::unwrap(m_head); }
>+    Node* unwrappedHead() const { return PtrTraits::unwrap(m_head); }
> 
>     typename PtrTraits::StorageType m_head { nullptr };
> };
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 645793478a2711adddb7914f5a4a37b634c4ec0f..4ecd7927e07169454e797f3948beb1d45d2aa20e 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-05-30  Keith Miller  <keith_miller@apple.com>
>+
>+        LLInt get_by_id prototype caching doesn't properly handle changes
>+        https://bugs.webkit.org/show_bug.cgi?id=186112
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/llint-proto-get-by-id-cache-change-prototype.js: Added.
>+        (foo):
>+        * stress/llint-proto-get-by-id-cache-intercept-value.js: Added.
>+        (foo):
>+
> 2018-05-23  Keith Miller  <keith_miller@apple.com>
> 
>         Define length on CoW array should properly convert to writable
>diff --git a/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js b/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..4f70b03f55bfcf7f670d388edad58cc124a16282
>--- /dev/null
>+++ b/JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js
>@@ -0,0 +1,19 @@
>+let p = Object.create({ foo: 1 });
>+let o = Object.create(p);
>+
>+let other = { foo: 10 };
>+
>+function foo() {
>+    return o.foo
>+}
>+
>+for (let i = 0; i < 10; i++)
>+    foo();
>+
>+p.__proto__ = null;
>+gc();
>+
>+Object.defineProperty(other, "foo", { get() { } });
>+
>+if (foo() !== undefined)
>+    throw new Error("bad get by id access");
>diff --git a/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js b/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..0b90bea4d32a21e2e1ed82d764d4b4d9049e46d1
>--- /dev/null
>+++ b/JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js
>@@ -0,0 +1,17 @@
>+let p = Object.create({ foo: 1 });
>+let o = Object.create(p);
>+
>+let other = { foo: 10 };
>+
>+function foo() {
>+    return o.foo
>+}
>+
>+for (let i = 0; i < 10; i++)
>+    foo();
>+
>+p.foo = null;
>+gc();
>+
>+if (foo() !== null)
>+    throw new Error("bad get by id access");

Actions: View | Formatted Diff | Diff

Attachments on bug 186112: 341607