[webkit-reviews] review granted: [Bug 39200] [DO NOT REVIEW]: Simplified handling of 'arguments' -- 1.2% speedup : [Attachment 56215] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 39200: [DO NOT REVIEW]: Simplified handling of 'arguments' -- 1.2% speedup
https://bugs.webkit.org/show_bug.cgi?id=39200

Attachment 56215: patch
https://bugs.webkit.org/attachment.cgi?id=56215&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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.