If Watchpoint::fire() looks at the state of the world, it should definitely see its...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Jul 2015 20:10:02 +0000 (20:10 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 13 Jul 2015 20:10:02 +0000 (20:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=146897

Reviewed by Mark Lam.

The idea is to eventually support adaptive watchpoints. An adaptive watchpoint will be
able to watch for a condition that is more fine-grained than any one watchpoint set. For
example, we might watch a singleton object to see if it ever acquires a property called
"foo". So long as it doesn't have such a property, we don't want to invalidate any code.
But if it gets that property, then we should deoptimize. Current watchpoints will
invalidate code as soon as any property is added (or deleted), because they will use the
transition watchpoint set of the singleton object's structure, and that fires anytime
there is any transition.

An adaptive watchpoint would remember the singleton object, and when it got fired, it
would check if the object's new structure has the property "foo". If not, it would check
if the object's new structure is watchable (i.e. has a valid transition watchpoint set).
If the property is missing and the structure is watchable, it would add itself to the
watchpoint set of the new structure. Otherwise, it would deoptimize.

There are two problems with this idea, and this patch fixes these problems. First, we
usually fire the transition watchpoint before we do the structure transition. This means
that if the fire() method looked at the singleton object's structure, it would see the old
structure, not the new one. It would have no way of knowing what the new structure is.
Second, inside the fire() method, the watchpoint set being invalidated still appears
valid, since we change the state after we fire all watchpoints.

This patch addresses both issues. Now, in the most important case (addPropertyTransition),
we fire the watchpoint set after we have modified the object. This is accomplished using
a deferral scope called DeferredStructureTransitionWatchpointFire. In cases where there is
no deferral, the adaptive watchpoint will conservatively resort to deoptimization because
it would find that the singleton object's structure is no longer watchable. This is
because in the absence of deferral, the singleton object would still have the original
structure, but that structure's watchpoint set would now report itself as having been
invalidated.

* bytecode/Watchpoint.cpp:
(JSC::WatchpointSet::fireAllSlow): Change the state of the set before firing all watchpoints.
(JSC::WatchpointSet::fireAllWatchpoints):
* runtime/JSObject.h:
(JSC::JSObject::putDirectInternal): Use the deferral scope.
* runtime/Structure.cpp:
(JSC::Structure::Structure): Pass the deferral scope to didTransitionFromThisStructure.
(JSC::Structure::addPropertyTransition): Pass the deferral scope to create().
(JSC::StructureFireDetail::dump): This is no longer anonymous.
(JSC::DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire): Start with a null structure.
(JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire): Fire the watchpoint if there is a structure.
(JSC::DeferredStructureTransitionWatchpointFire::add): Add a structure. Logically this is a list of deferred things, but we assert that there only will be one (happens to be true now).
(JSC::Structure::didTransitionFromThisStructure): Defer the watchpoint firing if there is a deferral scope.
* runtime/Structure.h:
(JSC::StructureFireDetail::StructureFireDetail): Move this to the header.
* runtime/StructureInlines.h:
(JSC::Structure::create): Pass the deferral scope to the constructor.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/Watchpoint.cpp
Source/JavaScriptCore/runtime/JSObject.h
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/Structure.h
Source/JavaScriptCore/runtime/StructureInlines.h

index aa70f40..9724291 100644 (file)
@@ -1,5 +1,61 @@
 2015-07-12  Filip Pizlo  <fpizlo@apple.com>
 
+        If Watchpoint::fire() looks at the state of the world, it should definitely see its set invalidated, and maybe it should see the object of interest in the transitioned-to state
+        https://bugs.webkit.org/show_bug.cgi?id=146897
+
+        Reviewed by Mark Lam.
+        
+        The idea is to eventually support adaptive watchpoints. An adaptive watchpoint will be
+        able to watch for a condition that is more fine-grained than any one watchpoint set. For
+        example, we might watch a singleton object to see if it ever acquires a property called
+        "foo". So long as it doesn't have such a property, we don't want to invalidate any code.
+        But if it gets that property, then we should deoptimize. Current watchpoints will
+        invalidate code as soon as any property is added (or deleted), because they will use the
+        transition watchpoint set of the singleton object's structure, and that fires anytime
+        there is any transition.
+        
+        An adaptive watchpoint would remember the singleton object, and when it got fired, it
+        would check if the object's new structure has the property "foo". If not, it would check
+        if the object's new structure is watchable (i.e. has a valid transition watchpoint set).
+        If the property is missing and the structure is watchable, it would add itself to the
+        watchpoint set of the new structure. Otherwise, it would deoptimize.
+        
+        There are two problems with this idea, and this patch fixes these problems. First, we
+        usually fire the transition watchpoint before we do the structure transition. This means
+        that if the fire() method looked at the singleton object's structure, it would see the old
+        structure, not the new one. It would have no way of knowing what the new structure is.
+        Second, inside the fire() method, the watchpoint set being invalidated still appears
+        valid, since we change the state after we fire all watchpoints.
+        
+        This patch addresses both issues. Now, in the most important case (addPropertyTransition),
+        we fire the watchpoint set after we have modified the object. This is accomplished using
+        a deferral scope called DeferredStructureTransitionWatchpointFire. In cases where there is
+        no deferral, the adaptive watchpoint will conservatively resort to deoptimization because
+        it would find that the singleton object's structure is no longer watchable. This is
+        because in the absence of deferral, the singleton object would still have the original
+        structure, but that structure's watchpoint set would now report itself as having been
+        invalidated.
+
+        * bytecode/Watchpoint.cpp:
+        (JSC::WatchpointSet::fireAllSlow): Change the state of the set before firing all watchpoints.
+        (JSC::WatchpointSet::fireAllWatchpoints):
+        * runtime/JSObject.h:
+        (JSC::JSObject::putDirectInternal): Use the deferral scope.
+        * runtime/Structure.cpp:
+        (JSC::Structure::Structure): Pass the deferral scope to didTransitionFromThisStructure.
+        (JSC::Structure::addPropertyTransition): Pass the deferral scope to create().
+        (JSC::StructureFireDetail::dump): This is no longer anonymous.
+        (JSC::DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire): Start with a null structure.
+        (JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire): Fire the watchpoint if there is a structure.
+        (JSC::DeferredStructureTransitionWatchpointFire::add): Add a structure. Logically this is a list of deferred things, but we assert that there only will be one (happens to be true now).
+        (JSC::Structure::didTransitionFromThisStructure): Defer the watchpoint firing if there is a deferral scope.
+        * runtime/Structure.h:
+        (JSC::StructureFireDetail::StructureFireDetail): Move this to the header.
+        * runtime/StructureInlines.h:
+        (JSC::Structure::create): Pass the deferral scope to the constructor.
+
+2015-07-12  Filip Pizlo  <fpizlo@apple.com>
+
         Watchpoints should be removed from their owning WatchpointSet before they are fired
         https://bugs.webkit.org/show_bug.cgi?id=146895
 
index 3f82d2c..761c067 100644 (file)
@@ -86,8 +86,8 @@ void WatchpointSet::fireAllSlow(const FireDetail& detail)
     ASSERT(state() == IsWatched);
     
     WTF::storeStoreFence();
+    m_state = IsInvalidated; // Do this first. Needed for adaptive watchpoints.
     fireAllWatchpoints(detail);
-    m_state = IsInvalidated;
     WTF::storeStoreFence();
 }
 
@@ -98,6 +98,10 @@ void WatchpointSet::fireAllSlow(const char* reason)
 
 void WatchpointSet::fireAllWatchpoints(const FireDetail& detail)
 {
+    // In case there are any adaptive watchpoints, we need to make sure that they see that this
+    // watchpoint has been already invalidated.
+    RELEASE_ASSERT(hasBeenInvalidated());
+    
     while (!m_set.isEmpty()) {
         Watchpoint* watchpoint = m_set.begin();
         ASSERT(watchpoint->isOnList());
index 5c9622c..d5c61ba 100644 (file)
@@ -1294,7 +1294,12 @@ inline bool JSObject::putDirectInternal(VM& vm, PropertyName propertyName, JSVal
     if ((mode == PutModePut) && !isExtensible())
         return false;
 
-    structure = Structure::addPropertyTransition(vm, structure, propertyName, attributes, offset, slot.context());
+    // We want the structure transition watchpoint to fire after this object has switched
+    // structure. This allows adaptive watchpoints to observe if the new structure is the one
+    // we want.
+    DeferredStructureTransitionWatchpointFire deferredWatchpointFire;
+    
+    structure = Structure::addPropertyTransition(vm, structure, propertyName, attributes, offset, slot.context(), &deferredWatchpointFire);
     
     validateOffset(offset);
     ASSERT(structure->isValidOffset(offset));
index 3985805..8098b2c 100644 (file)
@@ -210,7 +210,7 @@ Structure::Structure(VM& vm)
     ASSERT(hasGetterSetterProperties() || !m_classInfo->hasStaticSetterOrReadonlyProperties());
 }
 
-Structure::Structure(VM& vm, Structure* previous)
+Structure::Structure(VM& vm, Structure* previous, DeferredStructureTransitionWatchpointFire* deferred)
     : JSCell(vm, vm.structureStructure.get())
     , m_prototype(vm, this, previous->storedPrototype())
     , m_classInfo(previous->m_classInfo)
@@ -238,7 +238,7 @@ Structure::Structure(VM& vm, Structure* previous)
     ASSERT(!previous->typeInfo().structureIsImmortal());
     setPreviousID(vm, previous);
 
-    previous->didTransitionFromThisStructure();
+    previous->didTransitionFromThisStructure(deferred);
     if (previous->m_globalObject)
         m_globalObject.set(vm, this, previous->m_globalObject.get());
     ASSERT(hasReadOnlyOrGetterSetterPropertiesExcludingProto() || !m_classInfo->hasStaticSetterOrReadonlyProperties());
@@ -394,7 +394,7 @@ NonPropertyTransition Structure::suggestedArrayStorageTransition() const
     return AllocateArrayStorage;
 }
 
-Structure* Structure::addPropertyTransition(VM& vm, Structure* structure, PropertyName propertyName, unsigned attributes, PropertyOffset& offset, PutPropertySlot::Context context)
+Structure* Structure::addPropertyTransition(VM& vm, Structure* structure, PropertyName propertyName, unsigned attributes, PropertyOffset& offset, PutPropertySlot::Context context, DeferredStructureTransitionWatchpointFire* deferred)
 {
     ASSERT(!structure->isDictionary());
     ASSERT(structure->isObject());
@@ -412,7 +412,7 @@ Structure* Structure::addPropertyTransition(VM& vm, Structure* structure, Proper
         return transition;
     }
     
-    Structure* transition = create(vm, structure);
+    Structure* transition = create(vm, structure, deferred);
 
     transition->m_cachedPrototypeChain.setMayBeNull(vm, transition, structure->m_cachedPrototypeChain.get());
     transition->m_nameInPrevious = propertyName.uid();
@@ -950,29 +950,35 @@ void Structure::getPropertyNamesFromStructure(VM& vm, PropertyNameArray& propert
     }
 }
 
-namespace {
+void StructureFireDetail::dump(PrintStream& out) const
+{
+    out.print("Structure transition from ", *m_structure);
+}
 
-class StructureFireDetail : public FireDetail {
-public:
-    StructureFireDetail(const Structure* structure)
-        : m_structure(structure)
-    {
-    }
-    
-    virtual void dump(PrintStream& out) const override
-    {
-        out.print("Structure transition from ", *m_structure);
-    }
+DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire()
+    : m_structure(nullptr)
+{
+}
 
-private:
-    const Structure* m_structure;
-};
+DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire()
+{
+    if (m_structure)
+        m_structure->transitionWatchpointSet().fireAll(StructureFireDetail(m_structure));
+}
 
-} // anonymous namespace
+void DeferredStructureTransitionWatchpointFire::add(const Structure* structure)
+{
+    RELEASE_ASSERT(!m_structure);
+    RELEASE_ASSERT(structure);
+    m_structure = structure;
+}
 
-void Structure::didTransitionFromThisStructure() const
+void Structure::didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* deferred) const
 {
-    m_transitionWatchpointSet.fireAll(StructureFireDetail(this));
+    if (deferred)
+        deferred->add(this);
+    else
+        m_transitionWatchpointSet.fireAll(StructureFireDetail(this));
 }
 
 JSValue Structure::prototypeForLookup(CodeBlock* codeBlock) const
index 25a97fe..1d21225 100644 (file)
@@ -97,6 +97,31 @@ struct PropertyMapEntry {
     }
 };
 
+class StructureFireDetail : public FireDetail {
+public:
+    StructureFireDetail(const Structure* structure)
+        : m_structure(structure)
+    {
+    }
+    
+    virtual void dump(PrintStream& out) const override;
+
+private:
+    const Structure* m_structure;
+};
+
+class DeferredStructureTransitionWatchpointFire {
+    WTF_MAKE_NONCOPYABLE(DeferredStructureTransitionWatchpointFire);
+public:
+    JS_EXPORT_PRIVATE DeferredStructureTransitionWatchpointFire();
+    JS_EXPORT_PRIVATE ~DeferredStructureTransitionWatchpointFire();
+    
+    void add(const Structure*);
+    
+private:
+    const Structure* m_structure;
+};
+
 class Structure final : public JSCell {
 public:
     friend class StructureTransitionTable;
@@ -137,7 +162,7 @@ public:
 
     static void dumpStatistics();
 
-    JS_EXPORT_PRIVATE static Structure* addPropertyTransition(VM&, Structure*, PropertyName, unsigned attributes, PropertyOffset&, PutPropertySlot::Context = PutPropertySlot::UnknownContext);
+    JS_EXPORT_PRIVATE static Structure* addPropertyTransition(VM&, Structure*, PropertyName, unsigned attributes, PropertyOffset&, PutPropertySlot::Context = PutPropertySlot::UnknownContext, DeferredStructureTransitionWatchpointFire* = nullptr);
     static Structure* addPropertyTransitionToExistingStructureConcurrently(Structure*, UniquedStringImpl* uid, unsigned attributes, PropertyOffset&);
     JS_EXPORT_PRIVATE static Structure* addPropertyTransitionToExistingStructure(Structure*, PropertyName, unsigned attributes, PropertyOffset&);
     static Structure* removePropertyTransition(VM&, Structure*, PropertyName, PropertyOffset&);
@@ -402,7 +427,7 @@ public:
         m_transitionWatchpointSet.add(watchpoint);
     }
     
-    void didTransitionFromThisStructure() const;
+    void didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* = nullptr) const;
     
     InlineWatchpointSet& transitionWatchpointSet() const
     {
@@ -483,9 +508,9 @@ private:
 
     JS_EXPORT_PRIVATE Structure(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType, unsigned inlineCapacity);
     Structure(VM&);
-    Structure(VM&, Structure*);
+    Structure(VM&, Structure*, DeferredStructureTransitionWatchpointFire*);
 
-    static Structure* create(VM&, Structure*);
+    static Structure* create(VM&, Structure*, DeferredStructureTransitionWatchpointFire* = nullptr);
     
     static Structure* addPropertyTransitionToExistingStructureImpl(Structure*, UniquedStringImpl* uid, unsigned attributes, PropertyOffset&);
 
index 5c3aed7..f1955f6 100644 (file)
@@ -49,10 +49,10 @@ inline Structure* Structure::createStructure(VM& vm)
     return structure;
 }
 
-inline Structure* Structure::create(VM& vm, Structure* structure)
+inline Structure* Structure::create(VM& vm, Structure* structure, DeferredStructureTransitionWatchpointFire* deferred)
 {
     ASSERT(vm.structureStructure);
-    Structure* newStructure = new (NotNull, allocateCell<Structure>(vm.heap)) Structure(vm, structure);
+    Structure* newStructure = new (NotNull, allocateCell<Structure>(vm.heap)) Structure(vm, structure, deferred);
     newStructure->finishCreation(vm);
     return newStructure;
 }