We can apply the same speculation to js_fun_apply (bug 602129) as js_fun_call. This is a prerequisite for bug 595884, which will speculatively avoid creating the args object.

This patch has preliminary changes to js_fun_apply that simplify the next patch.

This patch is lightly tested. Preliminary results show a 3x (-m) speedup of function f(x) { return x+1 } f.apply(null, [3]) which puts us slightly ahead of jsc. I think there is less relative speedup than bug 602129 due to the meatier nature of ic::SplatApplyArguments, which needs to be done in any case.

Passes trace/ref tests. SS doesn't use much apply, but v8-raytrace (-m) does: raytrace: 1.199x as fast 294.5ms +/- 2.0% 245.7ms +/- 1.0% (This is without jandem's optimization in bug 595884 to avoid creating args objects.)

x64 was only providing a single SavedReg. This patch makes ValueReg use r10 (a temp reg) which frees up r15.

Refactored and green on try.

Comment on attachment 486279 [details] [diff] [review] factor/optimize apply 2 >diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp >+JSBool >+js_GetArrayElements(JSContext *cx, JSObject *aobj, jsuint length, Value *vp) This should use bool/true/false and be js:: rather than js_. More importantly, GetArrayElements does not to me suggest it would work on anything but arrays. GetElementsFromObject, perhaps, and add an interface-description comment by the declaration that makes explicit the requirement in the debug block at top of the method, maybe with a few words of rationalization? (But you should add the comment regardless what name is chosen.) >+{ >+#ifdef DEBUG >+ jsuint maxlen; >+ if (!js_GetLengthProperty(cx, aobj, &maxlen)) >+ return false; >+ JS_ASSERT(length <= maxlen); >+#endif This introduces observably wrong behavior in debug builds for (untested) something like this: var called = false; var o = { get length() { if (!called) return called = true, 0; throw 42; } }; Array.apply(undefined, o); If you're going to do this, lookup the property or something and ware side effects getting the property might have. >+ if (aobj->isDenseArray() && length <= aobj->getDenseArrayCapacity()) { >+ Value *srcbeg = aobj->getDenseArrayElements(); >+ Value *srcend = srcbeg + length; >+ for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src) >+ *dst = src->isMagic(JS_ARRAY_HOLE) ? UndefinedValue() : *src; Minor preference for src < srcend here, because in the case where somehow the fencepost is skipped, all values past it will also cause the loop to terminate. >+ /* >+ * Two cases, two loops: note how in the case of an active stack frame >+ * backing aobj, even though we copy from fp->argv, we still must check >+ * aobj->getArgsElement(i) for a hole, to handle a delete on the >+ * corresponding arguments element. See args_delProperty. >+ */ >+ JSStackFrame *fp = (JSStackFrame *) aobj->getPrivate(); >+ if (fp) { No reason not to put the declaration in the if. >+ } else { >+ Value *srcbeg = aobj->getArgsElements(); >+ Value *srcend = srcbeg + length; >+ for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src) >+ *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src; Same here. >+ } >+ } else { >+ for (uintN i = 0; i < length; i++) { >+ if (!aobj->getProperty(cx, INT_TO_JSID(jsint(i)), &vp[i])) >+ return JS_FALSE; >+ } A kosher bounds-check, in my opinion. :-)

(In reply to comment #9) Thanks, good catch on the debug-js_GetLengthProperty hazard. > More importantly, > GetArrayElements does not to me suggest it would work on anything but arrays. Chosen for parity with GetArrayElement. Per real-life conversation, renaming both to GetElement/GetElements. > >+ Value *srcbeg = aobj->getArgsElements(); > >+ Value *srcend = srcbeg + length; > >+ for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src) > >+ *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src; > > Same here. I generally prefer not to have overly-long for-initializers. I think it makes this loop more readable, since the loop itself only talks about the loop variable and its bounds.

Waldo: ignore the unnamed namespace, I removed it.

(In reply to comment #10) > > >+ Value *srcbeg = aobj->getArgsElements(); > > >+ Value *srcend = srcbeg + length; > > >+ for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src) > > >+ *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src; > > > > Same here. > > I generally prefer not to have overly-long for-initializers. I think it makes > this loop more readable, since the loop itself only talks about the loop > variable and its bounds. I saw after I made the comment that "Same here" bound to the wrong comment -- not to the != versus < comment I'd intended it to bind to, before adding an intervening comment. So what I really meant (and hoped you'd intuit without having to burn a comment on it) was s/!=/</ and nothing more here. Sorry for the confusion...

Comment on attachment 486802 [details] [diff] [review] factor/optimize apply 3 Now GetElement/GetElements are inconsistent with SetOrDeleteArrayElement and SetArrayElement. :-D Please change them as well (no need for me to review those changes). >diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp >+ for (Value *dst = vp, *src = srcbeg; src != srcend; ++dst, ++src) >+ *dst = src->isMagic(JS_ARRAY_HOLE) ? UndefinedValue() : *src; < rather than != still. >+ } else { >+ Value *srcbeg = aobj->getArgsElements(); >+ Value *srcend = srcbeg + length; >+ for (Value *dst = vp, *src = srcbeg; src < srcend; ++dst, ++src) >+ *dst = src->isMagic(JS_ARGS_HOLE) ? UndefinedValue() : *src; ...but you fixed this one, good. :-) r=me with the s/!=/</ and with the naming-consistency changes.

Comment on attachment 486280 [details] [diff] [review] apply ic >+ >+ /* >+ * Allocate nvals on the top of the stack, report error on failure. >+ * N.B. the caller must ensure |from == firstUnused()|. Is this now >= firstUnused() ? > DataLabelPtr assemble(void *ncode) ... >+ } else { >+ RegisterID newfp = tempRegs.takeAnyReg(); >+ masm.loadPtr(FrameAddress(offsetof(VMFrame, regs.sp)), newfp); Comment here that SplatApplyArgs has been called, and regs.sp has been bumped to where the new JSStackFrame should lie. >+ static bool isSaved(RegisterID reg) { >+ uint32 mask = (1 << reg); Instead, do Registers::maskReg(reg) >+ RegisterID argcReg = (RegisterID)-1; /* sssh gcc */ Prefer MaybeRegisterID for this. >+ if (ic.frameSize.isStatic()) >+ masm.move(Imm32(ic.frameSize.staticArgc()), Registers::ArgReg1); >+ else >+ masm.move(argcReg, Registers::ArgReg1); else if (argcReg != Registers::ArgReg1) Nice patch! I like how it didn't require too many changes.

http://hg.mozilla.org/tracemonkey/rev/e77069ddab00 http://hg.mozilla.org/tracemonkey/rev/fd21a9d4344a http://hg.mozilla.org/tracemonkey/rev/8715628a75f8 http://hg.mozilla.org/tracemonkey/rev/d9aceaabef28

Landed a while ago: http://hg.mozilla.org/mozilla-central/rev/e77069ddab00 http://hg.mozilla.org/mozilla-central/rev/fd21a9d4344a http://hg.mozilla.org/mozilla-central/rev/8715628a75f8 http://hg.mozilla.org/mozilla-central/rev/d9aceaabef28