Add initial version of DevTools Profiler

Add initial version of DevTools Profiler.

To activate it, run Chromium with the following 
additional js-flags: "--prof --noprof_auto --logfile=*"


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=16052

[pfeldman, 2009-05-13 15:53:44]

http://codereview.chromium.org/115299/diff/1017/37
File webkit/glue/devtools/debugger_agent.h (right):

http://codereview.chromium.org/115299/diff/1017/37#newcode19
Line 19: METHOD0(PauseProfiling) \
StartProfiling

http://codereview.chromium.org/115299/diff/1017/37#newcode22
Line 22: METHOD0(ResumeProfiling) \
StopProfiling

http://codereview.chromium.org/115299/diff/1017/37#newcode36
Line 36: METHOD2(ProfilerLog, std::string /* log */, int /* new_position */)
DidGetLogLines

http://codereview.chromium.org/115299/diff/1017/35
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/115299/diff/1017/35#newcode184
Line 184: #if USE(V8)
we kinda stopped putting these.

http://codereview.chromium.org/115299/diff/1017/35#newcode186
Line 186: v8::V8::PauseProfiler();
I would just make this call from debugger_agent_impl

http://codereview.chromium.org/115299/diff/1017/35#newcode194
Line 194: v8::V8::ResumeProfiler();
ditto

http://codereview.chromium.org/115299/diff/1017/35#newcode203
Line 203: static char buffer[65536];
ditto

http://codereview.chromium.org/115299/diff/1017/44
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/115299/diff/1017/44#newcode21
Line 21: goog.bind(this.handleHandleProfilerLog_, this);
Please use callback infrastructure for responses if possible (see dom_agent.js).

http://codereview.chromium.org/115299/diff/1017/43
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/115299/diff/1017/43#newcode719
Line 719: * @override
Could you put some more comments on these

[yurys, 2009-05-13 16:04:14]

http://codereview.chromium.org/115299/diff/1017/44
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/115299/diff/1017/44#newcode74
Line 74: this.isProcessingProfile_ = false;
this flag may become inconsistent with the agent state if you have more than one
profiler client

http://codereview.chromium.org/115299/diff/1017/44#newcode311
Line 311: * Starts (resumes) profiling.
I think you can create ProfilerAgent for the profiler stuff in the front-end

http://codereview.chromium.org/115299/diff/1017/44#newcode329
Line 329: debugPrint('processing profiler data...');
remove this line

http://codereview.chromium.org/115299/diff/1017/44#newcode571
Line 571: * @param {number} new_position
add comment on what the new position is

http://codereview.chromium.org/115299/diff/1017/44#newcode574
Line 574: log, new_position) {
style: newPosition

http://codereview.chromium.org/115299/diff/1017/44#newcode575
Line 575: debugPrint(new_position +
remove this line

[mnaganov (inactive), 2009-05-14 07:42:59]

http://codereview.chromium.org/115299/diff/1017/37
File webkit/glue/devtools/debugger_agent.h (right):

http://codereview.chromium.org/115299/diff/1017/37#newcode19
Line 19: METHOD0(PauseProfiling) \
On 2009/05/13 15:53:44, pfeldman wrote:
> StartProfiling

In fact, this is "StopProfiling". Done.

http://codereview.chromium.org/115299/diff/1017/37#newcode22
Line 22: METHOD0(ResumeProfiling) \
On 2009/05/13 15:53:44, pfeldman wrote:
> StopProfiling

In fact, this is "StartProfiling". Done.

http://codereview.chromium.org/115299/diff/1017/37#newcode36
Line 36: METHOD2(ProfilerLog, std::string /* log */, int /* new_position */)
On 2009/05/13 15:53:44, pfeldman wrote:
> DidGetLogLines

Done.

http://codereview.chromium.org/115299/diff/1017/35
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/115299/diff/1017/35#newcode184
Line 184: #if USE(V8)
On 2009/05/13 15:53:44, pfeldman wrote:
> we kinda stopped putting these.

Done.

http://codereview.chromium.org/115299/diff/1017/35#newcode186
Line 186: v8::V8::PauseProfiler();
On 2009/05/13 15:53:44, pfeldman wrote:
> I would just make this call from debugger_agent_impl

Calling them from AgentManager is more consistent with the existing code.

http://codereview.chromium.org/115299/diff/1017/35#newcode194
Line 194: v8::V8::ResumeProfiler();
On 2009/05/13 15:53:44, pfeldman wrote:
> ditto

#if removed.

http://codereview.chromium.org/115299/diff/1017/35#newcode203
Line 203: static char buffer[65536];
On 2009/05/13 15:53:44, pfeldman wrote:
> ditto

#if removed.

http://codereview.chromium.org/115299/diff/1017/44
File webkit/glue/devtools/js/debugger_agent.js (right):

http://codereview.chromium.org/115299/diff/1017/44#newcode21
Line 21: goog.bind(this.handleHandleProfilerLog_, this);
On 2009/05/13 15:53:44, pfeldman wrote:
> Please use callback infrastructure for responses if possible (see
dom_agent.js).

I considered this, but for now it looks like a complication---I don't need IDs
for callbacks.

http://codereview.chromium.org/115299/diff/1017/44#newcode74
Line 74: this.isProcessingProfile_ = false;
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> this flag may become inconsistent with the agent state if you have more than
one
> profiler client

Let's postpone thinking about multiple clients for now.

http://codereview.chromium.org/115299/diff/1017/44#newcode311
Line 311: * Starts (resumes) profiling.
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> I think you can create ProfilerAgent for the profiler stuff in the front-end

I can move it out, but DebuggerAgent will still have profiler-related functions
and will delegate to a contained ProfilerAgent. Doesn't look like a big win. My
plan is firstly to figure out how to cope with multiple agents, then we'll see
if we really need to keep profiler-related functions in Glue's DebuggerAgent,
and if not, creating a separate agent for profiling will be very natural.

http://codereview.chromium.org/115299/diff/1017/44#newcode329
Line 329: debugPrint('processing profiler data...');
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> remove this line

Done.

http://codereview.chromium.org/115299/diff/1017/44#newcode571
Line 571: * @param {number} new_position
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> add comment on what the new position is

Done.

http://codereview.chromium.org/115299/diff/1017/44#newcode574
Line 574: log, new_position) {
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> style: newPosition

Done.

http://codereview.chromium.org/115299/diff/1017/44#newcode575
Line 575: debugPrint(new_position +
On 2009/05/13 16:04:16, Yury Semikhatsky wrote:
> remove this line

Done.

http://codereview.chromium.org/115299/diff/1017/43
File webkit/glue/devtools/js/devtools.js (right):

http://codereview.chromium.org/115299/diff/1017/43#newcode719
Line 719: * @override
On 2009/05/13 15:53:44, pfeldman wrote:
> Could you put some more comments on these

Done.

[pfeldman, 2009-05-14 08:24:20]

LGTM

http://codereview.chromium.org/115299/diff/80/84
File webkit/glue/devtools/debugger_agent.h (right):

http://codereview.chromium.org/115299/diff/80/84#newcode20
Line 20: /* Stops profiling (samples collection). */ \
nit: be more positive in ordering things :)

http://codereview.chromium.org/115299/diff/80/82
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/115299/diff/80/82#newcode184
Line 184: DCHECK(attached_agents_->contains(debugger_agent));
no need for this DCHECK and maybe you pull this one line call up the call graph?

http://codereview.chromium.org/115299/diff/80/82#newcode190
Line 190: DCHECK(attached_agents_->contains(debugger_agent));
ditto

http://codereview.chromium.org/115299/diff/80/82#newcode201
Line 201: debugger_agent->DidGetLogLines(buffer, pos + read_size);
Does client know how much information there really is pending?

http://codereview.chromium.org/115299/diff/80/89
File webkit/glue/devtools/js/inspector_controller_impl.js (right):

http://codereview.chromium.org/115299/diff/80/89#newcode153
Line 153: WebInspector.setRecordingProfile(true);
could you put this into the agent instead?

http://codereview.chromium.org/115299/diff/80/89#newcode162
Line 162: devtools.tools.getDebuggerAgent().stopProfiling();
ditto

http://codereview.chromium.org/115299/diff/80/81
File webkit/webkit.gyp (right):

http://codereview.chromium.org/115299/diff/80/81#newcode4667
Line 4667: '../v8/tools/splaytree.js',
nit: sort alphabetically.

[mnaganov (inactive), 2009-05-14 08:46:24]

http://codereview.chromium.org/115299/diff/80/84
File webkit/glue/devtools/debugger_agent.h (right):

http://codereview.chromium.org/115299/diff/80/84#newcode20
Line 20: /* Stops profiling (samples collection). */ \
On 2009/05/14 08:24:20, pfeldman wrote:
> nit: be more positive in ordering things :)

Done.

http://codereview.chromium.org/115299/diff/80/82
File webkit/glue/devtools/debugger_agent_manager.cc (right):

http://codereview.chromium.org/115299/diff/80/82#newcode184
Line 184: DCHECK(attached_agents_->contains(debugger_agent));
On 2009/05/14 08:24:20, pfeldman wrote:
> no need for this DCHECK and maybe you pull this one line call up the call
graph?

Done.

http://codereview.chromium.org/115299/diff/80/82#newcode190
Line 190: DCHECK(attached_agents_->contains(debugger_agent));
On 2009/05/14 08:24:20, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/115299/diff/80/82#newcode201
Line 201: debugger_agent->DidGetLogLines(buffer, pos + read_size);
On 2009/05/14 08:24:20, pfeldman wrote:
> Does client know how much information there really is pending?

No. At least not now. It just retrieves log lines until there are any. As
currently the size of the log buffer is finite, the process is guaranteed to
finish.

http://codereview.chromium.org/115299/diff/80/89
File webkit/glue/devtools/js/inspector_controller_impl.js (right):

http://codereview.chromium.org/115299/diff/80/89#newcode153
Line 153: WebInspector.setRecordingProfile(true);
On 2009/05/14 08:24:20, pfeldman wrote:
> could you put this into the agent instead?

Done.

http://codereview.chromium.org/115299/diff/80/89#newcode162
Line 162: devtools.tools.getDebuggerAgent().stopProfiling();
On 2009/05/14 08:24:20, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/115299/diff/80/81
File webkit/webkit.gyp (right):

http://codereview.chromium.org/115299/diff/80/81#newcode4667
Line 4667: '../v8/tools/splaytree.js',
On 2009/05/14 08:24:20, pfeldman wrote:
> nit: sort alphabetically.

Done.