References from code to Structures should be stronger than weak
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 May 2016 02:23:28 +0000 (02:23 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 May 2016 02:23:28 +0000 (02:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157324

Reviewed by Mark Lam.

If code refers to a Structure and the Structure dies, then previously we'd kill the code.
This makes sense because the Structure could be the only thing left referring to some global
object or prototype.

But this also causes unnecessary churn. Sometimes there will be a structure that we just
haven't really done anything with recently and so it appears dead. The approach we use
elsewhere in our type inference is that the type that the code uses is general enough to
handle every past execution. Having the GC clear code when some Structure it uses dies means
that we forget that the code used that Structure. We'll either assume that the code is more
monomorphic than it really is (because after GC we patch in some other structure but not the
deleted one, so it looks like we only ever saw the new structure), or we'll assume that it's
crazier than it really is (because we'll remember that there had been some structure that
caused deletion, so we'll assume that deletions might happen in the future, so we'll use a
fully dynamic IC).

This change introduces a more nuanced policy: if it's cheap to mark a dead Structure then we
should mark it just so that all of the code that refers to it remembers that there had been
this exact Structure in the past. If the code often goes through different Structures then
we already have great mechanisms to realize that the code is nutty (namely, the
PolymorphicAccess size limit). But if the code just does this a handful of times then
remembering this old Structure is probably net good:

- It obeys the "handle all past executions" law.
- It preserves the history of the property access, allowing a precise measure of its past
  polymorphism.
- It makes the code ready to run fast if the user decides to use that Structure again.
  Marking the Structure means it will stay in whatever property transition tables it was in,
  so if the program does the same thing it did in the past, it will get this old Structure.

It looks like this is a progression in gbemu and it makes gbemu perform more
deterministically. Also, it seems that this makes JetStream run faster.

Over five in-browser runs of JetStream, here's what we see before and after:

Geometric Mean:
    Before              After
    229.23 +- 8.2523    230.70 +- 12.888
    232.91 +- 15.638    239.04 +- 13.766
    234.79 +- 12.760    236.32 +- 15.562
    236.20 +- 23.125    242.02 +- 3.3865
    237.22 +- 2.1929    237.23 +- 17.664

Just gbemu:
    Before              After
    541.0 +- 135.8      481.7 +- 143.4
    518.9 +- 15.65      508.1 +- 136.3
    362.5 +- 0.8884     489.7 +- 101.4
    470.7 +- 313.3      530.7 +- 11.49
    418.7 +- 180.6      537.2 +- 6.514

Notice that there is plenty of noise before and after, but the noise is now far less severe.
After this change I did not see any runs like "470.7 +- 313.3" where the size of the
confidence interval (313.3 * 2) is greater than the score (470.7). Also, notice that the
least noisy run before the change also got a lower score than we ever observed after the
change (36.5 +- 0.8884). The noise, and these occasional very low scores, are due to a
pathology where the GC would reset some stubs at an unfortunate time during profiling,
causing the optimizing compiler to make many poor decisions. That pathology doesn't exist
anymore.

On the other hand, prior to this change it was possible for gbemu to sometimes run sooooper
fast because the GC would cause the profiler to forget gbemu's behavior on the first tick
and focus only on its behavior in subsequent ticks. So, in steady state, we'd optimize gbemu
for its later behavior rather than a combination of its early behavior and later behavior.
We rarely got lucky this way, so it's not fair to view this quirk as a feature.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::propagateTransitions):
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::visitWeak):
(JSC::AccessCase::propagateTransitions):
(JSC::AccessCase::generateWithGuard):
(JSC::PolymorphicAccess::visitWeak):
(JSC::PolymorphicAccess::propagateTransitions):
(JSC::PolymorphicAccess::dump):
* bytecode/PolymorphicAccess.h:
* bytecode/StructureStubInfo.cpp:
(JSC::StructureStubInfo::visitWeakReferences):
(JSC::StructureStubInfo::propagateTransitions):
(JSC::StructureStubInfo::containsPC):
* bytecode/StructureStubInfo.h:
(JSC::StructureStubInfo::considerCaching):
* runtime/Structure.cpp:
(JSC::Structure::visitChildren):
(JSC::Structure::isCheapDuringGC):
(JSC::Structure::markIfCheap):
(JSC::Structure::prototypeChainMayInterceptStoreTo):
* runtime/Structure.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/bytecode/PolymorphicAccess.h
Source/JavaScriptCore/bytecode/StructureStubInfo.cpp
Source/JavaScriptCore/bytecode/StructureStubInfo.h
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/Structure.h

index eb3c7ba..e9c4591 100644 (file)
@@ -1,3 +1,98 @@
+2016-05-03  Filip Pizlo  <fpizlo@apple.com>
+
+        References from code to Structures should be stronger than weak
+        https://bugs.webkit.org/show_bug.cgi?id=157324
+
+        Reviewed by Mark Lam.
+        
+        If code refers to a Structure and the Structure dies, then previously we'd kill the code. 
+        This makes sense because the Structure could be the only thing left referring to some global
+        object or prototype.
+
+        But this also causes unnecessary churn. Sometimes there will be a structure that we just
+        haven't really done anything with recently and so it appears dead. The approach we use
+        elsewhere in our type inference is that the type that the code uses is general enough to
+        handle every past execution. Having the GC clear code when some Structure it uses dies means
+        that we forget that the code used that Structure. We'll either assume that the code is more
+        monomorphic than it really is (because after GC we patch in some other structure but not the
+        deleted one, so it looks like we only ever saw the new structure), or we'll assume that it's
+        crazier than it really is (because we'll remember that there had been some structure that
+        caused deletion, so we'll assume that deletions might happen in the future, so we'll use a
+        fully dynamic IC).
+
+        This change introduces a more nuanced policy: if it's cheap to mark a dead Structure then we
+        should mark it just so that all of the code that refers to it remembers that there had been
+        this exact Structure in the past. If the code often goes through different Structures then
+        we already have great mechanisms to realize that the code is nutty (namely, the
+        PolymorphicAccess size limit). But if the code just does this a handful of times then
+        remembering this old Structure is probably net good:
+
+        - It obeys the "handle all past executions" law.
+        - It preserves the history of the property access, allowing a precise measure of its past
+          polymorphism.
+        - It makes the code ready to run fast if the user decides to use that Structure again.
+          Marking the Structure means it will stay in whatever property transition tables it was in,
+          so if the program does the same thing it did in the past, it will get this old Structure.
+
+        It looks like this is a progression in gbemu and it makes gbemu perform more
+        deterministically. Also, it seems that this makes JetStream run faster.
+        
+        Over five in-browser runs of JetStream, here's what we see before and after:
+        
+        Geometric Mean:
+            Before              After
+            229.23 +- 8.2523    230.70 +- 12.888
+            232.91 +- 15.638    239.04 +- 13.766
+            234.79 +- 12.760    236.32 +- 15.562
+            236.20 +- 23.125    242.02 +- 3.3865
+            237.22 +- 2.1929    237.23 +- 17.664
+        
+        Just gbemu:
+            Before              After
+            541.0 +- 135.8      481.7 +- 143.4
+            518.9 +- 15.65      508.1 +- 136.3
+            362.5 +- 0.8884     489.7 +- 101.4
+            470.7 +- 313.3      530.7 +- 11.49
+            418.7 +- 180.6      537.2 +- 6.514
+        
+        Notice that there is plenty of noise before and after, but the noise is now far less severe.
+        After this change I did not see any runs like "470.7 +- 313.3" where the size of the 
+        confidence interval (313.3 * 2) is greater than the score (470.7). Also, notice that the
+        least noisy run before the change also got a lower score than we ever observed after the
+        change (36.5 +- 0.8884). The noise, and these occasional very low scores, are due to a
+        pathology where the GC would reset some stubs at an unfortunate time during profiling,
+        causing the optimizing compiler to make many poor decisions. That pathology doesn't exist
+        anymore.
+        
+        On the other hand, prior to this change it was possible for gbemu to sometimes run sooooper
+        fast because the GC would cause the profiler to forget gbemu's behavior on the first tick
+        and focus only on its behavior in subsequent ticks. So, in steady state, we'd optimize gbemu
+        for its later behavior rather than a combination of its early behavior and later behavior.
+        We rarely got lucky this way, so it's not fair to view this quirk as a feature.
+        
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::propagateTransitions):
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessCase::visitWeak):
+        (JSC::AccessCase::propagateTransitions):
+        (JSC::AccessCase::generateWithGuard):
+        (JSC::PolymorphicAccess::visitWeak):
+        (JSC::PolymorphicAccess::propagateTransitions):
+        (JSC::PolymorphicAccess::dump):
+        * bytecode/PolymorphicAccess.h:
+        * bytecode/StructureStubInfo.cpp:
+        (JSC::StructureStubInfo::visitWeakReferences):
+        (JSC::StructureStubInfo::propagateTransitions):
+        (JSC::StructureStubInfo::containsPC):
+        * bytecode/StructureStubInfo.h:
+        (JSC::StructureStubInfo::considerCaching):
+        * runtime/Structure.cpp:
+        (JSC::Structure::visitChildren):
+        (JSC::Structure::isCheapDuringGC):
+        (JSC::Structure::markIfCheap):
+        (JSC::Structure::prototypeChainMayInterceptStoreTo):
+        * runtime/Structure.h:
+
 2016-05-03  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Simplify console.clear
index a149e29..63e7334 100644 (file)
@@ -2616,32 +2616,16 @@ void CodeBlock::propagateTransitions(SlotVisitor& visitor)
 
 #if ENABLE(JIT)
     if (JITCode::isJIT(jitType())) {
-        for (Bag<StructureStubInfo>::iterator iter = m_stubInfos.begin(); !!iter; ++iter) {
-            StructureStubInfo& stubInfo = **iter;
-            if (stubInfo.cacheType != CacheType::Stub)
-                continue;
-            PolymorphicAccess* list = stubInfo.u.stub;
-            JSCell* origin = stubInfo.codeOrigin.codeOriginOwner();
-            if (origin && !Heap::isMarked(origin)) {
-                allAreMarkedSoFar = false;
-                continue;
-            }
-            for (unsigned j = list->size(); j--;) {
-                const AccessCase& access = list->at(j);
-                if (access.type() != AccessCase::Transition)
-                    continue;
-                if (Heap::isMarked(access.structure()))
-                    visitor.appendUnbarrieredReadOnlyPointer(access.newStructure());
-                else
-                    allAreMarkedSoFar = false;
-            }
-        }
+        for (Bag<StructureStubInfo>::iterator iter = m_stubInfos.begin(); !!iter; ++iter)
+            allAreMarkedSoFar &= (*iter)->propagateTransitions(visitor);
     }
 #endif // ENABLE(JIT)
     
 #if ENABLE(DFG_JIT)
     if (JITCode::isOptimizingJIT(jitType())) {
         DFG::CommonData* dfgCommon = m_jitCode->dfgCommon();
+        for (auto& weakReference : dfgCommon->weakStructureReferences)
+            allAreMarkedSoFar &= weakReference->markIfCheap(visitor);
         
         for (unsigned i = 0; i < dfgCommon->transitions.size(); ++i) {
             if (shouldMarkTransition(dfgCommon->transitions[i])) {
index 91e2240..dacefb2 100644 (file)
@@ -554,6 +554,27 @@ bool AccessCase::visitWeak(VM& vm) const
     return true;
 }
 
+bool AccessCase::propagateTransitions(SlotVisitor& visitor) const
+{
+    bool result = true;
+    
+    if (m_structure)
+        result &= m_structure->markIfCheap(visitor);
+    
+    switch (m_type) {
+    case Transition:
+        if (Heap::isMarked(m_structure->previousID()))
+            visitor.appendUnbarrieredReadOnlyPointer(m_structure.get());
+        else
+            result = false;
+        break;
+    default:
+        break;
+    }
+    
+    return result;
+}
+
 void AccessCase::generateWithGuard(
     AccessGenerationState& state, CCallHelpers::JumpList& fallThrough)
 {
@@ -1487,6 +1508,14 @@ bool PolymorphicAccess::visitWeak(VM& vm) const
     return true;
 }
 
+bool PolymorphicAccess::propagateTransitions(SlotVisitor& visitor) const
+{
+    bool result = true;
+    for (unsigned i = 0; i < size(); ++i)
+        result &= at(i).propagateTransitions(visitor);
+    return result;
+}
+
 void PolymorphicAccess::dump(PrintStream& out) const
 {
     out.print(RawPointer(this), ":[");
index 1234245..9469652 100644 (file)
@@ -248,6 +248,7 @@ private:
     AccessCase();
 
     bool visitWeak(VM&) const;
+    bool propagateTransitions(SlotVisitor&) const;
     
     // FIXME: This only exists because of how AccessCase puts post-generation things into itself.
     // https://bugs.webkit.org/show_bug.cgi?id=156456
@@ -396,6 +397,10 @@ public:
 
     // If this returns false then we are requesting a reset of the owning StructureStubInfo.
     bool visitWeak(VM&) const;
+    
+    // This returns true if it has marked everything it will ever marked. This can be used as an
+    // optimization to then avoid calling this method again during the fixpoint.
+    bool propagateTransitions(SlotVisitor&) const;
 
     void aboutToDie();
 
index d925cfc..77531c5 100644 (file)
@@ -230,7 +230,10 @@ void StructureStubInfo::visitWeakReferences(CodeBlock* codeBlock)
 {
     VM& vm = *codeBlock->vm();
     
-    bufferedStructures.clear();
+    bufferedStructures.genericFilter(
+        [&] (Structure* structure) -> bool {
+            return Heap::isMarked(structure);
+        });
 
     switch (cacheType) {
     case CacheType::GetByIdSelf:
@@ -250,6 +253,22 @@ void StructureStubInfo::visitWeakReferences(CodeBlock* codeBlock)
     resetByGC = true;
 }
 
+bool StructureStubInfo::propagateTransitions(SlotVisitor& visitor)
+{
+    switch (cacheType) {
+    case CacheType::Unset:
+        return true;
+    case CacheType::GetByIdSelf:
+    case CacheType::PutByIdReplace:
+        return u.byIdSelf.baseObjectStructure->markIfCheap(visitor);
+    case CacheType::Stub:
+        return u.stub->propagateTransitions(visitor);
+    }
+    
+    RELEASE_ASSERT_NOT_REACHED();
+    return true;
+}
+
 bool StructureStubInfo::containsPC(void* pc) const
 {
     if (cacheType != CacheType::Stub)
index 390b0ed..d10b5f5 100644 (file)
@@ -81,6 +81,9 @@ public:
     // Check if the stub has weak references that are dead. If it does, then it resets itself,
     // either entirely or just enough to ensure that those dead pointers don't get used anymore.
     void visitWeakReferences(CodeBlock*);
+    
+    // This returns true if it has marked everything that it will ever mark.
+    bool propagateTransitions(SlotVisitor&);
         
     ALWAYS_INLINE bool considerCaching(Structure* structure)
     {
index 46b4bd7..6696146 100644 (file)
@@ -1135,6 +1135,25 @@ void Structure::visitChildren(JSCell* cell, SlotVisitor& visitor)
     visitor.append(&thisObject->m_inferredTypeTable);
 }
 
+bool Structure::isCheapDuringGC()
+{
+    // FIXME: We could make this even safer by returning false if this structure's property table
+    // has any large property names.
+    // https://bugs.webkit.org/show_bug.cgi?id=157334
+    
+    return (!m_globalObject || Heap::isMarked(m_globalObject.get()))
+        && (!storedPrototypeObject() || Heap::isMarked(storedPrototypeObject()));
+}
+
+bool Structure::markIfCheap(SlotVisitor& visitor)
+{
+    if (!isCheapDuringGC())
+        return Heap::isMarked(this);
+    
+    visitor.appendUnbarrieredReadOnlyPointer(this);
+    return true;
+}
+
 bool Structure::prototypeChainMayInterceptStoreTo(VM& vm, PropertyName propertyName)
 {
     if (parseIndex(propertyName))
index dde3f25..9f67ef0 100644 (file)
@@ -260,6 +260,17 @@ public:
     StructureChain* prototypeChain(VM&, JSGlobalObject*) const;
     StructureChain* prototypeChain(ExecState*) const;
     static void visitChildren(JSCell*, SlotVisitor&);
+    
+    // A Structure is cheap to mark during GC if doing so would only add a small and bounded amount
+    // to our heap footprint. For example, if the structure refers to a global object that is not
+    // yet marked, then as far as we know, the decision to mark this Structure would lead to a large
+    // increase in footprint because no other object refers to that global object. This method
+    // returns true if all user-controlled (and hence unbounded in size) objects referenced from the
+    // Structure are already marked.
+    bool isCheapDuringGC();
+    
+    // Returns true if this structure is now marked.
+    bool markIfCheap(SlotVisitor&);
         
     // Will just the prototype chain intercept this property access?
     JS_EXPORT_PRIVATE bool prototypeChainMayInterceptStoreTo(VM&, PropertyName);