Bug 182912

Summary: Don't mark an array profile out of bounds for the cases where the DFG will convert the access to SaneChain
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 182940    
Attachments:
Description Flags
WIP
none
patch none

Description Saam Barati 2018-02-18 14:30:53 PST
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.
Comment 1 Saam Barati 2018-02-18 14:46:52 PST
We should probably do this:
https://bugs.webkit.org/show_bug.cgi?id=144668
Comment 2 Saam Barati 2018-02-18 14:52:20 PST
(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.
Comment 3 Saam Barati 2018-02-19 12:25:39 PST
Created attachment 334175 [details]
WIP

This makes JetStream's hash-map test 40-50% faster.
Comment 4 Saam Barati 2018-02-19 12:27:29 PST
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.
Comment 5 Radar WebKit Bug Importer 2018-02-19 14:10:52 PST
<rdar://problem/37685083>
Comment 6 Saam Barati 2018-02-19 14:50:04 PST
Created attachment 334188 [details]
patch
Comment 7 Saam Barati 2018-02-19 14:54:40 PST
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.
Comment 8 Keith Miller 2018-02-19 16:27:42 PST
Comment on attachment 334188 [details]
patch

r=me.
Comment 9 WebKit Commit Bot 2018-02-19 18:00:43 PST
Comment on attachment 334188 [details]
patch

Clearing flags on attachment: 334188

Committed r228720: <https://trac.webkit.org/changeset/228720>
Comment 10 WebKit Commit Bot 2018-02-19 18:00:45 PST
All reviewed patches have been landed.  Closing bug.