DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.

[Mark Lam]

This is already the preferred idiom used in most places in our compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's compileStringCharAt().  Consider the following:

    bool prototypeChainIsSane = false;
    if (globalObject->stringPrototypeChainIsSane()) {
        // FIXME: This could be captured using a Speculation mode that means
        // "out-of-bounds loads return a trivial value", something like
        // SaneChainOutOfBounds.
        // https://bugs.webkit.org/show_bug.cgi?id=144668
        
        m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
        m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));

        prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
    }

What's essential for correctness here is that the stringPrototype and objectPrototype structures be loaded before the loads in the second stringPrototypeChainIsSane() check.  Without a loadLoadFence before the second stringPrototypeChainIsSane() check, we can't guarantee that.  Elsewhere in the compiler, the preferred idiom for doing this right is to pre-load the structures first, do a loadLoadFence, and then do the IsSane check just once after e.g.

    Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
    Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);

    if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
        && objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
        && globalObject->arrayPrototypeChainIsSane()) {

        m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
        m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
        ...
    }

We should change DFG's SpeculativeJIT::compileGetByValOnString() and FTL's compileStringCharAt() to follow the same idiom.

<rdar://problem/54028228>

[Mark Lam]

Created attachment 377543 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 377543 [details]
proposed patch.

r=me

[Saam Barati]

Comment on attachment 377543 [details]
proposed patch.

r=me too

[Mark Lam]

Thanks for the reviews.  Landed in r249247: <http://trac.webkit.org/r249247>.

[Saam Barati]

Comment on attachment 377543 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=377543&action=review

> Source/JavaScriptCore/runtime/StructureInlines.h:111
> +    ASSERT(!isMainThread() || object->structure() == this);

I think this should be isCompilationThread()

> Source/JavaScriptCore/runtime/StructureInlines.h:119
> +    ASSERT(!isMainThread() || object->structure() == this);

ditto

[Mark Lam]

(In reply to Saam Barati from comment #5)
> Comment on attachment 377543 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377543&action=review
> 
> > Source/JavaScriptCore/runtime/StructureInlines.h:111
> > +    ASSERT(!isMainThread() || object->structure() == this);
> 
> I think this should be isCompilationThread()
> 
> > Source/JavaScriptCore/runtime/StructureInlines.h:119
> > +    ASSERT(!isMainThread() || object->structure() == this);
> 
> ditto

The use of isMainThread() effectively disables the assertions for worker threads as well.  I don't think we call these methods from the GC threads.  Otherwise, using isCompilationThread() would be wrong too.  I'll do the test in a separate bug so that we can test it before landing.

See https://bugs.webkit.org/show_bug.cgi?id=201449.