[DFG] Avoid OSR exit in the middle of string concatenation
https://bugs.webkit.org/show_bug.cgi?id=145820

Reviewed by Filip Pizlo.

DFG attempt to compile ValueAdd with String type into MakeRope(left, ToString(ToPrimitive(right))).

So when right is speculated as SpecObject, ToPrimitive(SpecObject) is speculated as SpecString.
It leads ToString to become Identity with a speculated type check.

However, ToPrimitive and ToString are originated from the same bytecode. And ToPrimitive may have
an observable side effect when the given parameter is an object (calling object.{toString,valueOf}).

So when object.toString() returns a number (it is allowed in the ES spec), ToPrimitive performs
observable `object.toString()` calling. But ToString is converted into a speculated type check for
SpecString and it raises OSR exit. And we exit to the original ValueAdd's bytecode position and
it redundantly performs an observable ToPrimitive execution.

To fix this, this patch avoid fixing up for newly introduced ToString node.
Since fix up phase is not iterated repeatedly, by avoiding fixing up when generating the node,
we can avoid conversion from ToString to Check.

* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
* tests/stress/toprimitive-speculated-types.js: Added.
(shouldBe):
(raw):
(Counter):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@185728 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index c664cd7..cad77cd 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,34 @@
+2015-06-18  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [DFG] Avoid OSR exit in the middle of string concatenation
+        https://bugs.webkit.org/show_bug.cgi?id=145820
+
+        Reviewed by Filip Pizlo.
+
+        DFG attempt to compile ValueAdd with String type into MakeRope(left, ToString(ToPrimitive(right))).
+
+        So when right is speculated as SpecObject, ToPrimitive(SpecObject) is speculated as SpecString.
+        It leads ToString to become Identity with a speculated type check.
+
+        However, ToPrimitive and ToString are originated from the same bytecode. And ToPrimitive may have
+        an observable side effect when the given parameter is an object (calling object.{toString,valueOf}).
+
+        So when object.toString() returns a number (it is allowed in the ES spec), ToPrimitive performs
+        observable `object.toString()` calling. But ToString is converted into a speculated type check for
+        SpecString and it raises OSR exit. And we exit to the original ValueAdd's bytecode position and
+        it redundantly performs an observable ToPrimitive execution.
+
+        To fix this, this patch avoid fixing up for newly introduced ToString node.
+        Since fix up phase is not iterated repeatedly, by avoiding fixing up when generating the node,
+        we can avoid conversion from ToString to Check.
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
+        * tests/stress/toprimitive-speculated-types.js: Added.
+        (shouldBe):
+        (raw):
+        (Counter):
+
 2015-06-18  Brian J. Burg  <burg@cs.washington.edu>
 
         Web Inspector: improve generated types for objects passed to backend commands
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index 15ae8f6..872275d 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -1482,8 +1482,12 @@
                 m_indexInBlock, SpecString, ToString, node->origin, Edge(toPrimitive));
             
             fixupToPrimitive(toPrimitive);
-            fixupToStringOrCallStringConstructor(toString);
-            
+
+            // Don't fix up ToString. ToString and ToPrimitive are originated from the same bytecode and
+            // ToPrimitive may have an observable side effect. ToString should not be converted into Check
+            // with speculative type check because OSR exit reproduce an observable side effect done in
+            // ToPrimitive.
+
             right.setNode(toString);
         }
         
diff --git a/Source/JavaScriptCore/tests/stress/toprimitive-speculated-types.js b/Source/JavaScriptCore/tests/stress/toprimitive-speculated-types.js
new file mode 100644
index 0000000..3a18a19
--- /dev/null
+++ b/Source/JavaScriptCore/tests/stress/toprimitive-speculated-types.js
@@ -0,0 +1,27 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + JSON.stringify(actual));
+}
+
+function raw(array) {
+    var result = '';
+    for (var i = 0; i < array.length; ++i) {
+        result += array[i];
+    }
+    return result;
+}
+
+function Counter() {
+    return {
+        count: 0,
+        toString() {
+            // Return a number even if the "toString" method.
+            return this.count++;
+        }
+    };
+}
+
+for (var i = 0; i < 10000; ++i) {
+    var c = Counter();
+    shouldBe(raw([c, c]), "01");
+}