methodOfGettingAValueProfileFor should return argument value profiles even when node...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Dec 2019 23:08:04 +0000 (23:08 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Dec 2019 23:08:04 +0000 (23:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205083

Reviewed by Yusuke Suzuki.

Inside methodOfGettingAValueProfileFor, we only grab profiles when the child
node and the parent node were from different code origins. This policy doesn't
make sense when the child node is the load of an argument value. In that case,
we can always just grab the argument profile.

We might want to reconsider this policy in general, since it's common for a
node to emit a GetLocal to grab its incoming arguments (this is frequently
done in the DFG when reloading locals across basic blocks).

This fixes an OSR exit compile loop inside Speedometer 2's React-Redux-TodoMVC
benchmark. That benchmark would repeatedly exit inside CompareStrictEq by
repeatedly speculating Object. That node would run with 95% incoming Objects,
and 5% incoming strings, and because we didn't grab the argument value profile
during exit, we never updated the profile with the String type information.

* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::methodOfGettingAValueProfileFor):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@253350 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGGraph.cpp

index 73992d5..ba93e74 100644 (file)
@@ -1,3 +1,28 @@
+2019-12-10  Saam Barati  <sbarati@apple.com>
+
+        methodOfGettingAValueProfileFor should return argument value profiles even when node and operandNode are the same origin
+        https://bugs.webkit.org/show_bug.cgi?id=205083
+
+        Reviewed by Yusuke Suzuki.
+
+        Inside methodOfGettingAValueProfileFor, we only grab profiles when the child
+        node and the parent node were from different code origins. This policy doesn't
+        make sense when the child node is the load of an argument value. In that case,
+        we can always just grab the argument profile.
+        
+        We might want to reconsider this policy in general, since it's common for a
+        node to emit a GetLocal to grab its incoming arguments (this is frequently
+        done in the DFG when reloading locals across basic blocks).
+        
+        This fixes an OSR exit compile loop inside Speedometer 2's React-Redux-TodoMVC
+        benchmark. That benchmark would repeatedly exit inside CompareStrictEq by
+        repeatedly speculating Object. That node would run with 95% incoming Objects,
+        and 5% incoming strings, and because we didn't grab the argument value profile
+        during exit, we never updated the profile with the String type information.
+
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::methodOfGettingAValueProfileFor):
+
 2019-12-10  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r253321.
index f874f33..7964985 100644 (file)
@@ -1633,20 +1633,24 @@ MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* curren
     // This represents IR like `CurrentNode(@operandNode)`. For example: `GetByVal(..., Int32:@GetLocal)`.
 
     for (Node* node = operandNode; node;) {
+        if (node->accessesStack(*this)) {
+            if (m_form != SSA && node->local().isArgument()) {
+                int argument = node->local().toArgument();
+                Node* argumentNode = m_rootToArguments.find(block(0))->value[argument];
+                // FIXME: We should match SetArgumentDefinitely nodes at other entrypoints as well:
+                // https://bugs.webkit.org/show_bug.cgi?id=175841
+                if (argumentNode && node->variableAccessData() == argumentNode->variableAccessData()) {
+                    CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
+                    return &profiledBlock->valueProfileForArgument(argument);
+                }
+            }
+        }
+
         // currentNode is null when we're doing speculation checks for checkArgumentTypes().
         if (!currentNode || node->origin.semantic != currentNode->origin.semantic || !currentNode->hasResult()) {
             CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
 
             if (node->accessesStack(*this)) {
-                if (m_form != SSA && node->local().isArgument()) {
-                    int argument = node->local().toArgument();
-                    Node* argumentNode = m_rootToArguments.find(block(0))->value[argument];
-                    // FIXME: We should match SetArgumentDefinitely nodes at other entrypoints as well:
-                    // https://bugs.webkit.org/show_bug.cgi?id=175841
-                    if (argumentNode && node->variableAccessData() == argumentNode->variableAccessData())
-                        return &profiledBlock->valueProfileForArgument(argument);
-                }
-
                 if (node->op() == GetLocal) {
                     return MethodOfGettingAValueProfile::fromLazyOperand(
                         profiledBlock,