Initial support for count-based profiling

Initial support for count-based profiling

(behind FLAG_count_based_interrupts; only on ia32)

Committed: https://code.google.com/p/v8/source/detail?r=10699

[Jakob Kummerow, 2012-02-09 16:08:41]

A few more new flags :-)
Please take a look.

This CL looks scarier than it is; the line count is actually pretty small.

Only ia32 has a complete implementation of --count-based-interrupts; I've
modified the other architectures just enough to make them compile.

[Erik Corry, 2012-02-10 13:01:42]

https://chromiumcodereview.appspot.com/9373028/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9373028/diff/1/src/full-codegen.h#newc...
src/full-codegen.h:427: void EmitStackCheck(IterationStatement* stmt, Label*
back_edge_target);
Please add a comment to explain what this label is used for.

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:1663: // stack guard check to enable interrupts.
This should read "to trigger any pending interrupt".

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:1666: if (FLAG_count_based_interrupts) {
Don't think we need this complexity.

[Jakob Kummerow, 2012-02-10 13:59:20]

Thanks for the comments, please take another look.

https://chromiumcodereview.appspot.com/9373028/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/9373028/diff/1/src/full-codegen.h#newc...
src/full-codegen.h:427: void EmitStackCheck(IterationStatement* stmt, Label*
back_edge_target);
On 2012/02/10 13:01:42, Erik Corry wrote:
> Please add a comment to explain what this label is used for.

Done.

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:1663: // stack guard check to enable interrupts.
On 2012/02/10 13:01:42, Erik Corry wrote:
> This should read "to trigger any pending interrupt".

Done.

https://chromiumcodereview.appspot.com/9373028/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:1666: if (FLAG_count_based_interrupts) {
On 2012/02/10 13:01:42, Erik Corry wrote:
> Don't think we need this complexity.

Done.

[Vyacheslav Egorov (Chromium), 2012-02-14 12:52:47]

lgtm with comments addressed

also please consider adding test that enables the flag and does some loopy
thingy to ensure that OSR is working correctly (and it will not invisibly break
if somebody touches something in OSR).

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/deoptimizer...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/deoptimizer...
src/ia32/deoptimizer-ia32.cc:234: ASSERT(*(call_target_address - 2) == 0x11); 
// offset
Please name magical constants. they are duplicated in the code.

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:337: __ sub(Operand::Cell(profiling_counter_),
generation of the sub can be moved out of the if and inside the if you can just
compute int step

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:347: ExternalReference stack_limit =
It is not clear to me why stack check sits in the else branch. 

It is a conscious decision to postpone interrupts until countdown reaches zero?

If so please add a comment about it.

[Vyacheslav Egorov (Chromium), 2012-02-14 12:52:54]

lgtm with comments addressed

also please consider adding test that enables the flag and does some loopy
thingy to ensure that OSR is working correctly (and it will not invisibly break
if somebody touches something in OSR).

[Jakob Kummerow, 2012-02-14 13:54:45]

Thanks for the review. I've addressed your comments and added a test.

Landing, unless you have further comments.

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/deoptimizer...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/deoptimizer...
src/ia32/deoptimizer-ia32.cc:234: ASSERT(*(call_target_address - 2) == 0x11); 
// offset
On 2012/02/14 12:52:47, Vyacheslav Egorov wrote:
> Please name magical constants. they are duplicated in the code.

Done.

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:337: __ sub(Operand::Cell(profiling_counter_),
On 2012/02/14 12:52:47, Vyacheslav Egorov wrote:
> generation of the sub can be moved out of the if and inside the if you can
just
> compute int step

Done.

https://chromiumcodereview.appspot.com/9373028/diff/9001/src/ia32/full-codege...
src/ia32/full-codegen-ia32.cc:347: ExternalReference stack_limit =
On 2012/02/14 12:52:47, Vyacheslav Egorov wrote:
> It is not clear to me why stack check sits in the else branch. 
> 
> It is a conscious decision to postpone interrupts until countdown reaches
zero?
> 
> If so please add a comment about it.

Done.