842522 - Don't force construction of arguments objects on dynamic name accesses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, if a script has dynamic name accesses then it will require an arguments object.  In many cases this is overly pessimistic.  Even if a script has dynamic name accesses, if there is no use of 'arguments' within the script then there is no way an arguments object will be necessary.  For scripts containing eval(), where the use of 'arguments' cannot be statically ruled out, the arguments object still does not always have to be constructed.  A binding can be introduced for 'arguments' and filled with the lazy arguments value, and if the eval script contains any uses of arguments then the arguments optimization can just be marked as having failed, in the same fashion as when a foo.apply(arguments) does not use the native apply.

The requirement for arguments objects is one of the two things holding back Ion compilation of the main function in ss-date-format-tofte, see bug 743394.  This change seems preferable to constructing arguments objects in Ion because the latter seems both pretty complicated and slow.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 715461 [details] [diff] [review]
patch

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

Hah, nice!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1287,5 @@
>              /*
> +             * Force construction of arguments objects for functions that
> +             * use 'arguments' within an eval.
> +             */
> +            if (pn->pn_atom == cx->names().arguments && caller.isFunctionFrame()) {

I'm thinking about the case of nested eval:
  function f() { eval("eval('arguments')") }
So, iirc, 'caller' (of the innermost eval) will be isFunctionFrame(), however caller.script() will be the script of outer 'eval'.  I think you can then just change it to caller.fun()->script().  Could you a test-case that hits this case (and a JS_ASSERT(script->function())) to JSScript::argumentsOptimizationFailed).

::: js/src/frontend/Parser.cpp
@@ +880,5 @@
>           * might, in the case of calls to eval) be assigned.
>           */
>          if (pc->sc->needStrictChecks()) {
> +            if (pc->sc->bindingsAccessedDynamically())
> +                funbox->setDefinitelyNeedsArgsObj();

Could you add a comment here as to why this is necessary?

::: js/src/jsscript.cpp
@@ +2814,5 @@
> +        jsbytecode *pc = script->code;
> +        while (*pc != JSOP_ARGUMENTS)
> +            pc += GetBytecodeLength(pc);
> +        pc += JSOP_ARGUMENTS_LENGTH;
> +        JS_ASSERT(*pc == JSOP_SETALIASEDVAR);

Hmm.  Normally (in all the other places where we have to read/write an aliased variable), the AliasedFormalIter is used.  Of course, 'arguments' here is not a formal, so we'd need an AliasedLocalIter.  It seems silly to add the whole iterator for this one use which you've already solved directly enough.  If this problem comes up a second time (needing to assign to an arbitrary aliased local), we should add AliasedLocalIter, though.  It'd be pretty simple.

Comment 2

[Luke Wagner [:luke]]

Random question: is the eval'd code doing any hot access of free variables (via JSOP_NAME/SETNAME)?  If so, we could be a little more aggressive in emitting GNAME or ALIASEDVAR ops by actually looking at the bindings of the enclosing functions.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #2)
> Random question: is the eval'd code doing any hot access of free variables
> (via JSOP_NAME/SETNAME)?  If so, we could be a little more aggressive in
> emitting GNAME or ALIASEDVAR ops by actually looking at the bindings of the
> enclosing functions.

At least for date-format-tofte, no.  The evals are all of the form 'foo()' where foo is some name in the scope chain, so eval is basically being used for doing dynamic indexes of the scope.  Once Ion is compiling JSOP_EVAL I think speeding up this pattern is the next step.

A similar concern is whether to compile NAMEs in the outer functions as if they are ALIASEDVAR/GNAME even within the outer functions, and deoptimize the compiled code only if the eval() actually introduces new bindings.  I modified the parser/emitter to date-format-tofte to test this but didn't see any improvement.  I might have messed something up though, can look again once this other stuff is in.

Comment 4

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> At least for date-format-tofte, no.  The evals are all of the form 'foo()'
> where foo is some name in the scope chain, so eval is basically being used
> for doing dynamic indexes of the scope.

Whoa, interesting.  Looking at tofte, it is literally written:
  eval(ia[ij] + "()");

This suggests some form of pattern matching on "evals that are really dynamic calls".  Do you remember we had someone visiting researcher who had done an analysis and found that the majority of all evals are trivial things like "o.i" (people don't know they can write o[str]), and this sounds like another one.  Have you considered pattern matching calls in EvalKernel, right next to where we pattern match JSON?

Even more work but potentially more profitable would be to take advantage of the expression (ia[ij] + "()").  In theory, we could do a simple decomposition in the frontend to bytecodes that checked whether the lhs of the concat was an identifier and, if so, did the dynamic lookup/call.

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #4)
> This suggests some form of pattern matching on "evals that are really
> dynamic calls".  Do you remember we had someone visiting researcher who had
> done an analysis and found that the majority of all evals are trivial things
> like "o.i" (people don't know they can write o[str]), and this sounds like
> another one.  Have you considered pattern matching calls in EvalKernel,
> right next to where we pattern match JSON?

This is roughly the plan.  At least for Ion (which, when used with baseline, will be ~90% of the time on tofte) this can be a separate VM function, as in that case the stub can just do the dynamic name lookup and return a function for the caller to invoke rather than doing the invoke in C++.

> Even more work but potentially more profitable would be to take advantage of
> the expression (ia[ij] + "()").  In theory, we could do a simple
> decomposition in the frontend to bytecodes that checked whether the lhs of
> the concat was an identifier and, if so, did the dynamic lookup/call.

Yeah, it's definitely possible to do basically the entire thing in jitcode, but how much is appropriate remains to be seen.  I'm hoping that most of the overhead is from using an eval script and doing C++ invokes, similar to what happened with string-unpack-code way back when.

Comment 6

[Luke Wagner [:luke]]

Sounds good

Comment 7

[Brian Hackett [Laid off!]]

Pushed, with some changes for JSOP_DEBUGGER.  Since the debugger can inspect anything along the scope chain, its presence forces construction of arguments objects in all scopes it is nested in (as was previously done for all dynamic name accesses).

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3b899354a6f