Share optimized code for closures. (issue 10103035) - v8-dev

The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Message from discussion Share optimized code for closures. (issue 10103035)
Received-SPF: pass (google.com: domain of 3J8bZTxUJAJ4AFPMKGSKAMBCPCTGCU-FPEK8GJ....@m3kw2wvrgufz5godrsrytgd7.apphosting.bounces.google.com designates 209.85.160.199 as permitted sender) client-ip=209.85.160.199;
MIME-Version: 1.0
Reply-To: fschnei...@chromium.org, mstarzin...@chromium.org, 
	v8-dev@googlegroups.com
Message-ID: <bcaec50161b1ded2e704c26cb...@google.com>
Date: Thu, 14 Jun 2012 11:08:23 +0000
Subject: Re: Share optimized code for closures. (issue 10103035)
From: fschnei...@chromium.org
To: mstarzin...@chromium.org
Cc: v8-dev@googlegroups.com
Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes


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/
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
Account Options
