Changeset 181817 in webkit


Ignore:
Timestamp:
Mar 20, 2015 4:26:26 PM (9 years ago)
Author:
fpizlo@apple.com
Message:

Observably effectful nodes in DFG IR should come last in their bytecode instruction (i.e. forExit section), except for Hint nodes
​https://bugs.webkit.org/show_bug.cgi?id=142920

Reviewed by Oliver Hunt, Geoffrey Garen, and Mark Lam.

Observably effectful, n.: If we reexecute the bytecode instruction after this node has
executed, then something other than the bytecode instruction's specified outcome will
happen.

We almost never had observably effectful nodes except at the end of the bytecode
instruction. The exception is a lowered transitioning PutById:

PutStructure(@o, S1 -> S2)
PutByOffset(@o, @o, @v)

The PutStructure is observably effectful: if you try to reexecute the bytecode after
doing the PutStructure, then we'll most likely crash. The generic PutById handling means
first checking what the old structure of the object is; but if we reexecute, the old
structure will seem to be the new structure. But the property ensured by the new
structure hasn't been stored yet, so any attempt to load it or scan it will crash.

Intriguingly, however, none of the other operations involved in the PutById are
observably effectful. Consider this example:

PutByOffset(@o, @o, @v)
PutStructure(@o, S1 -> S2)

Note that the PutStructure node doesn't reallocate property storage; see further below
for an example that does that. Because no property storage is happening, we know that we
already had room for the new property. This means that the PutByOffset is no observable
until the PutStructure executes and "reveals" the property. Hence, PutByOffset is not
observably effectful.

Now consider this:

b: AllocatePropertyStorage(@o)
PutByOffset(@b, @o, @v)
PutStructure(@o, S1 -> S2)

Surprisingly, this is also safe, because the AllocatePropertyStorage is not observably
effectful. It *does* reallocate the property storage and the new property storage pointer
is stored into the object. But until the PutStructure occurs, the world will just think
that the reallocation didn't happen, in the sense that we'll think that the property
storage is using less memory than what we just allocated. That's harmless.

The AllocatePropertyStorage is safe in other ways, too. Even if we GC'd after the
AllocatePropertyStorage but before the PutByOffset (or before the PutStructure),
everything could be expected to be fine, so long as all of @o, @v and @b are on the
stack. If they are all on the stack, then the GC will leave the property storage alone
(so the extra memory we just allocated would be safe). The GC will not scan the part of
the property storage that contains @v, but that's fine, so long as @v is on the stack.

The better long-term solution is probably bug 142921.

But for now, this:

  • Fixes an object materialization bug, exemplified by the two tests, that previously crashed 100% of the time with FTL enabled and concurrent JIT disabled.


  • Allows us to remove the workaround introduced in r174856.
  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::handlePutById):

  • dfg/DFGConstantFoldingPhase.cpp:

(JSC::DFG::ConstantFoldingPhase::emitPutByOffset):

  • dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::insertCheck):
(JSC::DFG::FixupPhase::indexOfNode): Deleted.
(JSC::DFG::FixupPhase::indexOfFirstNodeOfExitOrigin): Deleted.

  • dfg/DFGInsertionSet.h:

(JSC::DFG::InsertionSet::insertOutOfOrder): Deleted.
(JSC::DFG::InsertionSet::insertOutOfOrderNode): Deleted.

  • tests/stress/materialize-past-butterfly-allocation.js: Added.

(bar):
(foo0):
(foo1):
(foo2):
(foo3):
(foo4):

  • tests/stress/materialize-past-put-structure.js: Added.

(foo):

Location:
trunk/Source/JavaScriptCore
Files:
2 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r181814 r181817  
     12015-03-20  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Observably effectful nodes in DFG IR should come last in their bytecode instruction (i.e. forExit section), except for Hint nodes
     4        https://bugs.webkit.org/show_bug.cgi?id=142920
     5
     6        Reviewed by Oliver Hunt, Geoffrey Garen, and Mark Lam.
     7       
     8        Observably effectful, n.: If we reexecute the bytecode instruction after this node has
     9        executed, then something other than the bytecode instruction's specified outcome will
     10        happen.
     11
     12        We almost never had observably effectful nodes except at the end of the bytecode
     13        instruction.  The exception is a lowered transitioning PutById:
     14
     15        PutStructure(@o, S1 -> S2)
     16        PutByOffset(@o, @o, @v)
     17
     18        The PutStructure is observably effectful: if you try to reexecute the bytecode after
     19        doing the PutStructure, then we'll most likely crash.  The generic PutById handling means
     20        first checking what the old structure of the object is; but if we reexecute, the old
     21        structure will seem to be the new structure.  But the property ensured by the new
     22        structure hasn't been stored yet, so any attempt to load it or scan it will crash.
     23
     24        Intriguingly, however, none of the other operations involved in the PutById are
     25        observably effectful.  Consider this example:
     26
     27        PutByOffset(@o, @o, @v)
     28        PutStructure(@o, S1 -> S2)
     29
     30        Note that the PutStructure node doesn't reallocate property storage; see further below
     31        for an example that does that. Because no property storage is happening, we know that we
     32        already had room for the new property.  This means that the PutByOffset is no observable
     33        until the PutStructure executes and "reveals" the property.  Hence, PutByOffset is not
     34        observably effectful.
     35
     36        Now consider this:
     37
     38        b: AllocatePropertyStorage(@o)
     39        PutByOffset(@b, @o, @v)
     40        PutStructure(@o, S1 -> S2)
     41
     42        Surprisingly, this is also safe, because the AllocatePropertyStorage is not observably
     43        effectful. It *does* reallocate the property storage and the new property storage pointer
     44        is stored into the object. But until the PutStructure occurs, the world will just think
     45        that the reallocation didn't happen, in the sense that we'll think that the property
     46        storage is using less memory than what we just allocated. That's harmless.
     47
     48        The AllocatePropertyStorage is safe in other ways, too. Even if we GC'd after the
     49        AllocatePropertyStorage but before the PutByOffset (or before the PutStructure),
     50        everything could be expected to be fine, so long as all of @o, @v and @b are on the
     51        stack. If they are all on the stack, then the GC will leave the property storage alone
     52        (so the extra memory we just allocated would be safe). The GC will not scan the part of
     53        the property storage that contains @v, but that's fine, so long as @v is on the stack.
     54       
     55        The better long-term solution is probably bug 142921.
     56       
     57        But for now, this:
     58       
     59        - Fixes an object materialization bug, exemplified by the two tests, that previously
     60          crashed 100% of the time with FTL enabled and concurrent JIT disabled.
     61       
     62        - Allows us to remove the workaround introduced in r174856.
     63
     64        * dfg/DFGByteCodeParser.cpp:
     65        (JSC::DFG::ByteCodeParser::handlePutById):
     66        * dfg/DFGConstantFoldingPhase.cpp:
     67        (JSC::DFG::ConstantFoldingPhase::emitPutByOffset):
     68        * dfg/DFGFixupPhase.cpp:
     69        (JSC::DFG::FixupPhase::insertCheck):
     70        (JSC::DFG::FixupPhase::indexOfNode): Deleted.
     71        (JSC::DFG::FixupPhase::indexOfFirstNodeOfExitOrigin): Deleted.
     72        * dfg/DFGInsertionSet.h:
     73        (JSC::DFG::InsertionSet::insertOutOfOrder): Deleted.
     74        (JSC::DFG::InsertionSet::insertOutOfOrderNode): Deleted.
     75        * tests/stress/materialize-past-butterfly-allocation.js: Added.
     76        (bar):
     77        (foo0):
     78        (foo1):
     79        (foo2):
     80        (foo3):
     81        (foo4):
     82        * tests/stress/materialize-past-put-structure.js: Added.
     83        (foo):
     84
    1852015-03-20  Yusuke Suzuki  <utatane.tea@gmail.com>
    286

  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r181466 r181817  
    24232423        }
    24242424
    2425         addToGraph(PutStructure, OpInfo(transition), base);
    2426 
    24272425        StorageAccessData* data = m_graph.m_storageAccessData.add();
    24282426        data->offset = variant.offset();
    … …  
    24352433            base,
    24362434            value);
     2435
     2436        // FIXME: PutStructure goes last until we fix either
     2437        // https://bugs.webkit.org/show_bug.cgi?id=142921 or
     2438        // https://bugs.webkit.org/show_bug.cgi?id=142924.
     2439        addToGraph(PutStructure, OpInfo(transition), base);
    24372440
    24382441        if (m_graph.compilation())

  • trunk/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp

    r181466 r181817  
    569569        }
    570570
    571         if (variant.kind() == PutByIdVariant::Transition) {
    572             Node* putStructure = m_graph.addNode(SpecNone, PutStructure, origin, OpInfo(transition), childEdge);
    573             m_insertionSet.insertNode(indexInBlock, SpecNone, StoreBarrier, origin, Edge(node->child1().node(), KnownCellUse));
    574             m_insertionSet.insert(indexInBlock, putStructure);
    575         }
    576 
    577571        StorageAccessData& data = *m_graph.m_storageAccessData.add();
    578572        data.offset = variant.offset();
    … …  
    580574       
    581575        node->convertToPutByOffset(data, propertyStorage);
    582         m_insertionSet.insertNode(
    583             indexInBlock, SpecNone, StoreBarrier, origin,
    584             Edge(node->child2().node(), KnownCellUse));
     576
     577        if (variant.kind() == PutByIdVariant::Transition) {
     578            // FIXME: PutStructure goes last until we fix either
     579            // https://bugs.webkit.org/show_bug.cgi?id=142921 or
     580            // https://bugs.webkit.org/show_bug.cgi?id=142924.
     581            m_insertionSet.insertNode(
     582                indexInBlock + 1, SpecNone, PutStructure, origin, OpInfo(transition), childEdge);
     583        }
    585584    }
    586585   

  • trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

    r181650 r181817  
    8888        clearPhantomsAtEnd();
    8989        m_insertionSet.execute(block);
    90     }
    91    
    92     inline unsigned indexOfNode(Node* node, unsigned indexToSearchFrom)
    93     {
    94         unsigned index = indexToSearchFrom;
    95         while (index) {
    96             if (m_block->at(index) == node)
    97                 break;
    98             index--;
    99         }
    100         ASSERT(m_block->at(index) == node);
    101         return index;
    102     }
    103 
    104     inline unsigned indexOfFirstNodeOfExitOrigin(CodeOrigin& originForExit, unsigned indexToSearchFrom)
    105     {
    106         unsigned index = indexToSearchFrom;
    107         ASSERT(m_block->at(index)->origin.forExit == originForExit);
    108         while (index) {
    109             index--;
    110             if (m_block->at(index)->origin.forExit != originForExit) {
    111                 index++;
    112                 break;
    113             }
    114         }
    115         ASSERT(m_block->at(index)->origin.forExit == originForExit);
    116         return index;
    11790    }
    11891   
    … …  
    17831756    {
    17841757        observeUseKindOnNode<useKind>(node);
    1785         CodeOrigin& checkedNodeOrigin = node->origin.forExit;
    1786         CodeOrigin& currentNodeOrigin = m_currentNode->origin.forExit;
    1787         if (currentNodeOrigin == checkedNodeOrigin) {
    1788             // The checked node is within the same bytecode. Hence, the earliest
    1789             // position we can insert the check is right after the checked node.
    1790             indexInBlock = indexOfNode(node, indexInBlock) + 1;
    1791         } else {
    1792             // The checked node is from a preceding bytecode. Hence, the earliest
    1793             // position we can insert the check is at the start of the current
    1794             // bytecode.
    1795             indexInBlock = indexOfFirstNodeOfExitOrigin(currentNodeOrigin, indexInBlock);
    1796         }
    1797         m_insertionSet.insertOutOfOrderNode(
     1758        m_insertionSet.insertNode(
    17981759            indexInBlock, SpecNone, Check, m_currentNode->origin, Edge(node, useKind));
    17991760    }

  • trunk/Source/JavaScriptCore/dfg/DFGInsertionSet.h

    r177270 r181817  
    126126    }
    127127   
    128     Node* insertOutOfOrder(const Insertion& insertion)
    129     {
    130         size_t targetIndex = insertion.index();
    131         size_t entry = m_insertions.size();
    132         while (entry) {
    133             entry--;
    134             if (m_insertions[entry].index() <= targetIndex) {
    135                 entry++;
    136                 break;
    137             }
    138         }
    139         m_insertions.insert(entry, insertion);
    140         return insertion.element();
    141     }
    142    
    143     Node* insertOutOfOrder(size_t index, Node* element)
    144     {
    145         return insertOutOfOrder(Insertion(index, element));
    146     }
    147 
    148     template<typename... Params>
    149     Node* insertOutOfOrderNode(size_t index, SpeculatedType type, Params... params)
    150     {
    151         return insertOutOfOrder(index, m_graph.addNode(type, params...));
    152     }
    153 
    154128    void execute(BasicBlock* block)
    155129    {
Note: See TracChangeset for help on using the changeset viewer.