Share optimized code for closures.

Share optimized code for closures.

Each SharedFunctionInfo gets an optimized code map to store
one optimized code object per context. When allocating a new
closure we consult this map and check if there is optimized code
that can be shared.

This patch is based on an original patch
by Anton Muhin (http://codereview.chromium.org/6793013/).

BUG=v8:2087, v8:2094
TEST=test/mjsunit/compiler/optimized-closures.js
Committed: https://code.google.com/p/v8/source/detail?r=11817

[fschneider, 2012-04-20 14:05:43]

ia32 only. Still missing the sharing for the literals array.

http://codereview.chromium.org/10103035/diff/10006/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/10103035/diff/10006/src/factory.cc#newcode563
src/factory.cc:563: result->set_literals(*literals);
Instead of always allocating a new literals array, check if we can use a shared
literals array from the optimized code map stored, similar to the way we lookup
shared optimized code.

The same strategy has to be implemented for the FastNewClosureStub.

http://codereview.chromium.org/10103035/diff/10006/src/factory.cc#newcode574
src/factory.cc:574:
result->set_next_function_link(isolate()->heap()->undefined_value());
Not needed since next_function_link is already initialized in
Heap::AllocateFunction.

http://codereview.chromium.org/10103035/diff/10006/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/10103035/diff/10006/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:4428: __
push(Immediate(instr->hydrogen()->constant_elements()));
This should not be necessary if we share the literals array in the same way we
share optimized code among closures. So this change should not be necessary
anymore then.

[fschneider, 2012-05-09 08:55:00]

New ia32 patch uploaded. I'll port to the other platforms when this looks good
according to the feedback.

[fschneider, 2012-05-16 16:56:42]

Ping?

[Michael Starzinger, 2012-05-23 11:16:28]

First round. Sorry for the long delay, the CL fell through the cracks.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/compiler.cc
File src/compiler.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/compiler.cc#ne...
src/compiler.cc:603: static void
AddToOptimizedCodeMap(Handle<SharedFunctionInfo> shared,
Can we move this into the the SharedFunctionInfo class so that it is coupled
with the corresponding search method?

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/compiler.cc#ne...
src/compiler.cc:609: const int kEntryLength = 3;
Moving these constants into SharedFunctionInfo and reusing them in the search
method seems reasonable as well.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/compiler.cc#ne...
src/compiler.cc:626: for (int i = 0; i < old_length; i += kEntryLength) {
Can we use FixedArray::CopyTo or even a handlified version of
FixedArray::CopySize or a similar method to do the copying?

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/factory.cc
File src/factory.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/factory.cc#new...
src/factory.cc:561: FixedArray::cast(code_map->get(index + 1));
It would be nice to have constants for these offsets (i.e. -1, 0 and +1) in
SharedFunctionInfo.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/factory.cc#new...
src/factory.cc:562: ASSERT(cached_literals != NULL);
Better add an ASSERT that the global context stored in the cached literals array
still matches.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/frames.cc
File src/frames.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/frames.cc#newc...
src/frames.cc:837: FixedArray* literal_array = data->LiteralArray();
I know this is not part of your change, but for consistency reasons this should
actually be called "literals_array" (plural). We could change that in
DeoptimizationInputData. If it's too much overhead for this CL, just ignore this
comment.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/heap.cc#newcod...
src/heap.cc:3005: share->set_optimized_code_map(Smi::FromInt(0));
Would it be possible to set this to the empty fixed array instead and remove
some of the special casing in the search and add-to function.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/ia32/code-stub...
File src/ia32/code-stubs-ia32.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/ia32/code-stub...
src/ia32/code-stubs-ia32.cc:130: const int kEntryLength = 3;
Again, having this constant in SharedFunctionInfo seems safer.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/ia32/code-stub...
src/ia32/code-stubs-ia32.cc:7146: { ecx, edx, ebx, EMIT_REMEMBERED_SET},
Use the REG macro as well.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/ia32/deoptimiz...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:122:
function->shared()->set_optimized_code_map(Smi::FromInt(0));
I think it would make sense to have shared->ClearOptimizedCodeMap() as a helper.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/mark-compact.c...
src/mark-compact.cc:1305:
reinterpret_cast<SharedFunctionInfo*>(object)->BeforeVisitingPointers();
We should be able to use SharedFunctionInfo::cast() with the new GC.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/objects.h#newc...
src/objects.h:5164: // i - 1 is the context, position i the code, and i + 1 the
literals array.
Can we add a comment about what happens when no entry is found?

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/objects.h#newc...
src/objects.h:5274: // Invoked before pointers in SharedFunctionInfo are being
marked.
It would be nice to also have a short comment what this method does on top of
when it is being called.

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/23027/src/runtime.cc#new...
src/runtime.cc:8255:
function->shared()->set_optimized_code_map(Smi::FromInt(0));
It seems we can hoist that out of the condition.

[fschneider, 2012-06-14 11:08:23]

http://codereview.chromium.org/10103035/diff/23027/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/compiler.cc#newcode603
src/compiler.cc:603: static void
AddToOptimizedCodeMap(Handle<SharedFunctionInfo> shared,
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Can we move this into the the SharedFunctionInfo class so that it is coupled
> with the corresponding search method?

Done.

http://codereview.chromium.org/10103035/diff/23027/src/compiler.cc#newcode609
src/compiler.cc:609: const int kEntryLength = 3;
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Moving these constants into SharedFunctionInfo and reusing them in the search
> method seems reasonable as well.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/compiler.cc#newcode626
src/compiler.cc:626: for (int i = 0; i < old_length; i += kEntryLength) {
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Can we use FixedArray::CopyTo or even a handlified version of
> FixedArray::CopySize or a similar method to do the copying?

Done.

http://codereview.chromium.org/10103035/diff/23027/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/factory.cc#newcode561
src/factory.cc:561: FixedArray::cast(code_map->get(index + 1));
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> It would be nice to have constants for these offsets (i.e. -1, 0 and +1) in
> SharedFunctionInfo.

Agree. Though I'd have the constants relative to the entry start so that it does
not get confusing: 0 = context, 1 = code, 2 = literals

SearchOptimizedCodeMap would have to accordingly return the entry start instead
of the Code*

http://codereview.chromium.org/10103035/diff/23027/src/factory.cc#newcode562
src/factory.cc:562: ASSERT(cached_literals != NULL);
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Better add an ASSERT that the global context stored in the cached literals
array
> still matches.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:130: const int kEntryLength = 3;
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Again, having this constant in SharedFunctionInfo seems safer.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:7146: { ecx, edx, ebx, EMIT_REMEMBERED_SET},
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Use the REG macro as well.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/ia32/deoptimizer-ia32....
src/ia32/deoptimizer-ia32.cc:122:
function->shared()->set_optimized_code_map(Smi::FromInt(0));
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> I think it would make sense to have shared->ClearOptimizedCodeMap() as a
helper.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/mark-compact.cc#newcod...
src/mark-compact.cc:1305:
reinterpret_cast<SharedFunctionInfo*>(object)->BeforeVisitingPointers();
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> We should be able to use SharedFunctionInfo::cast() with the new GC.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/10103035/diff/23027/src/objects.h#newcode5164
src/objects.h:5164: // i - 1 is the context, position i the code, and i + 1 the
literals array.
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> Can we add a comment about what happens when no entry is found?

Done.

http://codereview.chromium.org/10103035/diff/23027/src/objects.h#newcode5274
src/objects.h:5274: // Invoked before pointers in SharedFunctionInfo are being
marked.
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> It would be nice to also have a short comment what this method does on top of
> when it is being called.

Done.

http://codereview.chromium.org/10103035/diff/23027/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10103035/diff/23027/src/runtime.cc#newcode8255
src/runtime.cc:8255:
function->shared()->set_optimized_code_map(Smi::FromInt(0));
On 2012/05/23 11:16:29, Michael Starzinger wrote:
> It seems we can hoist that out of the condition.

Done.

[Michael Starzinger, 2012-06-14 13:51:31]

LGTM (if one comment and one nit is addressed).

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc
File src/factory.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc#new...
src/factory.cc:555: ?
function_info->SearchOptimizedCodeMap(context->global_context())
We need to do this search anyways, even if the function is bound, so let's hoist
the search out to do it only once.

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc#new...
src/factory.cc:583:
function_info->SearchOptimizedCodeMap(context->global_context());
Drop this search and the outer flag check.

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/objects.cc#new...
src/objects.cc:7583: set_optimized_code_map(Smi::FromInt(0));
It would simplify the runtime code if we would initialize this with the empty
fixed array. The FastNewClosureStub however would need to compare against a root
pointer instead of doing a Smi(0) check. I am not sure how much slower the
pointer comparison will be.

[Florian Schneider, 2012-06-14 14:02:41]

Thanks for the review. Landing.

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc
File src/factory.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc#new...
src/factory.cc:555: ?
function_info->SearchOptimizedCodeMap(context->global_context())
On 2012/06/14 13:51:32, Michael Starzinger wrote:
> We need to do this search anyways, even if the function is bound, so let's
hoist
> the search out to do it only once.

Good point. Done.

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/factory.cc#new...
src/factory.cc:583:
function_info->SearchOptimizedCodeMap(context->global_context());
On 2012/06/14 13:51:32, Michael Starzinger wrote:
> Drop this search and the outer flag check.

Done.

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10103035/diff/38005/src/objects.cc#new...
src/objects.cc:7583: set_optimized_code_map(Smi::FromInt(0));
On 2012/06/14 13:51:32, Michael Starzinger wrote:
> It would simplify the runtime code if we would initialize this with the empty
> fixed array. The FastNewClosureStub however would need to compare against a
root
> pointer instead of doing a Smi(0) check. I am not sure how much slower the
> pointer comparison will be.

I'll leave it as is for now. I assume it should not make a big difference in
terms of performance, but I'd like to confirm that before changing it.