Enable preaging of code objects when --optimize-for-size.

Enable preaging of code objects when --optimize-for-size.

This change means that code which is never executed is garbage collected immediately, and code which is only executed once is collected more quickly (limiting heap growth), however, code which is re-executed is  reset to the young age, thus being kept around for the same number of GC generations as currently.

BUG=280984
R=danno@chromium.org, hpayer@chromium.org

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

[rmcilroy, 2013-09-09 09:48:13]

I'll add support for other architectures if you think this looks OK.

[Hannes Payer (out of office), 2013-09-09 15:17:53]

I am wondering about the performance impact of that change. What happens if you
preage also on non-low-memory devices?

[rmcilroy, 2013-09-11 15:28:28]

Updated for all architectures.  I'll report back with the performance impact if
enabled for all devices.

https://codereview.chromium.org/23480031/diff/22001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/23480031/diff/22001/src/ia32/codegen-ia32.cc#...
src/ia32/codegen-ia32.cc:1146: patcher.masm()->nop();
If you have any ideas for how to distingish between pre-aged and young code
sequences without adding this nop on x86/64, that would be awesome.

[Hannes Payer (out of office), 2013-09-12 18:16:24]

All right, the patch looks fine. But I don't think we should do it for
non-memory-constrained devices, at least not now. Can we turn it on/off based on
such a memoy-constrained device check? Moreover it would be nice to turn it
on/off using on a command line flag.

[Sven Panne, 2013-09-13 06:24:25]

On 2013/09/12 18:16:24, Hannes Payer wrote:
> All right, the patch looks fine. But I don't think we should do it for
> non-memory-constrained devices, at least not now. Can we turn it on/off based
on
> such a memoy-constrained device check? Moreover it would be nice to turn it
> on/off using on a command line flag.

This is what Isolate::is_memory_constrained() is for...

[rmcilroy_google, 2013-09-13 09:30:50]

On 2013/09/13 06:24:25, Sven Panne wrote:
> On 2013/09/12 18:16:24, Hannes Payer wrote:
> > All right, the patch looks fine. But I don't think we should do it for
> > non-memory-constrained devices, at least not now. Can we turn it on/off
based
> on
> > such a memoy-constrained device check? Moreover it would be nice to turn it
> > on/off using on a command line flag.
> 
> This is what Isolate::is_memory_constrained() is for...

Yes, I can use Isolate::is_memory_constrained() to disable this for
non-memory-constrained devices.  Do you also want me to put it behind a flag? 
If so, is this flag something you want to be able to turn it on for
non-memory-constrained devices, or to turn it off for all devices?

[Hannes Payer (out of office), 2013-09-13 09:56:40]

On 2013/09/13 09:30:50, rmcilroy_google wrote:
> On 2013/09/13 06:24:25, Sven Panne wrote:
> > On 2013/09/12 18:16:24, Hannes Payer wrote:
> > > All right, the patch looks fine. But I don't think we should do it for
> > > non-memory-constrained devices, at least not now. Can we turn it on/off
> based
> > on
> > > such a memoy-constrained device check? Moreover it would be nice to turn
it
> > > on/off using on a command line flag.
> > 
> > This is what Isolate::is_memory_constrained() is for...
> 
> Yes, I can use Isolate::is_memory_constrained() to disable this for
> non-memory-constrained devices.  Do you also want me to put it behind a flag? 
> If so, is this flag something you want to be able to turn it on for
> non-memory-constrained devices, or to turn it off for all devices?

It would be funky to overrule Isolate::is_memory_constrained() to experiment
with this feature if needed.

[rmcilroy, 2013-09-13 10:17:22]

On 2013/09/13 09:56:40, Hannes Payer wrote:
> On 2013/09/13 09:30:50, rmcilroy_google wrote:
> > On 2013/09/13 06:24:25, Sven Panne wrote:
> > > On 2013/09/12 18:16:24, Hannes Payer wrote:
> > > > All right, the patch looks fine. But I don't think we should do it for
> > > > non-memory-constrained devices, at least not now. Can we turn it on/off
> > based
> > > on
> > > > such a memoy-constrained device check? Moreover it would be nice to turn
> it
> > > > on/off using on a command line flag.
> > > 
> > > This is what Isolate::is_memory_constrained() is for...
> > 
> > Yes, I can use Isolate::is_memory_constrained() to disable this for
> > non-memory-constrained devices.  Do you also want me to put it behind a
flag? 
> > If so, is this flag something you want to be able to turn it on for
> > non-memory-constrained devices, or to turn it off for all devices?
> 
> It would be funky to overrule Isolate::is_memory_constrained() to experiment
> with this feature if needed.

I'm assuming "funky" here means it would be good ;).  I've updated this CL to
only pre-age for memory_constrained devices.  I'll send another CL for adding
the command line flag to set is_memory_constrained.

[rmcilroy, 2013-09-16 08:21:57]

On 2013/09/13 10:17:22, rmcilroy wrote:
> On 2013/09/13 09:56:40, Hannes Payer wrote:
> > On 2013/09/13 09:30:50, rmcilroy_google wrote:
> > > On 2013/09/13 06:24:25, Sven Panne wrote:
> > > > On 2013/09/12 18:16:24, Hannes Payer wrote:
> > > > > All right, the patch looks fine. But I don't think we should do it for
> > > > > non-memory-constrained devices, at least not now. Can we turn it
on/off
> > > based
> > > > on
> > > > > such a memoy-constrained device check? Moreover it would be nice to
turn
> > it
> > > > > on/off using on a command line flag.
> > > > 
> > > > This is what Isolate::is_memory_constrained() is for...
> > > 
> > > Yes, I can use Isolate::is_memory_constrained() to disable this for
> > > non-memory-constrained devices.  Do you also want me to put it behind a
> flag? 
> > > If so, is this flag something you want to be able to turn it on for
> > > non-memory-constrained devices, or to turn it off for all devices?
> > 
> > It would be funky to overrule Isolate::is_memory_constrained() to experiment
> > with this feature if needed.
> 
> I'm assuming "funky" here means it would be good ;).  I've updated this CL to
> only pre-age for memory_constrained devices.  I'll send another CL for adding
> the command line flag to set is_memory_constrained.

Added the command line flag for memory_constrained in the other CL. Any more
changes required in this CL or is it good to go?

[Sven Panne, 2013-09-16 09:42:52]

On 2013/09/16 08:21:57, rmcilroy wrote:
> Added the command line flag for memory_constrained in the other CL. Any more
> changes required in this CL or is it good to go?

I'm leaving this decision to Hannes, from my side it's OK...

[Hannes Payer (out of office), 2013-09-16 14:31:45]

https://codereview.chromium.org/23480031/diff/29001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/23480031/diff/29001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:1065: // survive further).
Since we have the mode now which is turned on on mobile but turned off on
ia32/x64, does this test still work as intended on both platforms (since before
we called the GC two times)?

Would it make sense to leave the test untouched and set the flag to not pre-age
explicitly and add additional tests for the pre-aging?

Same for the code below...

[rmcilroy, 2013-09-16 15:45:46]

https://codereview.chromium.org/23480031/diff/29001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/23480031/diff/29001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:1065: // survive further).
On 2013/09/16 14:31:45, Hannes Payer wrote:
> Since we have the mode now which is turned on on mobile but turned off on
> ia32/x64, does this test still work as intended on both platforms (since
before
> we called the GC two times)?
> 
> Would it make sense to leave the test untouched and set the flag to not
pre-age
> explicitly and add additional tests for the pre-aging?
> 
> Same for the code below...

Sounds like a plan. Done.

[Hannes Payer (out of office), 2013-09-16 16:13:03]

LGTM

[Michael Starzinger, 2013-09-17 22:56:38]

Drive-by comment.

https://codereview.chromium.org/23480031/diff/65001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/23480031/diff/65001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:166: __ nop();
IIUC this is a glorified way to store exactly one bit of information, namely
whether a code object is freshly generated and has never been aged before.
Wouldn't it be easier to store that bit of information in the code object
header? This could spare us from adding a NOP to every single method prologue.
Also it would keep platform specific changes to a minimum.

[danno, 2013-09-18 08:32:00]

NOT LGTM. I really don't like the conditional changes in the prologue. I am
pretty sure that you use a bit in the code object for the information you need
and rely off the execution of the age stub to increment it, rather than adding a
noop to all code, even non-constrained code. Here's how I'd make this change:

on execution --> go back to young, but increment a counter on the code object.
Only needs to be 2 bits, but not in instruction stream.
When you age code, go directly to "old" if the execution count is only 1. If
it's pinned at the max value, go through the old aging algorithm.

Also, please split this into 2 CLs, one for the resource management framework
and one for the actual code aging.

BTW, for resource constrained systems, why not just reduce the number of GCs to
one to reclaim code in general?

[rmcilroy, 2013-09-18 10:59:32]

> IIUC this is a glorified way to store exactly one bit of information, namely
> whether a code object is freshly generated and has never been aged before.
> Wouldn't it be easier to store that bit of information in the code object
> header? This could spare us from adding a NOP to every single method prologue.
> Also it would keep platform specific changes to a minimum.

This is a good point.  I think it made sense storing this bit of information in
the pre-existing nop on ARM, but was never happy with the added nop's in
ia32/x64 code.  I've reworked this to use a bit in the code object, but only for
FUNCTION types (I'm assuming if get around to optimizing a function then there
is less chance it will only be run once and so benefit for preaging).
 
> on execution --> go back to young, but increment a counter on the code object.
> Only needs to be 2 bits, but not in instruction stream.
> When you age code, go directly to "old" if the execution count is only 1. If
> it's pinned at the max value, go through the old aging algorithm.

I'm not sure how the counter would help over the single bit of information of
is_pre_aged.  I've implemented this as a bit on the code object, please let me
know if this is OK.

> Also, please split this into 2 CLs, one for the resource management framework
> and one for the actual code aging.

As discussed offline, the resource framework code is already split into a
seperate CL (which is already submitted).

> BTW, for resource constrained systems, why not just reduce the number of GCs
to
> one to reclaim code in general?

I tried this, but it gave a considerable reduction in performance for not much
more of a peak code-space heap size reduction. 

PTAL.

[Michael Starzinger, 2013-09-18 11:51:16]

On 2013/09/18 10:59:32, rmcilroy wrote:
> > IIUC this is a glorified way to store exactly one bit of information, namely
> > whether a code object is freshly generated and has never been aged before.
> > Wouldn't it be easier to store that bit of information in the code object
> > header? This could spare us from adding a NOP to every single method
prologue.
> > Also it would keep platform specific changes to a minimum.
> 
> This is a good point.  I think it made sense storing this bit of information
in
> the pre-existing nop on ARM, but was never happy with the added nop's in
> ia32/x64 code.  I've reworked this to use a bit in the code object, but only
for
> FUNCTION types (I'm assuming if get around to optimizing a function then there
> is less chance it will only be run once and so benefit for preaging).  

Currently we still don't flush optimized code, so adding the bit to
OPTIMIZED_FUNCTION as well wouldn't make any difference right now. That being
said, flushing of optimized code has been on our agenda for ages, and we
shouldn't build anything into the systems that makes flushing of optimized code
less powerful compared to unoptimized code.

[danno, 2013-09-18 12:15:08]

The reason I suggested a counter is an intuition about aging of code. If a
function has been executed zero times, then it should be thrown away immediately
during GC. If it has been executed only once, it probably was global
initialization code and probably should be thrown away quickly, too. If a
function has been executed 2 or more times, it probably is something that is
worth keeping around since it might be executed again.

Is there any reason to not have this optimization always on, not just for
low-memory devices? If there is no regression, it seems like I would prefer
having a unified policy, it reduces the testing matrix and makes tracking down
problems easier.

[rmcilroy, 2013-09-18 13:05:33]

> Currently we still don't flush optimized code, so adding the bit to
> OPTIMIZED_FUNCTION as well wouldn't make any difference right now. That being
> said, flushing of optimized code has been on our agenda for ages, and we
> shouldn't build anything into the systems that makes flushing of optimized
code
> less powerful compared to unoptimized code.

Ok, I added support for OPTIMIZED_FUNCTION too.

> The reason I suggested a counter is an intuition about aging of code. If a
> function has been executed zero times, then it should be thrown away
immediately
> during GC. If it has been executed only once, it probably was global
> initialization code and probably should be thrown away quickly, too. If a
> function has been executed 2 or more times, it probably is something that is
> worth keeping around since it might be executed again.

I don't know how you would maintain this counter though. You would need to set
the initial prologue to jump to a stub which increments this counter then
patches the prologue to be young again (that gets you from 0->1 invocation), but
then how would you know that the execution count had gone above 1 unless the
prologue has code to always increment this counter even when the code is young? 
This would seem like a pretty big performance hit, even if we disabled this
counter after the first GC subsequent to the code being generated. 

> Is there any reason to not have this optimization always on, not just for
> low-memory devices? If there is no regression, it seems like I would prefer
> having a unified policy, it reduces the testing matrix and makes tracking down
> problems easier.

It was on unconditionally in a previous patch-set, but Hannes wanted to be a bit
more conservative and only do it on low memory devices.  There didn't seem to be
a performance regression when we tested it though, so I'm happy to turn it on
unconditionally if you are OK with that.

[rmcilroy, 2013-09-23 12:58:34]

I've redo this to use code stubs which track whether code has been executed
zero, once or multiple times. I didn't bother with a counter on the Code object
as this didn't seem necessary given the stubs track this implicitly.

PTAL, let me know what you think.  If generally agreeable, I'll implement for
other architectures.

https://codereview.chromium.org/23480031/diff/87001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/23480031/diff/87001/test/cctest/test-heap.cc#...
test/cctest/test-heap.cc:1098: }
I would like to test the immediate GCing of code that's never been executed, but
I can't figure out a way to compile, but not execute the code (removing the call
to "foo()" in source above seems to mean that foo doesn't get compiled at all). 
Any ideas?

[rmcilroy, 2013-09-26 11:22:14]

On 2013/09/23 12:58:34, rmcilroy wrote:
> I've redo this to use code stubs which track whether code has been executed
> zero, once or multiple times. I didn't bother with a counter on the Code
object
> as this didn't seem necessary given the stubs track this implicitly.
> 
> PTAL, let me know what you think.  If generally agreeable, I'll implement for
> other architectures.
> 
> https://codereview.chromium.org/23480031/diff/87001/test/cctest/test-heap.cc
> File test/cctest/test-heap.cc (right):
> 
>
https://codereview.chromium.org/23480031/diff/87001/test/cctest/test-heap.cc#...
> test/cctest/test-heap.cc:1098: }
> I would like to test the immediate GCing of code that's never been executed,
but
> I can't figure out a way to compile, but not execute the code (removing the
call
> to "foo()" in source above seems to mean that foo doesn't get compiled at
all). 
> Any ideas?

Ping?

[danno, 2013-09-26 16:29:05]

I like this approach much more. Go ahead and do the platform ports.

https://codereview.chromium.org/23480031/diff/87001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/23480031/diff/87001/src/objects.h#newcode5154
src/objects.h:5154: kPreAgedCodeAge = kQuinquagenarianCodeAge,
// This is probably clearer if it is kIsOldCodeAge - 1.

[rmcilroy, 2013-09-30 19:38:43]

Added support for x64 and ia32. This ended up being more complex than I hoped
because on x64 I need to relocate the relative address call()'s once the code is
placed at its final address. To do this, I need to enable the RelocInfoIterator
to find the CODE_AGE_SEQUENCE relocinfo type at during Code::CopyFrom(), which
means setting a couple of the code objects setters (set_prologue_offset() and
set_has_debug_break_slots()) before Code::CopyFrom() is called.

PTAL, happy to reevaluate if there is a better way of doing this.

https://codereview.chromium.org/23480031/diff/87001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/23480031/diff/87001/src/objects.h#newcode5154
src/objects.h:5154: kPreAgedCodeAge = kQuinquagenarianCodeAge,
On 2013/09/26 16:29:06, danno wrote:
> // This is probably clearer if it is kIsOldCodeAge - 1.

Done.

[danno, 2013-10-04 16:30:08]

Getting very close.

https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:171: __ nop();
Use __ Nop(padding_size) here, lithium and on x64.

https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h#newcode216
src/v8globals.h:216: static const int kPrologueOffsetNotSet = -1;
This is probably better suited to be in codegen.h

https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:209: 
nit: remove whitespace change

[rmcilroy, 2013-10-07 10:56:12]

https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
src/ia32/full-codegen-ia32.cc:171: __ nop();
On 2013/10/04 16:30:09, danno wrote:
> Use __ Nop(padding_size) here, lithium and on x64.

Done.  Also cleaned up codegen::PatchCodeAge.

https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h#newcode216
src/v8globals.h:216: static const int kPrologueOffsetNotSet = -1;
On 2013/10/04 16:30:09, danno wrote:
> This is probably better suited to be in codegen.h

It is needed in a number of .h files (heap.h, factory.h, etc). If I move this to
codegen.h I need to add codegen.h to these headers includes, which seems to
cause a bunch of compile errors - e.g.:

In file included from ../src/gdb-jit.h:41:0,
                 from ../src/assembler.h:42,
                 from ../src/code-stubs.h:32,
                 from ../src/codegen.h:31,
                 from ../src/heap.h:35,
                 from ../src/elements.h:33,
                 from ../src/objects-inl.h:38,
                 from ../src/v8.h:61,
                 from ../src/accessors.cc:28:
../src/factory.h:460:13: error: ‘ROOT_ACCESSOR’ has not been declared
../src/factory.h:460:26: error: ISO C++ forbids declaration of ‘ROOT_LIST’ with
no type [-fpermissive]

Any ideas on how to solve this?

https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:209: 
On 2013/10/04 16:30:09, danno wrote:
> nit: remove whitespace change

Done.

[rmcilroy, 2013-10-14 12:22:25]

On 2013/10/07 10:56:12, rmcilroy wrote:
>
https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
> File src/ia32/full-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/23480031/diff/97001/src/ia32/full-codegen-ia3...
> src/ia32/full-codegen-ia32.cc:171: __ nop();
> On 2013/10/04 16:30:09, danno wrote:
> > Use __ Nop(padding_size) here, lithium and on x64.
> 
> Done.  Also cleaned up codegen::PatchCodeAge.
> 
> https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h
> File src/v8globals.h (right):
> 
> https://codereview.chromium.org/23480031/diff/97001/src/v8globals.h#newcode216
> src/v8globals.h:216: static const int kPrologueOffsetNotSet = -1;
> On 2013/10/04 16:30:09, danno wrote:
> > This is probably better suited to be in codegen.h
> 
> It is needed in a number of .h files (heap.h, factory.h, etc). If I move this
to
> codegen.h I need to add codegen.h to these headers includes, which seems to
> cause a bunch of compile errors - e.g.:
> 
> In file included from ../src/gdb-jit.h:41:0,
>                  from ../src/assembler.h:42,
>                  from ../src/code-stubs.h:32,
>                  from ../src/codegen.h:31,
>                  from ../src/heap.h:35,
>                  from ../src/elements.h:33,
>                  from ../src/objects-inl.h:38,
>                  from ../src/v8.h:61,
>                  from ../src/accessors.cc:28:
> ../src/factory.h:460:13: error: ‘ROOT_ACCESSOR’ has not been declared
> ../src/factory.h:460:26: error: ISO C++ forbids declaration of ‘ROOT_LIST’
with
> no type [-fpermissive]
> 
> Any ideas on how to solve this?
> 
>
https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64.cc
> File src/x64/full-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/23480031/diff/97001/src/x64/full-codegen-x64....
> src/x64/full-codegen-x64.cc:209: 
> On 2013/10/04 16:30:09, danno wrote:
> > nit: remove whitespace change
> 
> Done.

Ping?

[danno, 2013-10-14 18:53:14]

Can you try making this:

static const int kPrologueOffsetNotSet = -1;

a public static memory of Code in objects.h? That's better than v8globals.h

https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:167: PredictableCodeSizeScope
predictible_code_size_scope(
For this and other platforms, can you please consolidate the logic for emitting
the prologue into the macros assembler? Everything from PredictableCodeSizeScope
to the end of your change is the same for full codegen and crankshaft. You will
probably need a single parameter "for_generated_stub" to distinguish the
JSFunction pointer.

[rmcilroy, 2013-10-15 17:03:43]

On 2013/10/14 18:53:14, danno wrote:
> Can you try making this:
> 
> static const int kPrologueOffsetNotSet = -1;
> 
> a public static memory of Code in objects.h? That's better than v8globals.h

Good plan. Done.

https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm...
src/arm/full-codegen-arm.cc:167: PredictableCodeSizeScope
predictible_code_size_scope(
On 2013/10/14 18:53:14, danno wrote:
> For this and other platforms, can you please consolidate the logic for
emitting
> the prologue into the macros assembler? Everything from
PredictableCodeSizeScope
> to the end of your change is the same for full codegen and crankshaft. You
will
> probably need a single parameter "for_generated_stub" to distinguish the
> JSFunction pointer.

Done.

[rmcilroy, 2013-10-22 09:24:03]

On 2013/10/15 17:03:43, rmcilroy wrote:
> On 2013/10/14 18:53:14, danno wrote:
> > Can you try making this:
> > 
> > static const int kPrologueOffsetNotSet = -1;
> > 
> > a public static memory of Code in objects.h? That's better than v8globals.h
> 
> Good plan. Done.
> 
>
https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm.cc
> File src/arm/full-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/23480031/diff/109001/src/arm/full-codegen-arm...
> src/arm/full-codegen-arm.cc:167: PredictableCodeSizeScope
> predictible_code_size_scope(
> On 2013/10/14 18:53:14, danno wrote:
> > For this and other platforms, can you please consolidate the logic for
> emitting
> > the prologue into the macros assembler? Everything from
> PredictableCodeSizeScope
> > to the end of your change is the same for full codegen and crankshaft. You
> will
> > probably need a single parameter "for_generated_stub" to distinguish the
> > JSFunction pointer.
> 
> Done.

Ping?

[danno, 2013-10-22 12:32:11]

Almost there, sorry, one comment on your last change.

https://codereview.chromium.org/23480031/diff/117001/src/ia32/macro-assembler...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/23480031/diff/117001/src/ia32/macro-assembler...
src/ia32/macro-assembler-ia32.h:229: void Prologue(bool is_stub);
Here and other platforms: Can you please use an enum, e.g. 

enum PrologueFrameMode {
  BUILD_STUB_FRAME,
  BUILD_OPTIMIZED_FRAME
}

or something to that effect.

[danno, 2013-10-23 13:46:20]

Nice. LGTM. I'll land this for you.

[danno, 2013-10-23 13:48:31]

Committed patchset #24 manually as r17343 (presubmit successful).

[rmcilroy, 2015-03-07 22:05:02]

Patchset #25 (id:203001) has been deleted

[rmcilroy, 2015-03-22 16:55:24]

Patchset #16 (id:86001) has been deleted

[rmcilroy, 2015-03-22 16:55:42]

Patchset #5 (id:9001) has been deleted

[rmcilroy, 2015-03-22 16:55:50]

Patchset #4 (id:7001) has been deleted

[rmcilroy, 2015-03-22 16:55:58]

Patchset #3 (id:5001) has been deleted

[rmcilroy, 2015-03-22 16:56:01]

Patchset #2 (id:3001) has been deleted

[rmcilroy, 2015-03-22 16:56:07]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-03-22 16:56:40]

Patchset #2 (id:16001) has been deleted