Isolatify CPU profiler

Isolatify CPU profiler


BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=13987

[yurys, 2013-03-19 10:42:05]

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h#newcode192
src/cpu-profiler.h:192: if (cpu_profiler->is_profiling()) {                     
 \
I don't think we have to check if cpu_profiler is not NULL here as PROFILE macro
is only used in the places where isolate is known to be initialized and not yet
torn down.

[loislo, 2013-03-19 11:23:34]

lgtm

[Jakob Kummerow, 2013-03-19 11:42:14]

lgtm

[alph, 2013-03-19 11:44:28]

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:281: return NULL;
Is it intended to return NULL all the time?

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h#newcode191
src/cpu-profiler.h:191: CpuProfiler* cpu_profiler = isolate->cpu_profiler();    
 \
nit: put macro argument in parentheses

[Sven Panne, 2013-03-19 11:54:53]

LGTM with a tiny style nit. It would be great if that "Isolatification" was
pushed through to our external API by adding variants with an explicit Isolate
parameter, deprecating the old entries when Chrome is fixed accordingly. We do
this all the time for the core v8 API.

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h#newcode188
src/cpu-profiler.h:188: #define PROFILE(isolate, Call)                          
     \
Don't use a macro parameter more than once, assign to a local variable with a
strange name (ugly, but better than using the variable more than once). Is there
a deep reason why this macro can't be an inline function?

[yurys, 2013-03-19 12:10:54]

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:281: return NULL;
On 2013/03/19 11:44:28, alph wrote:
> Is it intended to return NULL all the time?

Good catch! Fixed.

[yurys, 2013-03-19 12:25:11]

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h
File src/cpu-profiler.h (right):

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h#newcode188
src/cpu-profiler.h:188: #define PROFILE(isolate, Call)                          
     \
On 2013/03/19 11:54:53, Sven Panne wrote:
> Don't use a macro parameter more than once, assign to a local variable with a
> strange name (ugly, but better than using the variable more than once).
Done.

> Is there a deep reason why this macro can't be an inline function?
Can you elaborate on how the inline function would look like given that the
macro accepts set of parameter as "Call" argument? I think we can get logger to
forward some event notifications to the CpuProfiler and get rid of the macro
that does just this but that would introduce additional depedency from Logger on
CpuProfiler.

https://codereview.chromium.org/12706020/diff/4002/src/cpu-profiler.h#newcode191
src/cpu-profiler.h:191: CpuProfiler* cpu_profiler = isolate->cpu_profiler();    
 \
On 2013/03/19 11:44:28, alph wrote:
> nit: put macro argument in parentheses

Done.

[yurys, 2013-03-19 12:38:09]

On 2013/03/19 11:54:53, Sven Panne wrote:
> LGTM with a tiny style nit. It would be great if that "Isolatification" was
> pushed through to our external API by adding variants with an explicit Isolate
> parameter, deprecating the old entries when Chrome is fixed accordingly. We do
> this all the time for the core v8 API.
> 
Let me do this in a separate patch.

[yurys, 2013-03-19 12:44:43]

Committed patchset #5 manually as r13987.