New approach to Crankshaft decision-making

New approach to Crankshaft decision-making

- make the profiler consider IC patching
- variable tick interval on ARM/MIPS
- primitive functions trigger their optimization themselves


Obsoleted/replaced by a series of other CLs.

[Jakob Kummerow, 2011-11-25 16:51:14]

First version. Building with snapshot is currently broken.

This depends on the definition of "primitive" functions from
http://codereview.chromium.org/8677008/.

Danno: WDYT?
Kevin: FYI. Feel free to comment if you care.

[Jakob Kummerow, 2011-12-06 13:59:07]

OK, I think this is really ready for review now :-)

Review guidance:
The most important changes are in runtime-profiler.cc, with corresponding
updates to runtime-profiler.h.

To enable the new optimization decisions, a bunch of changes were necessary
elsewhere:

1.) Track profiler ticks per function, meaning "how often has the profiler seen
this function on the stack since its last IC patching?" -->
code-stubs-<ARCH>.cc, heap.cc, ic-inl.h, objects.h, objects-inl.h

2.) Make primitive functions trigger their own optimization. For that, they need
to keep track of how often they've been called. --> full-codegen-<ARCH>.cc,
heap.cc, objects.h, objects-inl.h

3.) Inform the profiler about code generation events --> full-codegen.cc

4.) The GC now clears ICs only if Context::Exit() was recently executed. -->
api.cc, isolate.{h,cc}, mark-compact.cc

5.) Variable tick interval for ARM and MIPS --> log.{h,cc}, platform.h,
platform-<PLATFORM>.cc

In ic.cc, there's another change that's not really related to the rest: If we
see a COW array the very first time after an KeyedStoreIC miss, use the generic
stub immediately.

[danno, 2011-12-07 13:21:52]

One comment from a quick first pass

http://codereview.chromium.org/8700008/diff/3001/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/8700008/diff/3001/src/platform-linux.cc#newcod...
src/platform-linux.cc:1114: if (tick_count_ == 5) {
Constants for these?

[Jakob Kummerow, 2011-12-12 10:02:38]

http://codereview.chromium.org/8700008/diff/3001/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/8700008/diff/3001/src/platform-linux.cc#newcod...
src/platform-linux.cc:1114: if (tick_count_ == 5) {
On 2011/12/07 13:21:52, danno wrote:
> Constants for these?

Done.

[fschneider, 2011-12-12 11:31:21]

Here is my first comments:

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:136: info->function()->is_primitive() &&
What is a primitive function? I can't find the source code for this predicate.

http://codereview.chromium.org/8700008/diff/9001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8700008/diff/9001/src/objects.h#newcode5464
src/objects.h:5464: inline int profiler_ticks();
Shouldn't be this on SharedFunctionInfo instead?

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc
File src/runtime-profiler.cc (left):

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#oldc...
src/runtime-profiler.cc:59: static const int kSizeLimit = 1500;
The reason for looking at the size in some form was that large functions are
more expensive to optimize, so we want to be sure they're worth optimizing by
requiring more profiler ticks. The metric used is maybe not ideal, but some form
of taking size into account would be still useful. It seemed to help on ia32.

[fschneider, 2011-12-13 11:53:00]

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:135: if (FLAG_crankshaft &&
If a function has optimization disabled via DisableOptimization() because we
can't optimize it or we deoptimized frequently, we should not add code for
counting.

http://codereview.chromium.org/8700008/diff/9001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8700008/diff/9001/src/objects.h#newcode5464
src/objects.h:5464: inline int profiler_ticks();
On 2011/12/12 11:31:21, fschneider wrote:
> Shouldn't be this on SharedFunctionInfo instead?

This increases the size of each JSFunction by 1 word  from 36 to 40 (11%). This
could hurt memory-constrained devices on programs with lots of closures. I'd
prefer a solution without additional space cost.

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#newc...
src/runtime-profiler.cc:82: PrintF(" for recompilation, reason: %d", reason);
Print reason in English instead of just a number.

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#newc...
src/runtime-profiler.cc:163: Optimize(function, 1);
Name instead of an integer constant?

[Jakob Kummerow, 2011-12-14 08:42:31]

New patch set uploaded that addresses comments.

Moving the profiler_ticks field to SharedFunctionInfo might affect performance;
proper testing is currently blocked on an unrelated bug that is triggered when
running earley-boyer.

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:135: if (FLAG_crankshaft &&
On 2011/12/13 11:53:01, fschneider wrote:
> If a function has optimization disabled via DisableOptimization() because we
> can't optimize it or we deoptimized frequently, we should not add code for
> counting.

Done.

http://codereview.chromium.org/8700008/diff/9001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:136: info->function()->is_primitive() &&
On 2011/12/12 11:31:21, fschneider wrote:
> What is a primitive function? I can't find the source code for this predicate.

Basically a function that doesn't contain loops, calls, or un-inline-able
statements. The definition is in http://codereview.chromium.org/8677008/

http://codereview.chromium.org/8700008/diff/9001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8700008/diff/9001/src/objects.h#newcode5464
src/objects.h:5464: inline int profiler_ticks();
On 2011/12/13 11:53:01, fschneider wrote:
> On 2011/12/12 11:31:21, fschneider wrote:
> > Shouldn't be this on SharedFunctionInfo instead?
> 
> This increases the size of each JSFunction by 1 word  from 36 to 40 (11%).
This
> could hurt memory-constrained devices on programs with lots of closures. I'd
> prefer a solution without additional space cost.
> 

Done.

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc
File src/runtime-profiler.cc (left):

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#oldc...
src/runtime-profiler.cc:59: static const int kSizeLimit = 1500;
On 2011/12/12 11:31:21, fschneider wrote:
> The reason for looking at the size in some form was that large functions are
> more expensive to optimize, so we want to be sure they're worth optimizing by
> requiring more profiler ticks. The metric used is maybe not ideal, but some
form
> of taking size into account would be still useful. It seemed to help on ia32.

I see the reasoning and agree in principle, but with the new approach I haven't
been able to measure any benefit to considering the function size.

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#newc...
src/runtime-profiler.cc:82: PrintF(" for recompilation, reason: %d", reason);
On 2011/12/13 11:53:01, fschneider wrote:
> Print reason in English instead of just a number.

Done.

http://codereview.chromium.org/8700008/diff/9001/src/runtime-profiler.cc#newc...
src/runtime-profiler.cc:163: Optimize(function, 1);
On 2011/12/13 11:53:01, fschneider wrote:
> Name instead of an integer constant?

Done.