LLInt get_by_id prototype caching doesn't properly handle changes
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 May 2018 23:07:16 +0000 (23:07 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 May 2018 23:07:16 +0000 (23:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186112

Reviewed by Filip Pizlo.

JSTests:

* stress/llint-proto-get-by-id-cache-change-prototype.js: Added.
(foo):
* stress/llint-proto-get-by-id-cache-intercept-value.js: Added.
(foo):

Source/JavaScriptCore:

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):

Source/WTF:

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.

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

12 files changed:
JSTests/ChangeLog
JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js [new file with mode: 0644]
JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h
Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
Source/JavaScriptCore/bytecode/Watchpoint.h
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/WTF/ChangeLog
Source/WTF/wtf/Bag.h

index 56d63ef..b8487d1 100644 (file)
@@ -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 Filip Pizlo.
+
+        * 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-30  Caio Lima  <ticaiolima@gmail.com>
 
         [ESNext][BigInt] Implement support for "%" operation
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 (file)
index 0000000..4f70b03
--- /dev/null
@@ -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 (file)
index 0000000..0b90bea
--- /dev/null
@@ -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");
index 98d605c..f4959d2 100644 (file)
@@ -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 Filip Pizlo.
+
+        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-30  Caio Lima  <ticaiolima@gmail.com>
 
         [ESNext][BigInt] Implement support for "%" operation
index 8bd7fe7..fc5783a 100644 (file)
@@ -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) {
index fded8ea..0d4ed3a 100644 (file)
@@ -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
index 8a73c6c..66468f0 100644 (file)
@@ -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
index 493e20c..8e53a1b 100644 (file)
@@ -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;
index 4b37349..4476e2a 100644 (file)
@@ -68,9 +68,7 @@ class Watchpoint : public BasicRawSentinelNode<Watchpoint> {
     WTF_MAKE_NONCOPYABLE(Watchpoint);
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    Watchpoint()
-    {
-    }
+    Watchpoint() = default;
     
     virtual ~Watchpoint();
 
index de4c134..cb86479 100644 (file)
@@ -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);
 
index a30fd68..e20f70f 100644 (file)
@@ -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 Filip Pizlo.
+
+        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-30  Alex Christensen  <achristensen@webkit.org>
 
         Reduce String allocations
index feafbaa..f66eed3 100644 (file)
@@ -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 };
 };