Bug 21123 – using "arguments" in a function should not force creation of an activation object

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

This will make some tests much faster.

[Darin Adler]

Created attachment 23827 [details]
work in progress

[Geoffrey Garen]

Darin wanted to know how to emit an initial "op_make_an_arguments_object" at the end of codegen. Here's an example of something I've done in a work-in-progress patch:

[in CodeGenerator::generate]

    if (m_codeType == FunctionCode && m_codeBlock->needsFullScopeChain) {
        ASSERT(globalData()->machine->getOpcodeID(m_codeBlock->instructions[0].u.opcode) == op_init);
        m_codeBlock->instructions[0] = globalData()->machine->getOpcode(op_init_activation);
    }

[Geoffrey Garen]

(I had previously suggested reserving extra space at the beginning of the instruction buffer, but that turns out to be tricky, because at least exception info and structureID info have already been emitted with assumptions about opcode positions, and lots of code directly accesses the instruction buffer. The solution above is simpler.)

[Geoffrey Garen]

...I can just finish off Darin's patch once I've finished my own.

[Maciej Stachowiak]

I would like to finish this off perhaps with collaboration from cpst - I think I have a pretty clear picture of how to do it well. 

I think op_make_an_arguments_object could be emitted at the start of codegen, not the end, since we already track for local use of arguments.

[Cameron Zwarich (cpst)]

I'll assign this to myself, since I am working on bug 21200.

[Cameron Zwarich (cpst)]

I landed the devirtualization of JSObject::isActivationObject() separately as r37068.

[Cameron Zwarich (cpst)]

I just realized that the fact that one can write to arguments makes this patch a little bit more difficult. You can't just blindly tear off any 'arguments' object that is in the OptionalCalleeArguments slot, because it may be the 'arguments' object of another function.

Geoff proposed a solution, which is to make a local variable for the arguments object, separate from the slot in the call frame, which is actually stored in the symbol table. It's a bit of a waste, but there isn't really any other way to do it in light of eval.

[Cameron Zwarich (cpst)]

Created attachment 23933 [details]
Patch in progress

Here is my latest patch in progress. It is a 19% speedup on the V8 Raytrace test. It currently has two problems that prevent it from landing:

1) The issue of tearing off a different arguments object mentioned above.

2) Some slight simplification of codegen for op_ret.

The first will require some changes to detect 'arguments' entirely during parsing, so I can always put the mutable version of 'arguments' in the first local. I will write that part separately and post it with some tests for review.

[Cameron Zwarich (cpst)]

Created attachment 23952 [details]
Patch to move lexical 'arguments' detection to the parser

I need this in order to fix the problem with ambiguity of 'arguments' objects in tear-off.

[Geoffrey Garen]

Please add tests for declaring a function named "arguments."

+shouldBeTrue("assign_var_init_test()");

Despite a few strays in JavaScriptCore, the WebKit style is to use camelCase: assignVarInitTest, etc.

You don't need to track the use of 'arguments' in a FunctionExpr, since that use only determines the function's name, and not its symbol in local scope.

Please write a test case that verifies that "(function (arguments) { return arguments; })(1)" returns 1.

Please write a test case that verifies that "(function f(arguments) { return (function () { return f.arguments; })(); })(1)" returns 1. I believe this is broken now. The way to fix it is to use the symbol table. (It's OK to check the test in now with results that say "These results are incorrect.")

r=me with the changes above.

[Geoffrey Garen]

Comment on attachment 23952 [details]
Patch to move lexical 'arguments' detection to the parser

r=me

[Cameron Zwarich (cpst)]

Comment on attachment 23952 [details]
Patch to move lexical 'arguments' detection to the parser

This landed as r37117, so I will clear the review flag.

[Cameron Zwarich (cpst)]

Created attachment 23966 [details]
Patch in progress

Here is a new version of this patch. I made a final version that fixes all of the corner cases, but I noticed that it is only an 8% speedup rather than a 19% speedup.

This isn't that final version; rather it is an updated version of the previous patch with the line that regresses performance commented out:

+        // addVar(propertyNames().UTC, false);

I picked UTC because it doesn't appear in the Raytrace test. I will also post my newest patch that fixes all of the corner cases.

[Maciej Stachowiak]

I figured out the problem with using an extra local. Arguments::copyRegisters copies local var registers as well as param registers, but it has no need to. The hot arguments-using function in Raytrace happened to use no local vars, so allocating a local for "arguments" created the first, thus forcing a completely useless allocation for register copy. This version of Arguments::copyRegisters solves that problem and actually results in a patch that is even faster than the fast original:

    inline void Arguments::copyRegisters()
    {
        ASSERT(!d->activation);
        ASSERT(!d->registerArray);

        size_t numParametersMinusThis = d->callee->m_body->generatedByteCode().numParameters - 1;

        if (!numParametersMinusThis)
            return;

        int registerOffset = numParametersMinusThis + RegisterFile::CallFrameHeaderSize;
        size_t registerArraySize = numParametersMinusThis;

        Register* registerArray = new Register[registerArraySize];
        memcpy(registerArray, d->registers - registerOffset, registerArraySize * sizeof(Register));
        d->registerArray.set(registerArray);
        d->registers = registerArray + registerOffset;
    }

[Cameron Zwarich (cpst)]

Created attachment 23983 [details]
Proposed patch

[Darin Adler]

Comment on attachment 23983 [details]
Proposed patch

+            m_jit.movl_mr(RegisterFile::OptionalCalleeActivation * static_cast<int>(sizeof(Register)), X86::edi, X86::eax);
+            m_jit.orl_mr(RegisterFile::OptionalCalleeArguments * static_cast<int>(sizeof(Register)), X86::edi, X86::eax);

I'm disappointed that we have to make the fast case slower here by examining both of these. I wonder if some day we can find a way to make it faster.

+    } else if (JSValue* arguments = r[RegisterFile::OptionalCalleeArguments].getJSValue()) {
+        if (arguments->isObject(&Arguments::info))
+            static_cast<Arguments*>(arguments)->copyRegisters();
+    }

Why is this isObject check needed? When does OptionalCalleeArguments contains something that's non-zero but is not an Arguments object?

+                                        { $$ = createNodeFeatureInfo<PropertyNode*>(makeGetterOrSetterPropertyNode(globalPtr, *$1, *$2, $4.m_node.head, $7, LEXER->sourceRange($6, $8)), $4.m_featureInfo | ClosureFeature, 0); DBG($7, @6, @8); $7->setUsesArguments($7->usesArguments() | ($4.m_featureInfo & ArgumentsFeature)); if (!$$.m_node) YYABORT; }

I'd expect to have the new code be before the DBG rather than after. As these lines of code get super-long, I'd interested in having some of this logic move into a helper function so it's not all on one giant source line in the grammar. Or some other way of formatting this with multiple lines so it's easier to read.

This is a great speedup!

r=me

[Cameron Zwarich (cpst)]

(In reply to comment #17)
> (From update of attachment 23983 [details] [edit])
> +            m_jit.movl_mr(RegisterFile::OptionalCalleeActivation *
> static_cast<int>(sizeof(Register)), X86::edi, X86::eax);
> +            m_jit.orl_mr(RegisterFile::OptionalCalleeArguments *
> static_cast<int>(sizeof(Register)), X86::edi, X86::eax);
>
> I'm disappointed that we have to make the fast case slower here by examining
> both of these. I wonder if some day we can find a way to make it faster.

We are looking at removing all branches from op_ret in the next few days, so hopefully it happens. I will be making separate bugs for this.
 
> +    } else if (JSValue* arguments =
> r[RegisterFile::OptionalCalleeArguments].getJSValue()) {
> +        if (arguments->isObject(&Arguments::info))
> +            static_cast<Arguments*>(arguments)->copyRegisters();
> +    }
> 
> Why is this isObject check needed? When does OptionalCalleeArguments contains
> something that's non-zero but is not an Arguments object?

Sorry! This is from an earlier version of my patch. As I mentioned in the ChangeLog, OptionalCalleeArguments always contains 0 or an Arguments object. I changed these to assertions using Arguments::info before I landed.

> +                                        { $$ =
> createNodeFeatureInfo<PropertyNode*>(makeGetterOrSetterPropertyNode(globalPtr,
> *$1, *$2, $4.m_node.head, $7, LEXER->sourceRange($6, $8)), $4.m_featureInfo |
> ClosureFeature, 0); DBG($7, @6, @8); $7->setUsesArguments($7->usesArguments() |
> ($4.m_featureInfo & ArgumentsFeature)); if (!$$.m_node) YYABORT; }
> 
> I'd expect to have the new code be before the DBG rather than after. As these
> lines of code get super-long, I'd interested in having some of this logic move
> into a helper function so it's not all on one giant source line in the grammar.
> Or some other way of formatting this with multiple lines so it's easier to
> read.

I moved it before the DBG, but I agree we should format this (and other code in the parser) in a much better way.

This was landed in r37160.