Optimizing nested, constant object literals (like JSON objects) by building o...

Optimizing nested, constant object literals (like JSON objects) by building one large object template for the entire object instead of one for each sub-object.

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

[Mads Ager (chromium), 2009-03-05 13:19:13]

Doesn't this change lead to duplicate object literal boilerplates for nested
object literals?

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

http://codereview.chromium.org/39184/diff/1/3#newcode698
Line 698: bool is_constant_;
Should we call this something like 'simple' or 'trivial' instead and explain
what we mean by that (only contains constants and other 'simple' object
literals)?

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

http://codereview.chromium.org/39184/diff/1/6#newcode3086
Line 3086: Handle<Object> Parser::GetBoilerplateValue(ObjectLiteral::Property*
property) {
Files seems to be missing from the code review.  This changes the signature, but
parser.h is not included.

You need a good comment in parser.h to explain the return values of this method.

http://codereview.chromium.org/39184/diff/1/6#newcode3212
Line 3212: literal_index,
Do you still allocate all the nested literals in the literals array?  Doesn't
that mean that you get two identical boilerplates for the same nested literal? 
One in the object literal in which it is nested and one in the list of literals
in the function?  If that is the case, we should   make sure to avoid that
duplication.

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

http://codereview.chromium.org/39184/diff/1/4#newcode135
Line 135: Handle<FixedArray> literals,
Indentation is off for these three lines.

http://codereview.chromium.org/39184/diff/1/4#newcode161
Line 161: if (value->IsFixedArray()) {
The use of fixed arrays to signal that this was a nested object literal needs a
comment.

http://codereview.chromium.org/39184/diff/1/4#newcode163
Line 163: value = CreateObjectLiteralBoilerplate(literals, -1, array);
It might make the code more readable to introduce a named constant for -1?  Or
it might be even better to have two functions.  One for constructing an object
literal boilerplate and one for constructing a nested object literal
boilerplate.  They can share all the code in a helper function but one of them
adds the final boilerplate to the boilerplate array.

[olehougaard, 2009-03-06 09:05:10]

Please take another look at this.

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

http://codereview.chromium.org/39184/diff/1/3#newcode698
Line 698: bool is_constant_;
On 2009/03/05 13:19:14, Mads Ager wrote:
> Should we call this something like 'simple' or 'trivial' instead and explain
> what we mean by that (only contains constants and other 'simple' object
> literals)?

I went with 'simple'.

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

http://codereview.chromium.org/39184/diff/1/6#newcode3086
Line 3086: Handle<Object> Parser::GetBoilerplateValue(ObjectLiteral::Property*
property) {
On 2009/03/05 13:19:14, Mads Ager wrote:
> Files seems to be missing from the code review.  This changes the signature,
but
> parser.h is not included.
> 
> You need a good comment in parser.h to explain the return values of this
method.

The Parser class is defined in the parser.cc file, not parser.h. I've amended
the comment for GetBoilerplateValue above.

http://codereview.chromium.org/39184/diff/1/6#newcode3212
Line 3212: literal_index,
On 2009/03/05 13:19:14, Mads Ager wrote:
> Do you still allocate all the nested literals in the literals array?  Doesn't
> that mean that you get two identical boilerplates for the same nested literal?

> One in the object literal in which it is nested and one in the list of
literals
> in the function?  If that is the case, we should   make sure to avoid that
> duplication.

I don't create code for the nested literal, so I don't get two identical
boilerplates. I still reserve a space for them in the literals array, though. I
don't think saving that space is worth the hassle of figuring whether we'll need
it.

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

http://codereview.chromium.org/39184/diff/1/4#newcode135
Line 135: Handle<FixedArray> literals,
On 2009/03/05 13:19:14, Mads Ager wrote:
> Indentation is off for these three lines.

Fixed. Also fixed for the function above this.

http://codereview.chromium.org/39184/diff/1/4#newcode161
Line 161: if (value->IsFixedArray()) {
On 2009/03/05 13:19:14, Mads Ager wrote:
> The use of fixed arrays to signal that this was a nested object literal needs
a
> comment.

Comment added.

http://codereview.chromium.org/39184/diff/1/4#newcode163
Line 163: value = CreateObjectLiteralBoilerplate(literals, -1, array);
On 2009/03/05 13:19:14, Mads Ager wrote:
> It might make the code more readable to introduce a named constant for -1?  Or
> it might be even better to have two functions.  One for constructing an object
> literal boilerplate and one for constructing a nested object literal
> boilerplate.  They can share all the code in a helper function but one of them
> adds the final boilerplate to the boilerplate array.

Removed the literal_index from the recursion.

[Mads Ager (chromium), 2009-03-06 09:30:54]

LGTM

http://codereview.chromium.org/39184/diff/1001/23
File src/parser.cc (right):

http://codereview.chromium.org/39184/diff/1001/23#newcode165
Line 165: // Otherwise, it return undefined literal as the placeholder
it return -> return.