Re: Share optimized code for closures. (issue 10103035)

[fschn...@chromium.org]

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.cc#newcode130
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.cc#newcode7146
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.cc#newcode122
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#newcode1305
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.

http://codereview.chromium.org/10103035/

[mstar...@chromium.org]

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#newcode555
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#newcode583
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#newcode7583
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.

https://chromiumcodereview.appspot.com/10103035/

[fschn...@google.com]

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#newcode555
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#newcode583
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#newcode7583
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.

https://chromiumcodereview.appspot.com/10103035/