Closed Bug 605192 Opened 9 years ago Closed 9 years ago

JM: make f.apply(x, obj) fast

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Attached patch factor/optimize apply (obsolete) — Splinter Review
This patch has preliminary changes to js_fun_apply that simplify the next patch.
Attached patch WIP 1 (obsolete) — Splinter Review
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.
Attached patch WIP 2 (obsolete) — Splinter Review
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.)
Attachment #484893 - Attachment is obsolete: true
x64 was only providing a single SavedReg.  This patch makes ValueReg use r10 (a temp reg) which frees up r15.
Attachment #486000 - Flags: review?(sstangl)
Attached patch WIP 3 (obsolete) — Splinter Review
Attachment #484979 - Attachment is obsolete: true
Attachment #486000 - Flags: review?(sstangl) → review+
Attachment #484890 - Flags: review?(dvander)
Attached patch factor/optimize apply 2 (obsolete) — Splinter Review
Attachment #484891 - Attachment is obsolete: true
Attachment #486279 - Flags: review?(jwalden+bmo)
Attached patch apply icSplinter Review
Refactored and green on try.
Attachment #486001 - Attachment is obsolete: true
Attachment #486280 - Flags: review?(dvander)
Attachment #484890 - Flags: review?(dvander) → review+
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.  :-)
Attachment #486279 - Flags: review?(jwalden+bmo) → review-
(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.
Attachment #486279 - Attachment is obsolete: true
Attachment #486802 - Flags: review?(jwalden+bmo)
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.
Attachment #486802 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #486280 - Flags: review?(dvander) → review+
Depends on: 610038
Depends on: 614752
You need to log in before you can comment on or make changes to this bug.