Fix bug that disabled optimization when profiling.

Fix bug that disabled optimization when profiling.


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

[Florian Schneider, 2010-12-10 14:02:55]

LGTM.

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

http://codereview.chromium.org/5720003/diff/1/src/runtime-profiler.cc#newcode429
src/runtime-profiler.cc:429: if (top_is_in_js_state) {
Do you need to check for RuntimeProfiler::IsEnabled() here too?

[Vitaly Repeshko, 2010-12-10 14:05:35]

It seems wrong that state window tracking is done in SuspendIfNecessary. Why not
move this to NotifyTick instead? Added bonus: this way it'll only be used when
the runtime profiler is enabled.

Yet the suspend threshold and the state window size are related. I think, the
threshold should exceed the size.

[Karl Klose, 2010-12-10 14:08:48]

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

http://codereview.chromium.org/5720003/diff/1/src/runtime-profiler.cc#newcode429
src/runtime-profiler.cc:429: if (top_is_in_js_state) {
On 2010/12/10 14:02:55, fschneider1 wrote:
> Do you need to check for RuntimeProfiler::IsEnabled() here too?

Done.

[Karl Klose, 2010-12-10 14:12:56]

> It seems wrong that state window tracking is done in SuspendIfNecessary. Why
not
> move this to NotifyTick instead? Added bonus: this way it'll only be used when
> the runtime profiler is enabled.
We also want to sample the state when the runtime profiler is suspended, but
NotifyTick is not called in this case.

[Karl Klose, 2010-12-10 14:19:30]

On 2010/12/10 14:12:56, Karl Klose wrote:
> > It seems wrong that state window tracking is done in SuspendIfNecessary. Why
> not
> > move this to NotifyTick instead? Added bonus: this way it'll only be used
when
> > the runtime profiler is enabled.
> We also want to sample the state when the runtime profiler is suspended, but
> NotifyTick is not called in this case.
SuspendIfNecessary is called on every tick and returns true if the runtime
profiler should not be called. The calling code is in
Sampler::PlatformData::SignalSender()

[Vitaly Repeshko, 2010-12-10 14:40:40]

LGTM


Thanks,
Vitaly

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

http://codereview.chromium.org/5720003/diff/12001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:353: static void AddStateSample(SamplerState
current_state) {
Rename to "UpdateStateRatio"?

http://codereview.chromium.org/5720003/diff/12001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:374: AddStateSample(IN_JS_STATE);
Nice place to use ?: operator. Up to you though.

[Karl Klose, 2010-12-10 14:45:06]

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

http://codereview.chromium.org/5720003/diff/12001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:353: static void AddStateSample(SamplerState
current_state) {
On 2010/12/10 14:40:40, Vitaly wrote:
> Rename to "UpdateStateRatio"?

Done.

http://codereview.chromium.org/5720003/diff/12001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:374: AddStateSample(IN_JS_STATE);
On 2010/12/10 14:40:40, Vitaly wrote:
> Nice place to use ?: operator. Up to you though.

Done.