1030985 - Optimize arguments.callee for Shumway

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

It would be useful for Shumway codegen to have fast access to arguments.callee (like arguments.length) so properties can be hung off the function objects.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Optimize arguments.callee.

See first comment for motivation.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Optimize arguments.callee.

Accidentally left in dead code.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Optimize arguments.callee.

Oops, sorry for the churn. I was handling errors incorrectly in the refactored
isDefinitelyOptimizedArguments.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8446921 [details] [diff] [review]
Optimize arguments.callee.

Review of attachment 8446921 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! This should indeed speed things up quite a bit for Shumway.

Changing reviewer because bhackett is on vacation.

::: js/src/jit/BaselineIC.cpp
@@ +6086,5 @@
>      }
>  
> +    if (!TryAttachMagicArgumentsGetPropStub(cx, script, stub, val, res, attached))
> +        return false;
> +    if (attached)

`if (*attached)`
Also, could use the `if (!TryAttach... || *attached) return *attached;` pattern here.

@@ +6383,5 @@
>      FallbackICSpew(cx, stub, "GetProp(%s)", js_CodeName[op]);
>  
>      JS_ASSERT(op == JSOP_GETPROP || op == JSOP_CALLPROP || op == JSOP_LENGTH || op == JSOP_GETXPROP);
>  
>      RootedPropertyName name(cx, frame->script()->getName(pc));

Without the added support for .callee, this rooted could have been moved down below the fast path. That gets a lot more annoying now, but I wonder if it might still be useful.

::: js/src/jit/IonBuilder.cpp
@@ +8705,5 @@
> +    if (!isOptimizedArgs)
> +        return true;
> +
> +    if (name != names().callee)
> +        return true;

Does it make sense to move this check above the more-expensive one?

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8446921 [details] [diff] [review]
Optimize arguments.callee.

Review of attachment 8446921 [details] [diff] [review]:
-----------------------------------------------------------------

The Baseline implementation really bothers me.

- TryAttachLengthStub should only contain code for JSOP_LENGTH. In your implementation it calls TryAttachMagicArgumentsGetPropStub and that has some specialized code for ".callee", without checking it is not JSOP_LENGTH. That feels really error prone.

- TryAttachMagicArgumentsGetPropStub is called for for "length" and one for "callee". But you don't distinguish these cases in "TryAttachMagicArgumentsGetPropStub". "callee" could create a "MagicArgs.length" stub and "length" a "MagicArgs.callee" stub. This also feels error prone.

::: js/src/jit/IonBuilder.cpp
@@ +8705,5 @@
> +    if (!isOptimizedArgs)
> +        return true;
> +
> +    if (name != names().callee)
> +        return true;

The "checkIsDefinitelyOptimizedArguments" is not really expensive.

::: js/src/vm/Interpreter.cpp
@@ +225,5 @@
> +
> +    if (id == NameToId(cx->names().callee) && IsOptimizedArguments(fp, lval.address())) {
> +        vp.setObject(fp->callee());
> +        return true;
> +    }

Does it really matters to add this? We only run this a few times, before baseline kicks in.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> Comment on attachment 8446921 [details] [diff] [review]
> Optimize arguments.callee.
> 
> Review of attachment 8446921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The Baseline implementation really bothers me.
> 
> - TryAttachLengthStub should only contain code for JSOP_LENGTH. In your
> implementation it calls TryAttachMagicArgumentsGetPropStub and that has some
> specialized code for ".callee", without checking it is not JSOP_LENGTH. That
> feels really error prone.
> 

Good point, I'll refactor this.

> - TryAttachMagicArgumentsGetPropStub is called for for "length" and one for
> "callee". But you don't distinguish these cases in
> "TryAttachMagicArgumentsGetPropStub". "callee" could create a
> "MagicArgs.length" stub and "length" a "MagicArgs.callee" stub. This also
> feels error prone.

The cases are distinguished by the type of the result. It's not possible for arguments.callee to return an int32 or for arguments.length to return an object.

> 
> ::: js/src/jit/IonBuilder.cpp
> @@ +8705,5 @@
> > +    if (!isOptimizedArgs)
> > +        return true;
> > +
> > +    if (name != names().callee)
> > +        return true;
> 
> The "checkIsDefinitelyOptimizedArguments" is not really expensive.
> 
> ::: js/src/vm/Interpreter.cpp
> @@ +225,5 @@
> > +
> > +    if (id == NameToId(cx->names().callee) && IsOptimizedArguments(fp, lval.address())) {
> > +        vp.setObject(fp->callee());
> > +        return true;
> > +    }
> 
> Does it really matters to add this? We only run this a few times, before
> baseline kicks in.

Most definitely. The lazy args analysis affects the interpreter too. If we stop creating ArgumentsObject when there is only a use of .callee, the Interpreter will assert when trying to run getprop "callee" on MagicValue(JS_OPTIMIZED_ARGUMENTS).

I'll upload new patch soon.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Optimize arguments.callee.

Applied review comments.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8447428 [details] [diff] [review]
Optimize arguments.callee.

Review of attachment 8447428 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay. This is indeed already better. 
But DoGetPropFallback is already ugly. Let's try ameliorate that:

1) Remove the (op == JSOP_LENGTH || name == cx->names().callee) check from [1]
2) Move all logic that is inside the branch [1] to TryAttachMagicArgumentsGetPropStub
3) Do one test of "op == JSOP_LENGTH" and one for the "name == ...". So group together.

So something like:

TryAttachMagicArgumentsGetPropStub(...)
{
   if (name == cx->names().callee) {
       // 1. Get res from *frame->callee();
       // 2. Monitor result
       // 3. Generate GetProp(MagicArgs.callee)

       return true;
   }

   if (op == JSOP_LENGTH) {
       // 1. Get res from *frame->num...
       // 2. Monitor result
       // 3. TryAttachLengthStub();

       return true;
   }
}

You will need to adjust the arguments given to TryAttachMagicArgumentsGetPropStub.

(Since only refactoring is needed, already giving r+. Feel free to request another r? if unsure.)

::: js/src/jit/BaselineIC.cpp
@@ +6382,5 @@
>  
> +    if ((op == JSOP_LENGTH || name == cx->names().callee) &&
> +        val.isMagic(JS_OPTIMIZED_ARGUMENTS) &&
> +        IsOptimizedArguments(frame, val.address()))
> +    {

[1]

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33082e3bfed9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out (alongside bug 1030985's patch) under suspicion of breaking Android xpcshell tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=43281523&full=1&branch=mozilla-inbound#error0


https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa782aab36d


I'm not sure which bug is at fault because the test got coalesced away, but I don't want to wait the 75 minutes it'll take to get the test retriggered and finished.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 978211's retrigger came back green, so I relanded it. Looks like it was this patch.

Comment 12

[Shu-yu Guo [:shu]]

I have no idea what's causing that indexedDB test to fail.

Running locally and diffing logs, it seems that bent's guess was correct that the NotFoundError is being raised here: http://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js?from=test_temporary_storage.js&case=true#203

I found this out by commenting out stage 4. Stages 1-3 result in 108 checks being run, which is what the failure log linked in c10 shows.

Not sure how to proceed, since this is pretty bizarre failure to have caused by this patch.

Comment 13

[Shu-yu Guo [:shu]]

Ben, can you suggest instrumentation to figure out why db.transaction is throwing that error in that test?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBDatabase.cpp#665 is the only place where we should be throwing that... I guess I'd first see if the Sequence<> argument is valid and dump it every time, see if there's something funny about it?

Comment 15

[Shu-yu Guo [:shu]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBDatabase.
> cpp#665 is the only place where we should be throwing that... I guess I'd
> first see if the Sequence<> argument is valid and dump it every time, see if
> there's something funny about it?

From reading the code, it seems like that error could be thrown at any time we couldn't successfully find the object store, like http://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp?from=IDBTransaction.cpp#714

I guess it's also possible that it's  http://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js?from=test_temporary_storage.js&case=true#206 that's throwing. Could be anything before the checkUsage(4) line, but the only likely candidates seem to be db.transaction() and trans.objectStore().

Comment 16

[Shu-yu Guo [:shu]]

I'm fairly convinced that the error is a GCC 4.7 bug in the ARMv6 codegen. I can't figure out what the problem is but I've devised a workaround. If I heap allocate (wrapped in a ScopedJSDeletePtr) instead of stack allocate ICGetProp_ArgumentsCallee::Compiler, the failing indexedDB test now passes.

Try here: https://tbpl.mozilla.org/?tree=Try&rev=9654855a39af

Comment 17

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1546c3578328

Comment 18

[Till Schneidereit [:till]]

Apparently, the sheriffs forgot to close this after merging.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

It's an mcMerge bug - see bug 1062305.