Bug 39200 - Simplified handling of 'arguments' -- 1.2% SunSpider speedup
Summary: Simplified handling of 'arguments' -- 1.2% SunSpider speedup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-17 00:30 PDT by Geoffrey Garen
Modified: 2010-05-18 22:10 PDT (History)
2 users (show)

See Also:


Attachments
patch (42.20 KB, patch)
2010-05-17 00:30 PDT, Geoffrey Garen
darin: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2010-05-17 00:30:51 PDT
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.
Comment 1 WebKit Review Bot 2010-05-17 00:33:25 PDT
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.
Comment 2 Darin Adler 2010-05-17 14:50:42 PDT
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.
Comment 3 Darin Adler 2010-05-17 15:08:26 PDT
Geoff told me it was fine to review.
Comment 4 Geoffrey Garen 2010-05-18 22:10:33 PDT
Committed revision 59742.