Go into slow case when encountering object initialization on the top level to...

Go into slow case when encountering object initialization on the top level to optimize performance of code like
C.prototype.x = ...;
C.prototype.y = ...;
...
C.prototype.z = ...;

Committed: http://code.google.com/p/v8/source/detail?r=1372

[iposva, 2009-02-25 12:54:45]

Drive by comments.

-Ivan

http://codereview.chromium.org/27128/diff/1/5
File src/parser.cc (right):

http://codereview.chromium.org/27128/diff/1/5#newcode1269
Line 1269: if (assignment->op() != Token::ASSIGN) return false;
You check here that only ASSIGN statements are added when the block continues,
but you do not check for the initial node to be an ASSIGN statement anywhere I
can tell.

http://codereview.chromium.org/27128/diff/1/5#newcode1304
Line 1304: bool InitializationBlockFinder::SameObject(Expression* e1,
Expression* e2) {
What made you put this method outside the class declaration? Since this is a
private class I would just put all of the code in the declaration, or at least
pull all non-trivial (aka 1-liners) out to be consistent.

http://codereview.chromium.org/27128/diff/1/5#newcode1307
Line 1307: if (v1 != NULL && v2 != NULL) {
In general I find code more readable when () are added around boolean
expressions such as

if ((v1 != NULL) && (v2 != NULL))

[Mads Ager (chromium), 2009-02-25 13:11:07]

LGTM

You should add some tests though.  I'm not sure that any of our test code hits
this.  Please create a number of tests for different kinds of top-level
assignment sequences.

http://codereview.chromium.org/27128/diff/1/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/27128/diff/1/6#newcode2764
Line 2764: // Changing to slow case in the beginning of an initialization block
Change to slow case ... to avoid ...

or

Changing to slow case ... avoids ...

?

http://codereview.chromium.org/27128/diff/1/5
File src/parser.cc (right):

http://codereview.chromium.org/27128/diff/1/5#newcode74
Line 74: static const int kMinInitializationBlock = 3;
Is this limit based on benchmarking?  We should make sure to only perform the
two runtime calls if that overhead is less than the instance descriptor copying
overhead.

http://codereview.chromium.org/27128/diff/1/5#newcode1242
Line 1242: if (!InBlock() && (assignment != NULL)) StartBlock(assignment);
In BlockContinues, you check if the operation is Token::ASSIGN.  You should do
that here (or in StartBlock) as well?

Please add a test case that hits this.

http://codereview.chromium.org/27128/diff/1/5#newcode1345
Line 1345: // degradation in any other scopes.
You should write here that the reason for only doing this in top level code is
that map transitions are not used in this case leading to different maps for all
objects going through this series of assignments.

[olehougaard, 2009-02-26 07:31:44]

I've added a unit test to show we haven't messed up top-level assignments in
addition to fixing the issues below.

http://codereview.chromium.org/27128/diff/1/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/27128/diff/1/6#newcode2764
Line 2764: // Changing to slow case in the beginning of an initialization block
On 2009/02/25 13:11:07, Mads Ager wrote:
> Change to slow case ... to avoid ...
> 
> or
> 
> Changing to slow case ... avoids ...
> 
> ?

Fixed

http://codereview.chromium.org/27128/diff/1/5
File src/parser.cc (right):

http://codereview.chromium.org/27128/diff/1/5#newcode74
Line 74: static const int kMinInitializationBlock = 3;
On 2009/02/25 13:11:07, Mads Ager wrote:
> Is this limit based on benchmarking?  We should make sure to only perform the
> two runtime calls if that overhead is less than the instance descriptor
copying
> overhead.

It's based on benchmarking. I've added a comment to that effect.

http://codereview.chromium.org/27128/diff/1/5#newcode1242
Line 1242: if (!InBlock() && (assignment != NULL)) StartBlock(assignment);
On 2009/02/25 13:11:07, Mads Ager wrote:
> In BlockContinues, you check if the operation is Token::ASSIGN.  You should do
> that here (or in StartBlock) as well?
> 
> Please add a test case that hits this.

Fixed.

http://codereview.chromium.org/27128/diff/1/5#newcode1269
Line 1269: if (assignment->op() != Token::ASSIGN) return false;
On 2009/02/25 12:54:45, iposva wrote:
> You check here that only ASSIGN statements are added when the block continues,
> but you do not check for the initial node to be an ASSIGN statement anywhere I
> can tell.

Fixed.

http://codereview.chromium.org/27128/diff/1/5#newcode1304
Line 1304: bool InitializationBlockFinder::SameObject(Expression* e1,
Expression* e2) {
On 2009/02/25 12:54:45, iposva wrote:
> What made you put this method outside the class declaration? Since this is a
> private class I would just put all of the code in the declaration, or at least
> pull all non-trivial (aka 1-liners) out to be consistent.

I've put all the code in the declaration for consistency.

http://codereview.chromium.org/27128/diff/1/5#newcode1307
Line 1307: if (v1 != NULL && v2 != NULL) {
On 2009/02/25 12:54:45, iposva wrote:
> In general I find code more readable when () are added around boolean
> expressions such as
> 
> if ((v1 != NULL) && (v2 != NULL))
> 
> 

Fixed where found.

http://codereview.chromium.org/27128/diff/1/5#newcode1345
Line 1345: // degradation in any other scopes.
On 2009/02/25 13:11:07, Mads Ager wrote:
> You should write here that the reason for only doing this in top level code is
> that map transitions are not used in this case leading to different maps for
all
> objects going through this series of assignments.

Fixed.