Bug 182912 - Don't mark an array profile out of bounds for the cases where the DFG will convert the access to SaneChain
Summary: Don't mark an array profile out of bounds for the cases where the DFG will co...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
Keywords: InRadar
Depends on:
Blocks: 182940
  Show dependency treegraph
Reported: 2018-02-18 14:30 PST by Saam Barati
Modified: 2018-02-19 18:00 PST (History)
13 users (show)

See Also:

WIP (12.88 KB, patch)
2018-02-19 12:25 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (22.64 KB, patch)
2018-02-19 14:50 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:
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]

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]

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
Comment 6 Saam Barati 2018-02-19 14:50:04 PST
Created attachment 334188 [details]
Comment 7 Saam Barati 2018-02-19 14:54:40 PST
Comment on attachment 334188 [details]

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]

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

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.