Changeset 232313 in webkit

Timestamp:

May 30, 2018 4:07:16 PM (6 years ago)

Author:

keith_miller@apple.com

Message:

LLInt get_by_id prototype caching doesn't properly handle changes
​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.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/llint-proto-get-by-id-cache-change-prototype.js (added)
• JSTests/stress/llint-proto-get-by-id-cache-intercept-value.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/CodeBlock.cpp (modified) (3 diffs)
• Source/JavaScriptCore/bytecode/CodeBlock.h (modified) (1 diff)
• Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h (modified) (2 diffs)
• Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h (modified) (1 diff)
• Source/JavaScriptCore/bytecode/Watchpoint.h (modified) (1 diff)
• Source/JavaScriptCore/llint/LLIntSlowPaths.cpp (modified) (2 diffs)
• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/Bag.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -1260 +1260 @@
         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)))
@@ -1301 +1299 @@
         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;
@@ -1358 +1358 @@
     // 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;
     });
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -628 +628 @@
     }
 
-    typedef HashMap<Structure*, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
+    typedef HashMap<std::tuple<Structure*, Instruction*>, Bag<LLIntPrototypeLoadAdaptiveStructureWatchpoint>> StructureWatchpointMap;
     StructureWatchpointMap& llintGetByIdWatchpointMap() { return m_llintGetByIdWatchpointMap; }
 

trunk/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.h

@@ -34 +34 @@
 class LLIntPrototypeLoadAdaptiveStructureWatchpoint : public Watchpoint {
 public:
+    LLIntPrototypeLoadAdaptiveStructureWatchpoint() = default;
     LLIntPrototypeLoadAdaptiveStructureWatchpoint(const ObjectPropertyCondition&, Instruction*);
 
     void install();
+
+    const ObjectPropertyCondition& key() const { return m_key; }
 
 protected:
@@ -43 +46 @@
 private:
     ObjectPropertyCondition m_key;
-    Instruction* m_getByIdInstruction;
+    Instruction* m_getByIdInstruction { nullptr };
 };
 

trunk/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h

@@ -68 +68 @@
 
     bool isValidAndWatchable() const;
-    
+
+    size_t size() const { return m_data ? m_data->vector.size() : 0; }
     bool isEmpty() const
     {

trunk/Source/JavaScriptCore/bytecode/Watchpoint.h

@@ -69 +69 @@
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    Watchpoint()
-    {
-    }
+    Watchpoint() = default;
     
     virtual ~Watchpoint();

trunk/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

@@ -671 +671 @@
     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())
@@ -677 +677 @@
         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);

trunk/Source/WTF/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WTF/wtf/Bag.h

@@ -139 +139 @@
         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 };