Collect only optimizable function samples....

Collect only optimizable function samples.

Keep track of the ratio between JS and non-JS ticks and use this ratio to adjust the lookup threshold. (Also add support to trace compilation statistics.)



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

[Karl Klose, 2010-12-08 12:57:28]

http://golem.spb:9013/v8/r5946-v8-karlklose-jsratio.html#g

[Karl Klose, 2010-12-08 14:00:49]

New version that fixes the Kraken regression.

[fschneider, 2010-12-08 14:33:32]

drive-by:

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

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:305: if (js_ratio < .2) {
These constants need some comments about the rationale behind.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:402: in_JS_state--;
is_js_state vs in_JS_state: These variable names are confusing. Please choose
something that does not only differ in capitalization.

[Kasper Lund, 2010-12-08 14:54:15]

This is getting there.

It's somewhat confusing that we have two concepts of samples: One is the state
samples and one is the frame/function samples. 

It also might make sense to incorporate (manually inline) the code from
AddStateSample into the only caller of it and move all the static variables it
uses to keep track of state into the function so they are not visible to the
outside. Then introduce a single static Atomic32 variable that you update with
the right "barrier" instructions as the output of the computation (basically the
ratio).

http://codereview.chromium.org/5633009/diff/14001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5633009/diff/14001/src/compiler.cc#newcode116
src/compiler.cc:116: static double compilation_time_;
Convert these to statics inside the if (FLAG_trace_opt) { ... } in the
FinishOptimization method? Please initialize them to 0.

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

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:76: static const int kSamplerTicksDelta = 32;
kSamplerTicksBetweenThresholdAdjustment.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:92: static bool state_window[kStateWindowSize];
To me the name is of this is somewhat weird. If it's a state window it should
contain states. Maybe it would be nicer with an enum SampleState { JS, NON_JS }
and have a window of those? It's not clear what true means - and what false
means.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:94: static int in_JS_state = 0;
static int state_counts[2] = { 0, } and then use state_counts[state] to get to
either in_JS_state or not_in_JS_state.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:97: static int sampler_ticks = kSamplerTicksDelta;
This name doesn't really relay much information. It's the number of ticks? Hmm -
how about something ala 'sampler_ticks_until_threshold_adjustment'?

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:267: float js_ratio = in_JS_state / static_cast<float>(
Maybe comment on what this ratio means or change the variable name to make it
(even) more obvious.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:281: if (sampler_ticks > 0) {
Add a comment that explains what this does.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:291: if (!IsOptimizable(function) &&
!function->IsMarkedForLazyRecompilation()) {
Please explain what you're trying to filter out here through a comment.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:299: threshold_size_factor = (function_size >
kSizeLimit)
Why not initialize threshold_size_factor directly in the declaration of the
variable?

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:305: if (js_ratio < .2) {
0.2, 0.5, 0.75. I agree that you should explain the rationale behind the
threshold adjustments. Why does it make sense?

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:400: void AddStateSample(bool in_js_state) {
Use enum SampleState for the parameter.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:400: void AddStateSample(bool in_js_state) {
Make this function static.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:401: if (state_window[state_window_position]) {
state_counts[state_window[state_window_position]]--; No branches.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:407: if (in_js_state) {
state_counts[state]++; No branches.

http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:412: state_window_position = (state_window_position + 1)
&
ASSERT(IsPowerOf2(kStateWindowSize));

[karlklose, 2010-12-09 10:09:45]

> http://codereview.chromium.org/5633009/diff/14001/src/compiler.cc#newcode116
> src/compiler.cc:116: static double compilation_time_;
> Convert these to statics inside the if (FLAG_trace_opt) { ... } in the
> FinishOptimization method? Please initialize them to 0.
Done. 

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:76: static const int kSamplerTicksDelta = 32;
> kSamplerTicksBetweenThresholdAdjustment.
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:92: static bool state_window[kStateWindowSize];
> To me the name is of this is somewhat weird. If it's a state window it should
> contain states. Maybe it would be nicer with an enum SampleState { JS, NON_JS
}
> and have a window of those? It's not clear what true means - and what false
> means.
Done using enum.
 
>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:94: static int in_JS_state = 0;
> static int state_counts[2] = { 0, } and then use state_counts[state] to get to
> either in_JS_state or not_in_JS_state.
Done.
 
>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:97: static int sampler_ticks = kSamplerTicksDelta;
> This name doesn't really relay much information. It's the number of ticks? Hmm
-
> how about something ala 'sampler_ticks_until_threshold_adjustment'?
Done.
 
>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:267: float js_ratio = in_JS_state /
static_cast<float>(
> Maybe comment on what this ratio means or change the variable name to make it
> (even) more obvious.
Comment on js_ratio.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:281: if (sampler_ticks > 0) {
> Add a comment that explains what this does.
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:291: if (!IsOptimizable(function) &&
> !function->IsMarkedForLazyRecompilation()) {
> Please explain what you're trying to filter out here through a comment.
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:299: threshold_size_factor = (function_size >
> kSizeLimit)
> Why not initialize threshold_size_factor directly in the declaration of the
> variable?
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:305: if (js_ratio < .2) {
> 0.2, 0.5, 0.75. I agree that you should explain the rationale behind the
> threshold adjustments. Why does it make sense?
I removed the .5 case, it does not make much difference. Added comment.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:400: void AddStateSample(bool in_js_state) {
> Use enum SampleState for the parameter.
Done.
 
>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:400: void AddStateSample(bool in_js_state) {
> Make this function static.
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:401: if (state_window[state_window_position]) {
> state_counts[state_window[state_window_position]]--; No branches.
Done.

>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:407: if (in_js_state) {
> state_counts[state]++; No branches.
Done.
 
>
http://codereview.chromium.org/5633009/diff/14001/src/runtime-profiler.cc#new...
> src/runtime-profiler.cc:412: state_window_position = (state_window_position +
1)
> &
> ASSERT(IsPowerOf2(kStateWindowSize));
Done.

[Kasper Lund, 2010-12-09 11:51:51]

Much better. Comments:

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc#newcode127
src/compiler.cc:127: if (FLAG_trace_opt) {
Remove this check. FLAG_trace_opt is true at this point - otherwise we will have
returned. Consider adding a FLAG_trace_opt_verbose and check that flag instead.

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc#newcode129
src/compiler.cc:129: static int compiled_functions =0;
=0 -> = 0

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

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:315: // If we spent less than 20% of the time in JS,
code
spent => spend

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:324: if (function->IsMarkedForLazyRecompilation()) {
Can't you just move this code up to the !IsOptimizable check and avoid the
repeated IsMarkedForLazyRecompilation checks?

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:420: js_ratio = state_counts[IN_JS_STATE]*100 /
kStateWindowSize;
Spaces around *. Consider changing the type of js_ratio to Atomic32 and use the
NoBarrier_Store(&js_ratio, ...) here and NoBarrier_Load(&js_ratio, ...) in
OptimizeNow to load it into a local variable.

[Karl Klose, 2010-12-09 12:49:32]

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc#newcode127
src/compiler.cc:127: if (FLAG_trace_opt) {
On 2010/12/09 11:51:51, Kasper Lund wrote:
> Remove this check. FLAG_trace_opt is true at this point - otherwise we will
have
> returned. Consider adding a FLAG_trace_opt_verbose and check that flag
instead.

Done.

http://codereview.chromium.org/5633009/diff/23002/src/compiler.cc#newcode129
src/compiler.cc:129: static int compiled_functions =0;
On 2010/12/09 11:51:51, Kasper Lund wrote:
> =0 -> = 0

Done.

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

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:315: // If we spent less than 20% of the time in JS,
code
On 2010/12/09 11:51:51, Kasper Lund wrote:
> spent => spend

Done.

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:324: if (function->IsMarkedForLazyRecompilation()) {
On 2010/12/09 11:51:51, Kasper Lund wrote:
> Can't you just move this code up to the !IsOptimizable check and avoid the
> repeated IsMarkedForLazyRecompilation checks?

Done.

http://codereview.chromium.org/5633009/diff/23002/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:420: js_ratio = state_counts[IN_JS_STATE]*100 /
kStateWindowSize;
On 2010/12/09 11:51:51, Kasper Lund wrote:
> Spaces around *. Consider changing the type of js_ratio to Atomic32 and use
the
> NoBarrier_Store(&js_ratio, ...) here and NoBarrier_Load(&js_ratio, ...) in
> OptimizeNow to load it into a local variable.

Done.

[Kasper Lund, 2010-12-09 13:02:37]

LGTM!

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

http://codereview.chromium.org/5633009/diff/46001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:286: if (sampler_ticks_until_threshold_adjustment == 0)
{
Maybe make this <= 0 to be defensive.

http://codereview.chromium.org/5633009/diff/46001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:306: if (!IsOptimizable(function)) {
I would put this on one line.

http://codereview.chromium.org/5633009/diff/46001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:317: 
Remove this newline and add one below the line defining current_js_ratio.