Fix JS ratio computation on startup.

Fix JS ratio computation on startup.

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

[Vyacheslav Egorov (Chromium), 2011-04-09 17:00:21]

LGTM

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

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode135
src/runtime-profiler.cc:135: state_counts_[IN_NON_JS_STATE] = kStateWindowSize;
Wild idea: when going idle we stop profiler. Why don't we reset it's state when
we later restart it? 

We should experiment with this.

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode349
src/runtime-profiler.cc:349: state_window_ticks_ = Min(kStateWindowSize,
state_window_ticks_ + 1);
Our RuntimeProfiler is full of magic and very hard to grok. Lets start adding
comments. 

For example here you can mention that when we start profiling we don't have
enough ticks for full window (kStateWindowSize) so to calculate proper ratio we
have to use smaller window size equal to amount of ticks we got.

[Vitaly Repeshko, 2011-04-10 08:13:27]

Thanks for having a look!


-- Vitaly

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

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode135
src/runtime-profiler.cc:135: state_counts_[IN_NON_JS_STATE] = kStateWindowSize;
On 2011/04/09 17:00:21, Vyacheslav Egorov wrote:
> Wild idea: when going idle we stop profiler. Why don't we reset it's state
when
> we later restart it? 
> 
> We should experiment with this.

I agree. As we discussed, we should revisit the heuristics behind Reset().

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode349
src/runtime-profiler.cc:349: state_window_ticks_ = Min(kStateWindowSize,
state_window_ticks_ + 1);
On 2011/04/09 17:00:21, Vyacheslav Egorov wrote:
> Our RuntimeProfiler is full of magic and very hard to grok. Lets start adding
> comments. 
> 
> For example here you can mention that when we start profiling we don't have
> enough ticks for full window (kStateWindowSize) so to calculate proper ratio
we
> have to use smaller window size equal to amount of ticks we got.

It's not too late to make it readable :) Added a comment covering this aspect.

[Kasper Lund, 2011-04-11 06:10:06]

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

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode135
src/runtime-profiler.cc:135: state_counts_[IN_NON_JS_STATE] = kStateWindowSize;
Before this change, we were intentionally being less aggressive in our
optimizations initially (slow start). It could definitely be that the rest of
our heuristics have changed so much that it doesn't matter anymore, but it would
be nice to verify that this doesn't force us to optimize too much stuff when
loading real web apps. 

Somehow the new code bothers me a bit. Because the state_counts_[IN_JS_STATE] is
initialized to zero, you can compute the JS ratio by dividing with
state_window_ticks_ but you cannot do the same for the non-JS ratio (initialized
to kStateWindowSize). Would it be cleaner if state_counts_[IN_NON_JS_STATE] was
initialized to zero too and only do state_counts_[old_state]-- when
state_window_ticks_ indicated a full window? That way you could also get rid of
the STATIC_ASSERT and the memset.

[Vitaly Repeshko, 2011-04-11 19:43:53]

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

http://codereview.chromium.org/6826026/diff/1/src/runtime-profiler.cc#newcode135
src/runtime-profiler.cc:135: state_counts_[IN_NON_JS_STATE] = kStateWindowSize;
On 2011/04/11 06:10:06, Kasper Lund wrote:
> Before this change, we were intentionally being less aggressive in our
> optimizations initially (slow start). It could definitely be that the rest of
> our heuristics have changed so much that it doesn't matter anymore, but it
would
> be nice to verify that this doesn't force us to optimize too much stuff when
> loading real web apps. 
> 
> Somehow the new code bothers me a bit. Because the state_counts_[IN_JS_STATE]
is
> initialized to zero, you can compute the JS ratio by dividing with
> state_window_ticks_ but you cannot do the same for the non-JS ratio
(initialized
> to kStateWindowSize). Would it be cleaner if state_counts_[IN_NON_JS_STATE]
was
> initialized to zero too and only do state_counts_[old_state]-- when
> state_window_ticks_ indicated a full window? That way you could also get rid
of
> the STATIC_ASSERT and the memset.

The slow start is still here, because even if we optimize a lot, we spend quite
some time just compiling stuff and it's a separate VM state. So it's not like we
immediately jump to 100% of JS ratio.

I'm not quite happy with the window update code either. I'll work more on it.