Count-based profiling for primitive functions (hidden behind a flag)

Count-based profiling for primitive functions (hidden behind a flag)

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

[Jakob Kummerow, 2012-02-08 11:51:40]

Please take a look.

This is the first CL in a series towards having count-based profiling. It adds a
"self-optimization" header to primitive functions, and propagates a few bits of
information about compilation and IC patching events to the runtime profiler.

The most interesting changes are:
full-codegen-{arm,ia32,x64}.cc: generating the self-optimization header
runtime-profiler.{h,cc}: new behavior if FLAG_counting_profiler; old code moved
to "else {...}" sections.
ic-inl.h: Mark functions on the stack as not-yet-stable (in the type information
sense) when an IC is patched.

[Erik Corry, 2012-02-08 14:09:23]

The patch looks incomplete.  Where is the ShouldSelfOptimize function?

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:135: if (FLAG_counting_profiler &&
These two flags can be tested inside ShouldSelfOptimize for a reduction in
verbosity.  Perhaps the flags on the shared info can also be tested there?

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:151: Immediate(Smi::FromInt(1)));
It seems you can do:

__ sub(Operand::Cell(Handle<JSGlobalPropertyCell>(cell)), Immediate(...))

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:154: __ j(zero, compile_stub);
There is an assumption here that the Smi tag is zero and that we decrement one
at a time.  I would prefer j(mi, ... instead.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h
File src/ic-inl.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h#newcode94
src/ic-inl.h:94: if (FLAG_counting_profiler) {
Suggested comment:

// We do not want to optimize until the ICs have settled down
// so when they are patched, we postpone optimization for the current function
// and the functions above it on the stack that might want to
// inline this one.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h#newcode5388
src/objects.h:5388: kThisPropertyAssignmentsOffset + kPointerSize;
This is a Smi field.  Can we find another one and put them both in one field on
64 bit like we do for the ones below?

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:50: // Constants for old profiler.
s/old/statistical/

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:63: // Constants for new profiler.
s/new/counter based/

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:71: static const int kSizeLimitEarlyOpt = 500;
Name should contain 'Max'

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:238: int threshold =
function->shared()->ast_node_count() > 300
constant needs a name

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h
File src/runtime-profiler.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h#...
src/runtime-profiler.h:66: }
Missing blank line.

[Erik Corry, 2012-02-08 14:13:29]

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

https://chromiumcodereview.appspot.com/9361026/diff/1/src/full-codegen.cc#new...
src/full-codegen.cc:300: if (FLAG_counting_profiler) {
I suggest this condition could be folded into NotifyCodeGenerated

[Jakob Kummerow, 2012-02-08 15:24:07]

Thank you for the review. I have uploaded a new patch set.


On 2012/02/08 14:09:23, Erik Corry wrote:
> The patch looks incomplete.  Where is the ShouldSelfOptimize function?

Here:
http://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/ast.cc#171
But I have moved it to CompilationInfo now (see comments below).

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

https://chromiumcodereview.appspot.com/9361026/diff/1/src/full-codegen.cc#new...
src/full-codegen.cc:300: if (FLAG_counting_profiler) {
On 2012/02/08 14:13:30, Erik Corry wrote:
> I suggest this condition could be folded into NotifyCodeGenerated

Done.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:135: if (FLAG_counting_profiler &&
On 2012/02/08 14:09:23, Erik Corry wrote:
> These two flags can be tested inside ShouldSelfOptimize for a reduction in
> verbosity.  Perhaps the flags on the shared info can also be tested there?

Good idea to clean this up. I've moved ShouldSelfOptimize() to CompilationInfo,
where all the required information for this decision is accessible.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:151: Immediate(Smi::FromInt(1)));
On 2012/02/08 14:09:23, Erik Corry wrote:
> It seems you can do:
> 
> __ sub(Operand::Cell(Handle<JSGlobalPropertyCell>(cell)), Immediate(...))

Done.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:154: __ j(zero, compile_stub);
On 2012/02/08 14:09:23, Erik Corry wrote:
> There is an assumption here that the Smi tag is zero and that we decrement one
> at a time.  I would prefer j(mi, ... instead.

I've added an ASSERT for the Smi tag.
I can't easily change the jump condition because we intentionally don't reset
the countdown. After it has fired,  should we ever come back (e.g. after deopt),
we simply ignore it (until it overflows and reaches zero again after 2 billion
executions).

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h
File src/ic-inl.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h#newcode94
src/ic-inl.h:94: if (FLAG_counting_profiler) {
On 2012/02/08 14:09:23, Erik Corry wrote:
> Suggested comment:
> 
> // We do not want to optimize until the ICs have settled down
> // so when they are patched, we postpone optimization for the current function
> // and the functions above it on the stack that might want to
> // inline this one.

Done.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h#newcode5388
src/objects.h:5388: kThisPropertyAssignmentsOffset + kPointerSize;
On 2012/02/08 14:09:23, Erik Corry wrote:
> This is a Smi field.  Can we find another one and put them both in one field
on
> 64 bit like we do for the ones below?

I'd love to, but I don't see any other Smi fields. Don't worry, though, there
will be more in the future :-)

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:50: // Constants for old profiler.
On 2012/02/08 14:09:23, Erik Corry wrote:
> s/old/statistical/

Done.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:63: // Constants for new profiler.
On 2012/02/08 14:09:23, Erik Corry wrote:
> s/new/counter based/

Done. 
(However, given that this profiler is in its infancy, I'm not sure that "counter
based" will be its most precise characterization. I kinda liked the flexibility
in meaning that "new" provides ;-)

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:71: static const int kSizeLimitEarlyOpt = 500;
On 2012/02/08 14:09:23, Erik Corry wrote:
> Name should contain 'Max'

Done.

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc...
src/runtime-profiler.cc:238: int threshold =
function->shared()->ast_node_count() > 300
On 2012/02/08 14:09:23, Erik Corry wrote:
> constant needs a name

Done. (By removing it -- it's not clear yet whether this additional condition
causes any improvement.)

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h
File src/runtime-profiler.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h#...
src/runtime-profiler.h:66: }
On 2012/02/08 14:09:23, Erik Corry wrote:
> Missing blank line.

Done.

[Erik Corry, 2012-02-09 09:43:33]

LGTM

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
File src/arm/full-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
src/arm/full-codegen-arm.cc:144: // for the
SharedFunctionInfo::kCallsUntilPrimitiveOptimization-th time.
I think this comment should be moved into ShouldSelfOptimize, where it actually
describes the logic used to make the decision.  Then it is in the right place
for when we change the heuristic.

Here we should have a more generic comment like:

// We can optionally optimize based on counters rather than statistical
sampling.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
src/arm/full-codegen-arm.cc:146: if (FLAG_trace_opt) {
I think this might be annoyingly verbose, but we can add a trace-opt-verbose
flag later if we need to.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h
File src/ic-inl.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h#newcode93
src/ic-inl.h:93: target);
Confusing that this is under the counting_profiler flag when it relates to the
statistically sampling profiler.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h#newcode94
src/ic-inl.h:94: if (FLAG_counting_profiler) {
This function is getting too beefy for an inline function that is used 5 times. 
I suggest this part be moved to a helper function in the .cc file.

[Jakob Kummerow, 2012-02-09 10:13:19]

Comments addressed, landing.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
File src/arm/full-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
src/arm/full-codegen-arm.cc:144: // for the
SharedFunctionInfo::kCallsUntilPrimitiveOptimization-th time.
On 2012/02/09 09:43:33, Erik Corry wrote:
> I think this comment should be moved into ShouldSelfOptimize, where it
actually
> describes the logic used to make the decision.  Then it is in the right place
> for when we change the heuristic.
> 
> Here we should have a more generic comment like:
> 
> // We can optionally optimize based on counters rather than statistical
> sampling.

Done.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/arm/full-codegen...
src/arm/full-codegen-arm.cc:146: if (FLAG_trace_opt) {
On 2012/02/09 09:43:33, Erik Corry wrote:
> I think this might be annoyingly verbose, but we can add a trace-opt-verbose
> flag later if we need to.

As discussed, let's leave it for now. We can always remove it later.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h
File src/ic-inl.h (right):

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h#newcode93
src/ic-inl.h:93: target);
On 2012/02/09 09:43:33, Erik Corry wrote:
> Confusing that this is under the counting_profiler flag when it relates to the
> statistically sampling profiler.

As discussed, I'll split the flag in a separate CL.

https://chromiumcodereview.appspot.com/9361026/diff/3002/src/ic-inl.h#newcode94
src/ic-inl.h:94: if (FLAG_counting_profiler) {
On 2012/02/09 09:43:33, Erik Corry wrote:
> This function is getting too beefy for an inline function that is used 5
times. 
> I suggest this part be moved to a helper function in the .cc file.

Done.