Bug 1137624 - Remove ArrayJoin code duplication, and use a correct alias set. r=jandem
authorNicolas B. Pierron <nicolas.b.pierron@mozilla.com>
Wed, 23 Dec 2015 14:49:48 +0000
changeset 312584 631fef632ea8e620964b5c338c10c8a178cff3e8
parent 312583 3f06ed0dd63583447ac808a709f8f7515bfb1ecc
child 312585 0912f477bd73a81c77aa9ec7d6b397f9aa612d30
push id5703
push userraliiev@mozilla.com
push dateMon, 07 Mar 2016 14:18:41 +0000
treeherdermozilla-beta@31e373ad5b5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1137624
milestone46.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1137624 - Remove ArrayJoin code duplication, and use a correct alias set. r=jandem
js/src/jit-test/tests/ion/array-join-bug1137624-1.js
js/src/jit-test/tests/ion/array-join-bug1137624-2.js
js/src/jit/InlinableNatives.h
js/src/jit/MCallOptimize.cpp
js/src/jit/MIR.h
js/src/jit/VMFunctions.cpp
js/src/jsarray.cpp
js/src/jsarray.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/array-join-bug1137624-1.js
@@ -0,0 +1,12 @@
+
+try {
+  var x = ["a", {toString() { s = x.join("-"); }}];
+  var a = x.join("a");
+  var b = x.join("b");
+  assertEq(a, b);
+} catch (e) {
+  // Using assertion does not work.
+  quit(0);
+}
+
+quit(3);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/array-join-bug1137624-2.js
@@ -0,0 +1,9 @@
+
+function f() {
+    var x = 0;
+    var a = [{toString: () => x++}];
+    for (var i=0; i<10000; i++)
+	a.join("");
+    assertEq(x, 10000);
+}
+f();
--- a/js/src/jit/InlinableNatives.h
+++ b/js/src/jit/InlinableNatives.h
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef jit_InlinableNatives_h
 #define jit_InlinableNatives_h
 
 #define INLINABLE_NATIVE_LIST(_)    \
     _(Array)                        \
     _(ArrayIsArray)                 \
+    _(ArrayJoin)                    \
     _(ArrayPop)                     \
     _(ArrayShift)                   \
     _(ArrayPush)                    \
     _(ArrayConcat)                  \
     _(ArraySlice)                   \
     _(ArraySplice)                  \
                                     \
     _(AtomicsCompareExchange)       \
--- a/js/src/jit/MCallOptimize.cpp
+++ b/js/src/jit/MCallOptimize.cpp
@@ -63,16 +63,18 @@ IonBuilder::inlineNativeCall(CallInfo& c
     }
 
     switch (InlinableNative inlNative = target->jitInfo()->inlinableNative) {
       // Array natives.
       case InlinableNative::Array:
         return inlineArray(callInfo);
       case InlinableNative::ArrayIsArray:
         return inlineArrayIsArray(callInfo);
+      case InlinableNative::ArrayJoin:
+        return inlineArrayJoin(callInfo);
       case InlinableNative::ArrayPop:
         return inlineArrayPopShift(callInfo, MArrayPopShift::Pop);
       case InlinableNative::ArrayShift:
         return inlineArrayPopShift(callInfo, MArrayPopShift::Shift);
       case InlinableNative::ArrayPush:
         return inlineArrayPush(callInfo);
       case InlinableNative::ArrayConcat:
         return inlineArrayConcat(callInfo);
@@ -641,16 +643,18 @@ IonBuilder::inlineArrayJoin(CallInfo& ca
 
     callInfo.setImplicitlyUsedUnchecked();
 
     MArrayJoin* ins = MArrayJoin::New(alloc(), callInfo.thisArg(), callInfo.getArg(0));
 
     current->add(ins);
     current->push(ins);
 
+    if (!resumeAfter(ins))
+        return InliningStatus_Error;
     return InliningStatus_Inlined;
 }
 
 IonBuilder::InliningStatus
 IonBuilder::inlineArrayPush(CallInfo& callInfo)
 {
     if (callInfo.argc() != 1 || callInfo.constructing()) {
         trackOptimizationOutcome(TrackedOutcome::CantInlineNativeBadForm);
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -9518,17 +9518,19 @@ class MArrayJoin
     }
     MDefinition* sep() const {
         return getOperand(1);
     }
     bool possiblyCalls() const override {
         return true;
     }
     virtual AliasSet getAliasSet() const override {
-        return AliasSet::Load(AliasSet::Element | AliasSet::ObjectFields);
+        // Array.join might coerce the elements of the Array to strings.  This
+        // coercion might cause the evaluation of the some JavaScript code.
+        return AliasSet::Store(AliasSet::Any);
     }
     MDefinition* foldsTo(TempAllocator& alloc) override;
 };
 
 // See comments above MMemoryBarrier, below.
 
 enum MemoryBarrierRequirement
 {
--- a/js/src/jit/VMFunctions.cpp
+++ b/js/src/jit/VMFunctions.cpp
@@ -375,51 +375,25 @@ ArrayConcatDense(JSContext* cx, HandleOb
     if (!js::array_concat(cx, 1, argv.begin()))
         return nullptr;
     return &argv[0].toObject();
 }
 
 JSString*
 ArrayJoin(JSContext* cx, HandleObject array, HandleString sep)
 {
-    // The annotations in this function follow the first steps of join
-    // specified in ES5.
-
-    // Step 1
-    RootedObject obj(cx, array);
-    if (!obj)
-        return nullptr;
-
-    AutoCycleDetector detector(cx, obj);
-    if (!detector.init())
-        return nullptr;
-
-    if (detector.foundCycle())
+    JS::AutoValueArray<3> argv(cx);
+    argv[0].setUndefined();
+    argv[1].setObject(*array);
+    argv[2].setString(sep);
+    if (!js::array_join(cx, 1, argv.begin()))
         return nullptr;
-
-    // Steps 2 and 3
-    uint32_t length;
-    if (!GetLengthProperty(cx, obj, &length))
-        return nullptr;
-
-    // Steps 4 and 5
-    RootedLinearString sepstr(cx);
-    if (sep) {
-        sepstr = sep->ensureLinear(cx);
-        if (!sepstr)
-            return nullptr;
-    } else {
-        sepstr = cx->names().comma;
-    }
-
-    // Step 6 to 11
-    return js::ArrayJoin<false>(cx, obj, sepstr, length);
+    return argv[0].toString();
 }
 
-
 bool
 CharCodeAt(JSContext* cx, HandleString str, int32_t index, uint32_t* code)
 {
     char16_t c;
     if (!str->getChar(cx, index, &c))
         return false;
     *code = c;
     return true;
--- a/js/src/jsarray.cpp
+++ b/js/src/jsarray.cpp
@@ -1075,82 +1075,23 @@ ArrayJoinKernel(JSContext* cx, Separator
                 return false;
         }
     }
 
     return true;
 }
 
 template <bool Locale>
-JSString*
-js::ArrayJoin(JSContext* cx, HandleObject obj, HandleLinearString sepstr, uint32_t length)
+bool
+ArrayJoin(JSContext* cx, CallArgs& args)
 {
     // This method is shared by Array.prototype.join and
     // Array.prototype.toLocaleString. The steps in ES5 are nearly the same, so
     // the annotations in this function apply to both toLocaleString and join.
 
-    // Steps 1 to 6, should be done by the caller.
-
-    // Step 6 is implicit in the loops below.
-
-    // An optimized version of a special case of steps 7-11: when length==1 and
-    // the 0th element is a string, ToString() of that element is a no-op and
-    // so it can be immediately returned as the result.
-    if (length == 1 && !Locale && GetAnyBoxedOrUnboxedInitializedLength(obj) == 1) {
-        Value elem0 = GetAnyBoxedOrUnboxedDenseElement(obj, 0);
-        if (elem0.isString())
-            return elem0.toString();
-    }
-
-    StringBuffer sb(cx);
-    if (sepstr->hasTwoByteChars() && !sb.ensureTwoByteChars())
-        return nullptr;
-
-    // The separator will be added |length - 1| times, reserve space for that
-    // so that we don't have to unnecessarily grow the buffer.
-    size_t seplen = sepstr->length();
-    CheckedInt<uint32_t> res = CheckedInt<uint32_t>(seplen) * (length - 1);
-    if (length > 0 && !res.isValid()) {
-        ReportAllocationOverflow(cx);
-        return nullptr;
-    }
-
-    if (length > 0 && !sb.reserve(res.value()))
-        return nullptr;
-
-    // Various optimized versions of steps 7-10.
-    if (seplen == 0) {
-        EmptySeparatorOp op;
-        if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
-            return nullptr;
-    } else if (seplen == 1) {
-        char16_t c = sepstr->latin1OrTwoByteChar(0);
-        if (c <= JSString::MAX_LATIN1_CHAR) {
-            CharSeparatorOp<Latin1Char> op(c);
-            if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
-                return nullptr;
-        } else {
-            CharSeparatorOp<char16_t> op(c);
-            if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
-                return nullptr;
-        }
-    } else {
-        StringSeparatorOp op(sepstr);
-        if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
-            return nullptr;
-    }
-
-    // Step 11
-    return sb.finishString();
-}
-
-template <bool Locale>
-bool
-ArrayJoin(JSContext* cx, CallArgs& args)
-{
     // Step 1
     RootedObject obj(cx, ToObject(cx, args.thisv()));
     if (!obj)
         return false;
 
     AutoCycleDetector detector(cx, obj);
     if (!detector.init())
         return false;
@@ -1163,32 +1104,83 @@ ArrayJoin(JSContext* cx, CallArgs& args)
     // Steps 2 and 3
     uint32_t length;
     if (!GetLengthProperty(cx, obj, &length))
         return false;
 
     // Steps 4 and 5
     RootedLinearString sepstr(cx);
     if (!Locale && args.hasDefined(0)) {
-        JSString* s = ToString<CanGC>(cx, args[0]);
+        JSString *s = ToString<CanGC>(cx, args[0]);
         if (!s)
             return false;
         sepstr = s->ensureLinear(cx);
         if (!sepstr)
             return false;
     } else {
         sepstr = cx->names().comma;
     }
 
-    // Step 6 to 11
-    JSString* res = js::ArrayJoin<Locale>(cx, obj, sepstr, length);
-    if (!res)
+    // Step 6 is implicit in the loops below.
+
+    // An optimized version of a special case of steps 7-11: when length==1 and
+    // the 0th element is a string, ToString() of that element is a no-op and
+    // so it can be immediately returned as the result.
+    if (length == 1 && !Locale && GetAnyBoxedOrUnboxedInitializedLength(obj) == 1) {
+        Value elem0 = GetAnyBoxedOrUnboxedDenseElement(obj, 0);
+        if (elem0.isString()) {
+            args.rval().set(elem0);
+            return true;
+        }
+    }
+
+    StringBuffer sb(cx);
+    if (sepstr->hasTwoByteChars() && !sb.ensureTwoByteChars())
+        return false;
+
+    // The separator will be added |length - 1| times, reserve space for that
+    // so that we don't have to unnecessarily grow the buffer.
+    size_t seplen = sepstr->length();
+    CheckedInt<uint32_t> res = CheckedInt<uint32_t>(seplen) * (length - 1);
+    if (length > 0 && !res.isValid()) {
+        ReportAllocationOverflow(cx);
+        return false;
+    }
+
+    if (length > 0 && !sb.reserve(res.value()))
         return false;
 
-    args.rval().setString(res);
+    // Various optimized versions of steps 7-10.
+    if (seplen == 0) {
+        EmptySeparatorOp op;
+        if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
+            return false;
+    } else if (seplen == 1) {
+        char16_t c = sepstr->latin1OrTwoByteChar(0);
+        if (c <= JSString::MAX_LATIN1_CHAR) {
+            CharSeparatorOp<Latin1Char> op(c);
+            if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
+                return false;
+        } else {
+            CharSeparatorOp<char16_t> op(c);
+            if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
+                return false;
+        }
+    } else {
+        StringSeparatorOp op(sepstr);
+        if (!ArrayJoinKernel<Locale>(cx, op, obj, length, sb))
+            return false;
+    }
+
+    // Step 11
+    JSString *str = sb.finishString();
+    if (!str)
+        return false;
+
+    args.rval().setString(str);
     return true;
 }
 
 /* ES5 15.4.4.3 */
 static bool
 array_toLocaleString(JSContext* cx, unsigned argc, Value* vp)
 {
     JS_CHECK_RECURSION(cx, return false);
@@ -3116,17 +3108,17 @@ array_of(JSContext* cx, unsigned argc, V
 static const JSFunctionSpec array_methods[] = {
 #if JS_HAS_TOSOURCE
     JS_FN(js_toSource_str,      array_toSource,     0,0),
 #endif
     JS_SELF_HOSTED_FN(js_toString_str, "ArrayToString",      0,0),
     JS_FN(js_toLocaleString_str,       array_toLocaleString, 0,0),
 
     /* Perl-ish methods. */
-    JS_FN("join",               array_join,         1,JSFUN_GENERIC_NATIVE),
+    JS_INLINABLE_FN("join",     array_join,         1,JSFUN_GENERIC_NATIVE, ArrayJoin),
     JS_FN("reverse",            array_reverse,      0,JSFUN_GENERIC_NATIVE),
     JS_FN("sort",               array_sort,         1,JSFUN_GENERIC_NATIVE),
     JS_INLINABLE_FN("push",     array_push,         1,JSFUN_GENERIC_NATIVE, ArrayPush),
     JS_INLINABLE_FN("pop",      array_pop,          0,JSFUN_GENERIC_NATIVE, ArrayPop),
     JS_INLINABLE_FN("shift",    array_shift,        0,JSFUN_GENERIC_NATIVE, ArrayShift),
     JS_FN("unshift",            array_unshift,      1,JSFUN_GENERIC_NATIVE),
     JS_INLINABLE_FN("splice",   array_splice,       2,JSFUN_GENERIC_NATIVE, ArraySplice),
 
--- a/js/src/jsarray.h
+++ b/js/src/jsarray.h
@@ -164,30 +164,23 @@ extern bool
 array_pop(JSContext* cx, unsigned argc, js::Value* vp);
 
 extern bool
 array_splice_impl(JSContext* cx, unsigned argc, js::Value* vp, bool pop);
 
 extern bool
 array_concat(JSContext* cx, unsigned argc, js::Value* vp);
 
-template <bool Locale>
-JSString*
-ArrayJoin(JSContext* cx, HandleObject obj, HandleLinearString sepstr, uint32_t length);
-
 extern bool
 array_concat_dense(JSContext* cx, HandleObject arr1, HandleObject arr2,
                    HandleObject result);
 
 extern bool
 array_join(JSContext* cx, unsigned argc, js::Value* vp);
 
-extern JSString*
-array_join_impl(JSContext* cx, HandleValue array, HandleString sep);
-
 extern void
 ArrayShiftMoveElements(JSObject* obj);
 
 extern bool
 array_shift(JSContext* cx, unsigned argc, js::Value* vp);
 
 extern bool
 array_unshift(JSContext* cx, unsigned argc, js::Value* vp);