Bug 39200 – Simplified handling of 'arguments' -- 1.2% SunSpider speedup

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

[Geoffrey Garen]

Created attachment 56215 [details]
patch

About a 9%-11% speedup in raw call performance.

No ChangeLog or tests yet. Feel free to leave comments, but please don't mark as reviewed until the EWS bots have cycled.

[WebKit Review Bot]

Attachment 56215 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/bytecode/CodeBlock.h:390:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/bytecode/CodeBlock.h:391:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 56215 [details]
patch

> +        void setArgumentsRegister(int argumentsRegister) { m_argumentsRegister = argumentsRegister; ASSERT(m_argumentsRegister != -1); }

I would have the assertion assert based on the argument instead of the data member because the mistake is passing the wrong value, and having the assertion mention the argument name points to the mistake ever so slightly more directly.

> +        addVar(); // Hidden reference to arguments object, used for tear-off
> +        m_argumentsRegisterIndex = addVar(propertyNames().arguments, false)->index(); // User-visible arguments property

What's missing from these comments is the point that the user-visible property can change, but the tear-off must use the original value. That's obvious to you but may not be obvious to the person reading this later.

The other non-obvious thing about the code is how the tear-off finds the hidden reference. Mechanically speaking, what's the excuse for ignoring the return value from addVar.

The way to write this in the code is to have a local variable and use ASSERT_UNUSED to assert its relationship to m_argumentsRegisterIndex.

It would look something like this:

    Register* unmodifiedArgumentsRegister = addVar();
    m_argumentsRegisterIndex = addVar(xxx)->index();
    codeBlock->setArgumentsRegister(m_argumentsRegisterIndex);

    // We can save a little storage by hard-coding the knowledge that the two arguments values
    // are stored in consecutive registers, and storing only the index of the visible one.
    ASSERT_UNUSED(unmodifiedArgumentsRegister, unmodifiedArgumentsRegister->index() == codeBlock->unmodifiedArgumentsRegister());

I think that's better than the addVar with ignored result and comments in the header that aren't obviously directly connected.

I also suggest getting rid of m_argumentsRegisterIndex. I think that everywhere it's used we could instead use codeBlock->argumentsRegister().

As we discussed in person, we should add an inline function to map from an arguments register index to the unmodified arguments register's index to CodeBlock.h. Even though this is a simple "- 1", it's great to have all the code that does this call a single function where the comment can indicate what's going on.

r=me -- this seems great as is and will be slightly better if you use a function do all that "-1"ing.

[Darin Adler]

Geoff told me it was fine to review.

[Geoffrey Garen]

Committed revision 59742.