- Remove function boilerplate objects and use SharedFunctionInfos in...

- Remove function boilerplate objects and use SharedFunctionInfos in
  their place.
- Add one field to the SharedFunctionInfo to remember the number of
  literals used in this function.


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

[iposva, 2010-03-06 10:52:57]

First cut at removing function boilerplate objects. If you get the time please
take a first pass as this is a rather large change. I'll pick it up on Monday in
California again.

Thanks,
-Ivan

Missing: non-ia32 architectures, simple closure creation
Test failure: test-log-stack-tracer

[Kasper Lund, 2010-03-08 08:21:52]

LGTM. My main comment is that I'm not too crazy about the name shared_function
which is used in a lot of places. The reason why I don't like it is that it
confuses me that something called shared_function isn't a function. I would
change it to 'shared' or 'shared_info'.

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

http://codereview.chromium.org/669240/diff/1/6#newcode1216
src/api.cc:1216: i::Handle<i::Object> obj = Utils::OpenHandle(this);
Maybe refactor this code so it can be shared with the same code in
Script::SetData?

http://codereview.chromium.org/669240/diff/1/17
File src/bootstrapper.cc (right):

http://codereview.chromium.org/669240/diff/1/17#newcode819
src/bootstrapper.cc:819: Handle<SharedFunctionInfo> shared_function;
In general, I would probably try to call local variables of type
SharedFunctionInfo shared_info / shared instead of shared_function because it's
weird to me that something called shared_function isn't a function.

http://codereview.chromium.org/669240/diff/1/25
File src/codegen.cc (right):

http://codereview.chromium.org/669240/diff/1/25#newcode340
src/codegen.cc:340: Compiler::BuildFunction(node->fun(), script(), this);
It's somewhat weird that BuildFunction doesn't return a function.
BuildFunctionInfo?

http://codereview.chromium.org/669240/diff/1/20
File src/compiler.cc (right):

http://codereview.chromium.org/669240/diff/1/20#newcode120
src/compiler.cc:120: static Handle<SharedFunctionInfo> MakeFunction(
It's weird that MakeFunction doesn't return a function. Maybe change to
MakeFunctionInfo?

http://codereview.chromium.org/669240/diff/1/20#newcode422
src/compiler.cc:422: Handle<Script> script,
Funky indentation.

http://codereview.chromium.org/669240/diff/1/27
File src/compiler.h (right):

http://codereview.chromium.org/669240/diff/1/27#newcode254
src/compiler.h:254: // Compile a function boilerplate object (the function is
possibly
Update comment. Maybe change name from BuildFunction to BuildFunctionInfo?

http://codereview.chromium.org/669240/diff/1/28
File src/debug.cc (right):

http://codereview.chromium.org/669240/diff/1/28#newcode1967
src/debug.cc:1967: Handle<SharedFunctionInfo> fun) {
fun -> shared

http://codereview.chromium.org/669240/diff/1/12
File src/debug.h (right):

http://codereview.chromium.org/669240/diff/1/12#newcode608
src/debug.h:608: Handle<SharedFunctionInfo> fun);
fun -> shared?

http://codereview.chromium.org/669240/diff/1/30
File src/fast-codegen.cc (right):

http://codereview.chromium.org/669240/diff/1/30#newcode566
src/fast-codegen.cc:566: SharedFunctionLiteral* expr) {
Indentation seems slightly off here.

http://codereview.chromium.org/669240/diff/1/7
File src/heap.cc (right):

http://codereview.chromium.org/669240/diff/1/7#newcode1264
src/heap.cc:1264: // Allocate the empty array
Terminate comment with .

http://codereview.chromium.org/669240/diff/1/7#newcode1269
src/heap.cc:1269: obj = AllocateEmptyFixedArray();
It would be nice with at least a comment on what this is for.

http://codereview.chromium.org/669240/diff/1/35
File src/heap.h (right):

http://codereview.chromium.org/669240/diff/1/35#newcode63
src/heap.h:63: V(FixedArray, empty_fixed_array2, EmptyFixedArray2)              
            \
Line too long ... and what is this?

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

http://codereview.chromium.org/669240/diff/1/14#newcode4060
src/ia32/codegen-ia32.cc:4060: if (0 && scope()->is_function_scope() &&
0 -> false

http://codereview.chromium.org/669240/diff/1/16
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/669240/diff/1/16#newcode541
src/ia32/codegen-ia32.h:541: // Instantiate the function boilerplate.
Update comment.

http://codereview.chromium.org/669240/diff/1/13
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/669240/diff/1/13#newcode199
src/ia32/fast-codegen-ia32.cc:199: SharedFunctionLiteral* expr) {
Indentation.

http://codereview.chromium.org/669240/diff/1/13#newcode771
src/ia32/fast-codegen-ia32.cc:771: SharedFunctionLiteral* expr) {
Indentation.

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

http://codereview.chromium.org/669240/diff/1/5#newcode922
src/prettyprinter.cc:922: PrintLiteralIndented("SHARED FUNCTION",
node->shared_function(), true);
SHARED FUNCTION -> SHARED INFO?

http://codereview.chromium.org/669240/diff/1/3
File test/cctest/test-func-name-inference.cc (right):

http://codereview.chromium.org/669240/diff/1/3#newcode65
test/cctest/test-func-name-inference.cc:65: Handle<Object> obj =
v8::Utils::OpenHandle(*script);
This also looks a bit like the code in Script::SetData.

[iposva, 2010-03-22 23:56:17]

Addressed review comments. Now working on the other architectures, but please
take another look in the meantime.

Thanks,
-Ivan

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

http://codereview.chromium.org/669240/diff/1/6#newcode1216
src/api.cc:1216: i::Handle<i::Object> obj = Utils::OpenHandle(this);
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Maybe refactor this code so it can be shared with the same code in
> Script::SetData?

Done.

http://codereview.chromium.org/669240/diff/1/17
File src/bootstrapper.cc (right):

http://codereview.chromium.org/669240/diff/1/17#newcode819
src/bootstrapper.cc:819: Handle<SharedFunctionInfo> shared_function;
On 2010/03/08 08:21:52, Kasper Lund wrote:
> In general, I would probably try to call local variables of type
> SharedFunctionInfo shared_info / shared instead of shared_function because
it's
> weird to me that something called shared_function isn't a function.

Decided to call it function_info everywhere, shared_info is not conveying the
fact that it is the information associated with a function.

http://codereview.chromium.org/669240/diff/1/25
File src/codegen.cc (right):

http://codereview.chromium.org/669240/diff/1/25#newcode340
src/codegen.cc:340: Compiler::BuildFunction(node->fun(), script(), this);
On 2010/03/08 08:21:52, Kasper Lund wrote:
> It's somewhat weird that BuildFunction doesn't return a function.
> BuildFunctionInfo?

Done.

http://codereview.chromium.org/669240/diff/1/20
File src/compiler.cc (right):

http://codereview.chromium.org/669240/diff/1/20#newcode120
src/compiler.cc:120: static Handle<SharedFunctionInfo> MakeFunction(
On 2010/03/08 08:21:52, Kasper Lund wrote:
> It's weird that MakeFunction doesn't return a function. Maybe change to
> MakeFunctionInfo?

Done.

http://codereview.chromium.org/669240/diff/1/20#newcode422
src/compiler.cc:422: Handle<Script> script,
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Funky indentation.

Done.

http://codereview.chromium.org/669240/diff/1/27
File src/compiler.h (right):

http://codereview.chromium.org/669240/diff/1/27#newcode254
src/compiler.h:254: // Compile a function boilerplate object (the function is
possibly
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Update comment. Maybe change name from BuildFunction to BuildFunctionInfo?

Done.

http://codereview.chromium.org/669240/diff/1/28
File src/debug.cc (right):

http://codereview.chromium.org/669240/diff/1/28#newcode1967
src/debug.cc:1967: Handle<SharedFunctionInfo> fun) {
On 2010/03/08 08:21:52, Kasper Lund wrote:
> fun -> shared

ditto

http://codereview.chromium.org/669240/diff/1/12
File src/debug.h (right):

http://codereview.chromium.org/669240/diff/1/12#newcode608
src/debug.h:608: Handle<SharedFunctionInfo> fun);
On 2010/03/08 08:21:52, Kasper Lund wrote:
> fun -> shared?

This fun ctionality has been changed in the meantime.

http://codereview.chromium.org/669240/diff/1/30
File src/fast-codegen.cc (right):

http://codereview.chromium.org/669240/diff/1/30#newcode566
src/fast-codegen.cc:566: SharedFunctionLiteral* expr) {
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Indentation seems slightly off here.

Done.

http://codereview.chromium.org/669240/diff/1/7
File src/heap.cc (right):

http://codereview.chromium.org/669240/diff/1/7#newcode1264
src/heap.cc:1264: // Allocate the empty array
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/669240/diff/1/7#newcode1269
src/heap.cc:1269: obj = AllocateEmptyFixedArray();
On 2010/03/08 08:21:52, Kasper Lund wrote:
> It would be nice with at least a comment on what this is for.

ditto

http://codereview.chromium.org/669240/diff/1/35
File src/heap.h (right):

http://codereview.chromium.org/669240/diff/1/35#newcode63
src/heap.h:63: V(FixedArray, empty_fixed_array2, EmptyFixedArray2)              
            \
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Line too long ... and what is this?

Surviving debugging code. Gone!

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

http://codereview.chromium.org/669240/diff/1/14#newcode4060
src/ia32/codegen-ia32.cc:4060: if (0 && scope()->is_function_scope() &&
On 2010/03/08 08:21:52, Kasper Lund wrote:
> 0 -> false

Done. We should implement the FastNewClosureStub, but I am focusing on getting
this massive change in first...

http://codereview.chromium.org/669240/diff/1/16
File src/ia32/codegen-ia32.h (right):

http://codereview.chromium.org/669240/diff/1/16#newcode541
src/ia32/codegen-ia32.h:541: // Instantiate the function boilerplate.
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Update comment.

Done.

http://codereview.chromium.org/669240/diff/1/13
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/669240/diff/1/13#newcode199
src/ia32/fast-codegen-ia32.cc:199: SharedFunctionLiteral* expr) {
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Indentation.

Done.

http://codereview.chromium.org/669240/diff/1/13#newcode771
src/ia32/fast-codegen-ia32.cc:771: SharedFunctionLiteral* expr) {
On 2010/03/08 08:21:52, Kasper Lund wrote:
> Indentation.

Done.

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

http://codereview.chromium.org/669240/diff/1/5#newcode922
src/prettyprinter.cc:922: PrintLiteralIndented("SHARED FUNCTION",
node->shared_function(), true);
On 2010/03/08 08:21:52, Kasper Lund wrote:
> SHARED FUNCTION -> SHARED INFO?

Done.

http://codereview.chromium.org/669240/diff/1/3
File test/cctest/test-func-name-inference.cc (right):

http://codereview.chromium.org/669240/diff/1/3#newcode65
test/cctest/test-func-name-inference.cc:65: Handle<Object> obj =
v8::Utils::OpenHandle(*script);
On 2010/03/08 08:21:52, Kasper Lund wrote:
> This also looks a bit like the code in Script::SetData.

Yes, except that I cannot access the static helper function inside api.cc from
here.

[iposva, 2010-03-23 00:44:07]

Added ARM and X64 changes.

-Ivan

[Kasper Lund, 2010-03-23 05:32:25]

LGTM if you fix any linting issues. I'll take care of reoptimizing the fast
closure allocation code again ASAP.

[iposva, 2010-03-23 06:01:48]

Adressed linting problems.

-Ivan