Optimizing generation of nested literals for both object and array literals.

Optimizing generation of nested literals for both object and array literals.

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

[Kasper Lund, 2009-03-09 14:31:41]

LGTM.

http://codereview.chromium.org/40295/diff/1/3
File src/ast.h (right):

http://codereview.chromium.org/40295/diff/1/3#newcode639
Line 639: explicit MaterializedLiteral(int literal_index, bool is_simple)
Doesn't need to be explicit anymore, but I guess it doesn't hurt.

http://codereview.chromium.org/40295/diff/1/3#newcode667
Line 667: CONSTANT,             // Property with constant value (compile time).
Align comments?

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

http://codereview.chromium.org/40295/diff/1/10#newcode3454
Line 3454: case ObjectLiteral::Property::COMPUTED: {
Maybe mention in a comment that there's a 'fall through' here.

http://codereview.chromium.org/40295/diff/1/10#newcode3512
Line 3512: // by calling Runtime_CreateObjectLiteral.
Runtime_CreateArrayLiteralBoilerplate?

http://codereview.chromium.org/40295/diff/1/10#newcode3586
Line 3586: frame_->CallRuntime(Runtime::kCloneObjectLiteralBoilerplate, 1);
Maybe the name should be generalized a bit here to indicate that is is also used
to clone array literals?

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

http://codereview.chromium.org/40295/diff/1/9#newcode3101
Line 3101: if (object_literal != NULL  && object_literal->is_simple()) {
Two spaces after NULL.

http://codereview.chromium.org/40295/diff/1/9#newcode3106
Line 3106: if (array_literal != NULL  && array_literal->is_simple()) {
Can an expression convert to an array literal and an object literal? The code
seems to indicate that, and if it's not the case, I would probably make that
clear by wrapping the array literal case in an else clause or something. Remove
extra space after NULL here too.

http://codereview.chromium.org/40295/diff/1/9#newcode3110
Line 3110: return result;
I guess you're sure that it's either an object literal or an array literal,
right? Maybe that could be more clear from the code.

http://codereview.chromium.org/40295/diff/1/7
File src/parser.h (right):

http://codereview.chromium.org/40295/diff/1/7#newcode171
Line 171: class CompileTimeValue {
Inherit from AllStatic?

http://codereview.chromium.org/40295/diff/1/4
File src/runtime.cc (right):

http://codereview.chromium.org/40295/diff/1/4#newcode276
Line 276: Handle<FixedArray> literals = args.at<FixedArray>(0);
Is there a reason why you can't use the CONVERT_ARG_CHECKED macros here (and
maybe use CONVERT_CHECKED for the index)? I guess that it's somewhat closer to
the CreateObjectLiteralBoilerplate this way...

[olehougaard, 2009-03-10 08:35:11]

It dawned on me that I needed to make the change for ARM as well, so please take
another look. Specific comments addressed below.

http://codereview.chromium.org/40295/diff/1/3
File src/ast.h (right):

http://codereview.chromium.org/40295/diff/1/3#newcode667
Line 667: CONSTANT,             // Property with constant value (compile time).
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Align comments?

Fixed

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

http://codereview.chromium.org/40295/diff/1/10#newcode3454
Line 3454: case ObjectLiteral::Property::COMPUTED: {
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Maybe mention in a comment that there's a 'fall through' here.

Done.

http://codereview.chromium.org/40295/diff/1/10#newcode3512
Line 3512: // by calling Runtime_CreateObjectLiteral.
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Runtime_CreateArrayLiteralBoilerplate?

Fixed.

http://codereview.chromium.org/40295/diff/1/10#newcode3586
Line 3586: frame_->CallRuntime(Runtime::kCloneObjectLiteralBoilerplate, 1);
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Maybe the name should be generalized a bit here to indicate that is is also
used
> to clone array literals?

Fixed

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

http://codereview.chromium.org/40295/diff/1/9#newcode3101
Line 3101: if (object_literal != NULL  && object_literal->is_simple()) {
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Two spaces after NULL.

Fixed

http://codereview.chromium.org/40295/diff/1/9#newcode3106
Line 3106: if (array_literal != NULL  && array_literal->is_simple()) {
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Can an expression convert to an array literal and an object literal? The code
> seems to indicate that, and if it's not the case, I would probably make that
> clear by wrapping the array literal case in an else clause or something.
Remove
> extra space after NULL here too.

Fixed

http://codereview.chromium.org/40295/diff/1/9#newcode3110
Line 3110: return result;
On 2009/03/09 14:31:41, Kasper Lund wrote:
> I guess you're sure that it's either an object literal or an array literal,
> right? Maybe that could be more clear from the code.

I changed the code and put in some assertions to make it more clear.

http://codereview.chromium.org/40295/diff/1/7
File src/parser.h (right):

http://codereview.chromium.org/40295/diff/1/7#newcode171
Line 171: class CompileTimeValue {
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Inherit from AllStatic?

Done

http://codereview.chromium.org/40295/diff/1/4
File src/runtime.cc (right):

http://codereview.chromium.org/40295/diff/1/4#newcode276
Line 276: Handle<FixedArray> literals = args.at<FixedArray>(0);
On 2009/03/09 14:31:41, Kasper Lund wrote:
> Is there a reason why you can't use the CONVERT_ARG_CHECKED macros here (and
> maybe use CONVERT_CHECKED for the index)? I guess that it's somewhat closer to
> the CreateObjectLiteralBoilerplate this way...

Fixed in both functions.

[Kasper Lund, 2009-03-10 08:45:42]

LGTM.