Bug 21123 - using "arguments" in a function should not force creation of an activation object
Summary: using "arguments" in a function should not force creation of an activation ob...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks: 20813
  Show dependency treegraph
 
Reported: 2008-09-25 16:06 PDT by Darin Adler
Modified: 2008-10-01 16:47 PDT (History)
3 users (show)

See Also:


Attachments
work in progress (24.11 KB, patch)
2008-09-25 16:06 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch in progress (25.52 KB, patch)
2008-09-30 00:45 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch to move lexical 'arguments' detection to the parser (23.35 KB, patch)
2008-09-30 13:36 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch in progress (26.24 KB, patch)
2008-09-30 22:21 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (42.57 KB, patch)
2008-10-01 12:04 PDT, Cameron Zwarich (cpst)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2008-09-25 16:06:14 PDT
This will make some tests much faster.
Comment 1 Darin Adler 2008-09-25 16:06:56 PDT
Created attachment 23827 [details]
work in progress
Comment 2 Geoffrey Garen 2008-09-25 16:26:49 PDT
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);
    }

Comment 3 Geoffrey Garen 2008-09-25 16:28:44 PDT
(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.)
Comment 4 Geoffrey Garen 2008-09-25 17:23:58 PDT
...I can just finish off Darin's patch once I've finished my own.
Comment 5 Maciej Stachowiak 2008-09-26 09:15:21 PDT
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.
Comment 6 Cameron Zwarich (cpst) 2008-09-28 11:57:59 PDT
I'll assign this to myself, since I am working on bug 21200.
Comment 7 Cameron Zwarich (cpst) 2008-09-29 13:10:07 PDT
I landed the devirtualization of JSObject::isActivationObject() separately as r37068.
Comment 8 Cameron Zwarich (cpst) 2008-09-29 14:41:55 PDT
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.
Comment 9 Cameron Zwarich (cpst) 2008-09-30 00:45:50 PDT
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.
Comment 10 Cameron Zwarich (cpst) 2008-09-30 13:36:48 PDT
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.
Comment 11 Geoffrey Garen 2008-09-30 13:55:30 PDT
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.
Comment 12 Geoffrey Garen 2008-09-30 13:55:46 PDT
Comment on attachment 23952 [details]
Patch to move lexical 'arguments' detection to the parser

r=me
Comment 13 Cameron Zwarich (cpst) 2008-09-30 15:20:34 PDT
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.
Comment 14 Cameron Zwarich (cpst) 2008-09-30 22:21:58 PDT
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.
Comment 15 Maciej Stachowiak 2008-10-01 04:20:59 PDT
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;
    }

Comment 16 Cameron Zwarich (cpst) 2008-10-01 12:04:14 PDT
Created attachment 23983 [details]
Proposed patch
Comment 17 Darin Adler 2008-10-01 13:09:58 PDT
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
Comment 18 Cameron Zwarich (cpst) 2008-10-01 16:47:30 PDT
(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.