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: | JavaScriptCore | Assignee: | 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
Saam Barati
2018-02-18 14:30:53 PST
We should probably do this: https://bugs.webkit.org/show_bug.cgi?id=144668 (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. Created attachment 334175 [details]
WIP
This makes JetStream's hash-map test 40-50% faster.
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. Created attachment 334188 [details]
patch
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 on attachment 334188 [details]
patch
r=me.
Comment on attachment 334188 [details] patch Clearing flags on attachment: 334188 Committed r228720: <https://trac.webkit.org/changeset/228720> All reviewed patches have been landed. Closing bug. |