Bug 182912 – Don't mark an array profile out of bounds for the cases where the DFG will convert the access to SaneChain

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Currently, we mark it out of bounds just if we can't getIndexQuickly. But that's not always the best thing to do, because, that will always mark it out of bounds if we see a hole. The DFG/FTL are smart enough to know when the array is sane chain vs not sane chain, and we can emit a GetByVal that just knows a hole is undefined when it's sane chain. So pessimize a GetByVal in that situation is bad from a performance standpoint. Currently, we only do the sane chain optimization for Contiguous/Double arrays. So, we should probably keep that in mind. But it'd be helpful to have another bit inside ArrayProfile that says if you read holes. Then, we can use that as input to Int32 arrays as well. We don't want a load from a hole from an int32 to always be marked as out of bounds, since when we're out of bounds, we must claim TOP for the effects we do.

[Saam Barati]

We should probably do this:
https://bugs.webkit.org/show_bug.cgi?id=144668

[Saam Barati]

(In reply to Saam Barati from comment #1)
> We should probably do this:
> https://bugs.webkit.org/show_bug.cgi?id=144668

I think what we really want is a bit in ArrayProfile saying if we're going to load a hole and that we're an original array structure (or something like that). If we are, we can lower Int32 array load to SaneChain as well. We probably want to do this with ArrayStorage as well.

[Saam Barati]

Created attachment 334175 [details]
WIP

This makes JetStream's hash-map test 40-50% faster.

[Saam Barati]

Comment on attachment 334175 [details]
WIP

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

> Source/JavaScriptCore/jit/JITOperations.cpp:1765
> +            if ((object->indexingType() == ArrayWithDouble || object->indexingType() == ArrayWithContiguous)

checking double here could theoretically lead to an infinite recompile loop. If the DFG doesn't convert this to sane chain, it'll exit on loading NaN. It only converts to sane chain when the result isn't used as other. We could probably optimistically leave this code as is, but have FixupPhase check exit state.

[Radar WebKit Bug Importer]

<rdar://problem/37685083>

[Saam Barati]

Created attachment 334188 [details]
patch

[Saam Barati]

Comment on attachment 334188 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:771
> +                    if (canDoSaneChain) {
> +                        JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
> +                        Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure();
> +                        Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure();
> +                        if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()

The only change in this code here is moving to this. The old code theoretically could hoist the load of arrayPrototypeChainIsSane() above  the loads of the Structures. In which case, we're in a race. This is almost certainly not gonna happen, but I refactored it to be correct. Same with the code in ArrayMode. The alternative would have been to stick in a loadLoadFence between watching the structure's and checking arrayProtoytpeChainIsSain(), but I went with this route since other code in the DFG uses this method.

[Keith Miller]

Comment on attachment 334188 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 334188 [details]
patch

Clearing flags on attachment: 334188

Committed r228720: <https://trac.webkit.org/changeset/228720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.