[not for commit] V8 profiler API preview.

[not for commit] V8 profiler API preview.

This API is enough for getting JS-only profiles (for DevTools.)
In order to replace tick processor scripts, it will be needed to
add the following:

 - import of address -> function maps for C / C++ code;
 - retrieval of processing statistics (unresolved ticks, etc.);
 - filtering (for filtering ICs and by VM state);
 - import and export of stubs / builtins names from snapshots;

See http://codereview.chromium.org/1547023.

[mnaganov (inactive), 2010-03-22 15:29:12]

Mads and Soeren,

Can you please take a look at the API for the C++ profiler? I think, it will
make sense to commit it together with wiring new profiles processor to VM, so
API will be functional.

As for switching between old and new implementation, I suggest that I'll
introduce a preprocessor directive (because using a flag may have a significant
performance penalty.) Or maybe it will make sense to initiate a parallel branch,
as Erik previously did for new snapshots -- I'm not sure which approach is
better from production point of view.

[Søren Thygesen Gjesse, 2010-03-23 08:25:11]

Some comments below, but it LGTM.

I will prefer a preprocessor directive for switching between implementations,
and only go for a branch if the amoint of #ifdef/#endif's we have to deal with
during the transition explodes.

http://codereview.chromium.org/1105009/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1/2#newcode80
include/v8-profiler.h:80: * Returns the number, 1-based, of the line where the
function originates.
We are not consistent here, as Message::GetLineNumber() returns 1-based line
numbers whereas Function::GetScriptLineNumber() and the debugger returns zero
based line numbers. I have bees advocating for zero based as that is what we use
internally, but I am not so sure anymore. However we should have a constant for
information not available as Function::kLineOffsetNotFound. Maybe using a shared
constant for this will be the best, and -1 will fit in both schemes.

http://codereview.chromium.org/1105009/diff/1/2#newcode88
include/v8-profiler.h:88: */
Should we consider using uint64_t and change to a higher resolution than
milliseconds?

http://codereview.chromium.org/1105009/diff/1/2#newcode118
include/v8-profiler.h:118: /** Returns CPU profile UID (assigned by the
profiler.) */
UID -> ID?

http://codereview.chromium.org/1105009/diff/1/2#newcode133
include/v8-profiler.h:133: * Interface for controlling CPU profiling in V8.
On need for "in V8".

[Mads Ager (chromium), 2010-03-23 08:38:26]

The interface looks good to me.

http://codereview.chromium.org/1105009/diff/1002/3001
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1002/3001#newcode112
include/v8-profiler.h:112: * CpuProfile contains a CPU profile in a form of 2
call trees:
in a form -> in form
2 -> two?

http://codereview.chromium.org/1105009/diff/1002/3001#newcode152
include/v8-profiler.h:152: * profiles with the same title are silently ignored.
*/
Move '*/' to next line.

[mnaganov (inactive), 2010-03-23 09:35:37]

Thanks!

Feel free to provide any additional comments, if you will have them later.

http://codereview.chromium.org/1105009/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1/2#newcode80
include/v8-profiler.h:80: * Returns the number, 1-based, of the line where the
function originates.
On 2010/03/23 08:25:11, Søren Gjesse wrote:
> We are not consistent here, as Message::GetLineNumber() returns 1-based line
> numbers whereas Function::GetScriptLineNumber() and the debugger returns zero
> based line numbers. I have bees advocating for zero based as that is what we
use
> internally, but I am not so sure anymore. However we should have a constant
for
> information not available as Function::kLineOffsetNotFound. Maybe using a
shared
> constant for this will be the best, and -1 will fit in both schemes.

I think that as this info is for people, and editor programs they use start line
numeration with 1, it's preferred to return 1-based numbers, otherwise somebody
next in the chain will anyway need to add 1 to the number returned.

Using a constant for "no info" is certainly a good way.

http://codereview.chromium.org/1105009/diff/1/2#newcode88
include/v8-profiler.h:88: */
On 2010/03/23 08:25:11, Søren Gjesse wrote:
> Should we consider using uint64_t and change to a higher resolution than
> milliseconds?

I propose doubles for easier interconnection with JavaScript, where Numbers are
doubles.

I think there is no need to try to achieve a better resolution, than
milliseconds with a software-only (that is, not using CPU h/w counters)
profiler.

[zundel, 2010-03-26 12:53:33]

http://codereview.chromium.org/1105009/diff/1002/3001
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1002/3001#newcode152
include/v8-profiler.h:152: * profiles with the same title are silently ignored.
*/
Does this comment mean that you can start different profiles and have them
accumulating simultaneously?  

start profile1 at time T1
start profile2 at time T2
stop profile2 at time T3
stop profile1 at time T4

so that profile2 is profiling a subset of profile1?

[mnaganov (inactive), 2010-03-26 12:57:50]

On 2010/03/26 12:53:33, zundel wrote:
> http://codereview.chromium.org/1105009/diff/1002/3001
> File include/v8-profiler.h (right):
> 
> http://codereview.chromium.org/1105009/diff/1002/3001#newcode152
> include/v8-profiler.h:152: * profiles with the same title are silently
ignored.
> */
> Does this comment mean that you can start different profiles and have them
> accumulating simultaneously?  
> 
> start profile1 at time T1
> start profile2 at time T2
> stop profile2 at time T3
> stop profile1 at time T4
> 
> so that profile2 is profiling a subset of profile1?

Yes. You can even do this:

start profile1 at time T1
start profile2 at time T2
stop profile1 at time T3
stop profile2 at time T4

so profile1 and profile2 will overlap

[zundel, 2010-03-26 13:49:15]

http://codereview.chromium.org/1105009/diff/1002/3001
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1002/3001#newcode129
include/v8-profiler.h:129: };
One thing I love about the v8 profiling data is that it classifies time by VM
state (compiling, garbage collection, etc).  Do you anticipate a way of exposing
that information?

http://codereview.chromium.org/1105009/diff/1002/3001#newcode152
include/v8-profiler.h:152: * profiles with the same title are silently ignored.
*/
Should this function return a uid for use in GetProfile(uid) ?

http://codereview.chromium.org/1105009/diff/1002/3001#newcode153
include/v8-profiler.h:153: static void StartProfiling(Handle<String> title =
Handle<String>());
for starting/stopping profiles automatically, I don't care so much about the
names (they aren't going to be ready by a human).  Could we come up with a way
for a unique name be assigned automatically?  Same with stopping the profile.  I
would want some kind of handle (uid?) returned from StartProfile to use to turn
this one off.

[zundel, 2010-03-26 13:57:33]

http://codereview.chromium.org/1105009/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/1105009/diff/1/2#newcode88
include/v8-profiler.h:88: */
On 2010/03/23 09:35:37, Michail Naganov wrote:
> On 2010/03/23 08:25:11, Søren Gjesse wrote:
> > Should we consider using uint64_t and change to a higher resolution than
> > milliseconds?
> 
> I propose doubles for easier interconnection with JavaScript, where Numbers
are
> doubles.
> 
> I think there is no need to try to achieve a better resolution, than
> milliseconds with a software-only (that is, not using CPU h/w counters)
> profiler.

I concur on the double change, plus it would allow you to specify
sub-millisecond if you needed to.

I've been playing with profiling activity over short intervals.  I did an
experimental change to the profiler on Mac which uses usleep() with lower
granularity profiles and it was somewhat useful (if not entirely accurate).  But
the best profile timer (linux itimer TIMER_PROFILE) only gave about 2-4ms
granularity when set to 1ms anyway.

[mnaganov (inactive), 2010-03-26 15:00:52]

On 2010/03/26 13:49:15, zundel wrote:
> http://codereview.chromium.org/1105009/diff/1002/3001
> File include/v8-profiler.h (right):
> 
> http://codereview.chromium.org/1105009/diff/1002/3001#newcode129
> include/v8-profiler.h:129: };
> One thing I love about the v8 profiling data is that it classifies time by VM
> state (compiling, garbage collection, etc).  Do you anticipate a way of
exposing
> that information?
> 

I can easily add overall statistics of ticks count per VM state.

> http://codereview.chromium.org/1105009/diff/1002/3001#newcode152
> include/v8-profiler.h:152: * profiles with the same title are silently
ignored.
> */
> Should this function return a uid for use in GetProfile(uid) ?
> 

This is the scenario of start / stop usage:

StartProfiling("name");
profile = StopProfiling("name");
now 'profile' is available for UI.

This is the scenario of GetProfile usage:

Get list of profile entries using GetProfilesCount / GetProfile(index). Now you
have uids, so when an individual profile is about to be opened on ui, it can be
retrieved using GetProfile(uid) (ugh, I actually renamed it into FindProfile to
avoid confusion.)

> http://codereview.chromium.org/1105009/diff/1002/3001#newcode153
> include/v8-profiler.h:153: static void StartProfiling(Handle<String> title =
> Handle<String>());
> for starting/stopping profiles automatically, I don't care so much about the
> names (they aren't going to be ready by a human).  Could we come up with a way
> for a unique name be assigned automatically?  Same with stopping the profile. 
I
> would want some kind of handle (uid?) returned from StartProfile to use to
turn
> this one off.

Well, in WebKit for some reason they use string names which they generate
automatically, when user presses "Start / Stop profiling" button. And string
names are used for profiles started via 'console' object. Using only numeric
uids will require managing (name, uid) pairs externally. Adding an overloaded
version that accepts numeric uids instead of names looks like cluttering an
interface. So, the current approach seems sufficient.

[mnaganov (inactive), 2010-03-26 15:02:07]

On 2010/03/26 13:57:33, zundel wrote:
> http://codereview.chromium.org/1105009/diff/1/2
> File include/v8-profiler.h (right):
> 
> http://codereview.chromium.org/1105009/diff/1/2#newcode88
> include/v8-profiler.h:88: */
> On 2010/03/23 09:35:37, Michail Naganov wrote:
> > On 2010/03/23 08:25:11, Søren Gjesse wrote:
> > > Should we consider using uint64_t and change to a higher resolution than
> > > milliseconds?
> > 
> > I propose doubles for easier interconnection with JavaScript, where Numbers
> are
> > doubles.
> > 
> > I think there is no need to try to achieve a better resolution, than
> > milliseconds with a software-only (that is, not using CPU h/w counters)
> > profiler.
> 
> I concur on the double change, plus it would allow you to specify
> sub-millisecond if you needed to.
> 
> I've been playing with profiling activity over short intervals.  I did an
> experimental change to the profiler on Mac which uses usleep() with lower
> granularity profiles and it was somewhat useful (if not entirely accurate). 
But
> the best profile timer (linux itimer TIMER_PROFILE) only gave about 2-4ms
> granularity when set to 1ms anyway.

Yes. Higher sampling frequencies are possible on Mac and Windows, but not on
Linux.