Changeset 206183 in webkit

Timestamp:

Sep 20, 2016 3:57:16 PM (8 years ago)

Author:

fpizlo@apple.com

Message:

DFG::StoreBarrierInsertionPhase should assume that any epoch increment may make objects older
​https://bugs.webkit.org/show_bug.cgi?id=162319

Reviewed by Saam Barati.

The store barrier phase needs to be aware of the fact that an object that is not in the
OldBlack state may be concurrently brought into that state. That means that:

• We cannot reason about the relative ages of objects. An object is either new, in which case we can store to it without barriers, or it's not in which case it needs a barrier.

• After we insert a barrier on an object, the object is no longer new, because now the GC knows about it and the GC may do things to it, like make it OldBlack.

This is a perf-neutral change. These optimizations were never particularly profitable.

• dfg/DFGStoreBarrierInsertionPhase.cpp:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGStoreBarrierInsertionPhase.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-09-20  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG::StoreBarrierInsertionPhase should assume that any epoch increment may make objects older
+        https://bugs.webkit.org/show_bug.cgi?id=162319
+
+        Reviewed by Saam Barati.
+        
+        The store barrier phase needs to be aware of the fact that an object that is not in the
+        OldBlack state may be concurrently brought into that state. That means that:
+        
+        - We cannot reason about the relative ages of objects. An object is either new, in which
+          case we can store to it without barriers, or it's not in which case it needs a barrier.
+        
+        - After we insert a barrier on an object, the object is no longer new, because now the GC
+          knows about it and the GC may do things to it, like make it OldBlack.
+        
+        This is a perf-neutral change. These optimizations were never particularly profitable.
+
+        * dfg/DFGStoreBarrierInsertionPhase.cpp:
+
 2016-09-20  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGStoreBarrierInsertionPhase.cpp

@@ -411 +411 @@
         } }
         
-        // We don't need a store barrier if the base is at least as new as the child. For
-        // example this won't need a barrier:
-        //
-        // var o = {}
-        // var p = {}
-        // p.f = o
-        //
-        // This is stronger than the currentEpoch rule in considerBarrier(Edge), because it will
-        // also eliminate barriers in cases like this:
-        //
-        // var o = {} // o.epoch = 1, currentEpoch = 1
-        // var p = {} // o.epoch = 1, p.epoch = 2, currentEpoch = 2
-        // var q = {} // o.epoch = 1, p.epoch = 2, q.epoch = 3, currentEpoch = 3
-        // p.f = o // p.epoch >= o.epoch
-        //
-        // This relationship works because if it holds then we are in one of the following
-        // scenarios. Note that we don't know *which* of these scenarios we are in, but it's
-        // one of them (though without loss of generality, you can replace "a GC happened" with
-        // "many GCs happened").
-        //
-        // 1) There is no GC between the allocation/last-barrier of base, child and now. Then
-        //    we definitely don't need a barrier.
-        //
-        // 2) There was a GC after child was allocated but before base was allocated. Then we
-        //    don't need a barrier, because base is still a new object.
-        //
-        // 3) There was a GC after both child and base were allocated. Then they are both old.
-        //    We don't need barriers on stores of old into old. Note that in this case it
-        //    doesn't matter if there was also a GC between the allocation of child and base.
-        //
-        // Note that barriers will lift an object into the current epoch. This is sort of weird.
-        // It means that later if you store that object into some other object, and that other
-        // object was previously newer object, you'll think that you need a barrier. We could
-        // avoid this by tracking allocation epoch and barrier epoch separately. For now I think
-        // that this would be overkill. But this does mean that there are the following
-        // possibilities when this relationship holds:
-        //
-        // 4) Base is allocated first. A GC happens and base becomes old. Then we allocate
-        //    child. (Note that alternatively the GC could happen during the allocation of
-        //    child.) Then we run a barrier on base. Base will appear to be as new as child
-        //    (same epoch). At this point, we don't need another barrier on base.
-        //
-        // 5) Base is allocated first. Then we allocate child. Then we run a GC. Then we run a
-        //    barrier on base. Base will appear newer than child. We don't need a barrier
-        //    because both objects are old.
-        //
-        // Something we watch out for here is that the null epoch is a catch-all for objects
-        // allocated before we did any epoch tracking. Two objects being in the null epoch
-        // means that we don't know their epoch relationship.
-        if (!!base->epoch() && !!child->epoch() && base->epoch() >= child->epoch()) {
-            if (verbose)
-                dataLog("            Rejecting because of epoch ordering.\n");
-            return;
-        }
-
         considerBarrier(base);
     }
@@ -491 +436 @@
     void insertBarrier(unsigned nodeIndex, Edge base, bool exitOK = true)
     {
+        // Inserting a barrier means that the object may become marked by the GC, which will make
+        // the object black.
+        base->setEpoch(Epoch());
+
         // If we're in global mode, we should only insert the barriers once we have converged.
         if (!reallyInsertBarriers())
@@ -509 +458 @@
         m_insertionSet.insertNode(
             nodeIndex, SpecNone, StoreBarrier, m_node->origin.takeValidExit(exitOK), base);
-
-        base->setEpoch(m_currentEpoch);
     }