Bug 1351278 - Avoid stepping over Windows stack guard page, r=jandem
authorLars T Hansen <lhansen@mozilla.com>
Fri, 07 Apr 2017 14:41:35 +0200
changeset 567073 288f9c879d41b61c90f16af0e002d6331df921bd
parent 567072 89407b3607a2feedd46b2d13717724bc5f41b33b
child 567074 6efe7915f97b389c51b85bb2e1e45004f33ca680
push id55429
push userbmo:hskupin@gmail.com
push dateMon, 24 Apr 2017 10:48:57 +0000
reviewersjandem
bugs1351278
milestone55.0a1
Bug 1351278 - Avoid stepping over Windows stack guard page, r=jandem
js/src/jit-test/tests/arrays/apply-optimization.js
js/src/jit/CodeGenerator.cpp
js/src/jit/Lowering.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/arrays/apply-optimization.js
@@ -0,0 +1,58 @@
+function make(k) {
+    var a = new Array(k);
+    for ( let i=0 ; i < k ; i++ )
+	a[i] = {}
+    return a;
+}
+
+function g() {
+    return arguments.length;
+}
+
+function f(a) {
+    var sum = 0;
+    for ( let i=0 ; i < 1000 ; i++ )
+	sum += g.apply(null, a);
+    return sum;
+}
+
+function F2() {
+    var sum = 0;
+    for ( let i=0 ; i < 1000 ; i++ )
+	sum += g.apply(null, arguments);
+    return sum;
+}
+
+function F(a) {
+    return F2.apply(null, a);
+}
+
+function time(k, t) {
+    var then = Date.now();
+    assertEq(t(), 1000*k);
+    var now = Date.now();
+    return now - then;
+}
+
+function p(v) {
+    // Uncomment to see timings
+    // print(v);
+}
+
+f(make(200));
+
+// There is currently a cutoff after 375 where we bailout in order to avoid
+// handling very large stack frames.  This slows the operation down by a factor
+// of 100 or so.
+
+p(time(374, () => f(make(374))));
+p(time(375, () => f(make(375))));
+p(time(376, () => f(make(376))));
+p(time(377, () => f(make(377))));
+
+F(make(200));
+
+p(time(374, () => F(make(374))));
+p(time(375, () => F(make(375))));
+p(time(376, () => F(make(376))));
+p(time(377, () => F(make(377))));
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -4618,28 +4618,45 @@ CodeGenerator::emitApplyGeneric(T* apply
     // Pop arguments and continue.
     masm.bind(&end);
     emitPopArguments(extraStackSpace);
 }
 
 void
 CodeGenerator::visitApplyArgsGeneric(LApplyArgsGeneric* apply)
 {
+    // Limit the number of parameters we can handle to a number that does not risk
+    // us allocating too much stack, notably on Windows where there is a 4K guard page
+    // that has to be touched to extend the stack.  See bug 1351278.  The value "3000"
+    // is the size of the guard page minus an arbitrary, but large, safety margin.
+
+    LSnapshot* snapshot = apply->snapshot();
+    Register argcreg = ToRegister(apply->getArgc());
+
+    uint32_t limit = 3000 / sizeof(Value);
+    bailoutCmp32(Assembler::Above, argcreg, Imm32(limit), snapshot);
+
     emitApplyGeneric(apply);
 }
 
 void
 CodeGenerator::visitApplyArrayGeneric(LApplyArrayGeneric* apply)
 {
     LSnapshot* snapshot = apply->snapshot();
     Register tmp = ToRegister(apply->getTempObject());
 
     Address length(ToRegister(apply->getElements()), ObjectElements::offsetOfLength());
     masm.load32(length, tmp);
-    bailoutCmp32(Assembler::Above, tmp, Imm32(JitOptions.maxStackArgs), snapshot);
+
+    // See comment in visitApplyArgsGeneric, above.
+
+    uint32_t limit = 3000 / sizeof(Value);
+    bailoutCmp32(Assembler::Above, tmp, Imm32(limit), snapshot);
+
+    // Ensure that the array does not contain an uninitialized tail.
 
     Address initializedLength(ToRegister(apply->getElements()),
                               ObjectElements::offsetOfInitializedLength());
     masm.sub32(initializedLength, tmp);
     bailoutCmp32(Assembler::NotEqual, tmp, Imm32(0), snapshot);
 
     emitApplyGeneric(apply);
 }
--- a/js/src/jit/Lowering.cpp
+++ b/js/src/jit/Lowering.cpp
@@ -579,19 +579,20 @@ LIRGenerator::visitApplyArgs(MApplyArgs*
 
     LApplyArgsGeneric* lir = new(alloc()) LApplyArgsGeneric(
         useFixedAtStart(apply->getFunction(), CallTempReg3),
         useFixedAtStart(apply->getArgc(), CallTempReg0),
         useBoxFixedAtStart(apply->getThis(), CallTempReg4, CallTempReg5),
         tempFixed(CallTempReg1),  // object register
         tempFixed(CallTempReg2)); // stack counter register
 
-    // Bailout is only needed in the case of possible non-JSFunction callee.
-    if (!apply->getSingleTarget())
-        assignSnapshot(lir, Bailout_NonJSFunctionCallee);
+    // Bailout is needed in the case of possible non-JSFunction callee or too
+    // many values in the arguments array.  I'm going to use NonJSFunctionCallee
+    // for the code even if that is not an adequate description.
+    assignSnapshot(lir, Bailout_NonJSFunctionCallee);
 
     defineReturn(lir, apply);
     assignSafepoint(lir, apply);
 }
 
 void
 LIRGenerator::visitApplyArray(MApplyArray* apply)
 {