Added command line switches and UI (controlled via a build option)

Added command line switches and UI (controlled via a build option)
to make it easier to use the sampling profiler.

BUG=None
TEST=None

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

[jamesr, 2011-02-01 01:21:45]

Looks pretty good to me.  I think someone else should look at the startup path
to make sure this is hooking at the right level (I added Evan to check this
since I think he's been looking at startup, feel free to punt to someone else if
you think they are better qualified).

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.cc
File base/debug/profiler.cc (right):

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.cc#newco...
base/debug/profiler.cc:14: #endif
does this code work at all on windows? if not we should probably make sure it's
guarded by the ifdef as well

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h
File base/debug/profiler.h (right):

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h#newcode25
base/debug/profiler.h:25: void StartProfiling(const std::string name);
nit: should this be by reference?

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:16: const std::string
default_profile_name("chrome-profile-{pid}");
nit: this should be static and some sort of kThingy

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:37: const int kProfilingFlushSeconds = 10;
nit: static

[Evan Martin, 2011-02-01 02:45:59]

Awesome that you're doing this!

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h
File base/debug/profiler.h (right):

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h#newcode7
base/debug/profiler.h:7: // and if you would like to yield (breakpoint) into the
debugger.
This comment is wrong, no?

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode28
build/common.gypi:28: 'profiling%': 0,
Gyp is super-confusing, but you can either ask me about it or trust me that you
can eliminate this line...

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode33
build/common.gypi:33: 'profiling%': '<(profiling)',
And this one...

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode68
build/common.gypi:68: 'profiling%': '<(profiling)',
and this one...

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode174
build/common.gypi:174: 'profiling%': '<(profiling)',
And then move this one a bit lower, where you'll see there are some introduced
there that are commented.  So move this line down to there and move the comment
from above to join it.

http://codereview.chromium.org/6250070/diff/2001/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/6250070/diff/2001/chrome/app/chrome_main.cc#ne...
chrome/app/chrome_main.cc:639: Profiling::StartAtStart();
I've been trying to move code out of this monster function.
Are you trying to handle profiling right at startup?  Do you want this to happen
for all Chrome processes?

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:35: if (Profiling::BeingProfiled()) {
Can do an early-exit here to reduce indentation

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:37: const int kProfilingFlushSeconds = 10;
On 2011/02/01 01:21:45, jamesr wrote:
> nit: static

Just my opinion, but I don't think that's required here.

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:39: if (!flush_seconds) {
Is it possible for this to race between multiple threads?  This feels like it
should maybe be state for the current object.  Or it should just be grabbed in
Profiling::Start and stashed into a file-static int.

[DaveMoore, 2011-02-01 19:21:54]

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.cc
File base/debug/profiler.cc (right):

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.cc#newco...
base/debug/profiler.cc:14: #endif
I didn't see anything that specifically said whether this is supported on
Windows or not. I figured it wouldn't hurt to have it there and we could see.
On 2011/02/01 01:21:45, jamesr wrote:
> does this code work at all on windows? if not we should probably make sure
it's
> guarded by the ifdef as well

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h
File base/debug/profiler.h (right):

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h#newcode7
base/debug/profiler.h:7: // and if you would like to yield (breakpoint) into the
debugger.
On 2011/02/01 02:45:59, Evan Martin wrote:
> This comment is wrong, no?

Done.

http://codereview.chromium.org/6250070/diff/2001/base/debug/profiler.h#newcode25
base/debug/profiler.h:25: void StartProfiling(const std::string name);
On 2011/02/01 01:21:45, jamesr wrote:
> nit: should this be by reference?

Done.

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode28
build/common.gypi:28: 'profiling%': 0,
On 2011/02/01 02:45:59, Evan Martin wrote:
> Gyp is super-confusing, but you can either ask me about it or trust me that
you
> can eliminate this line...

Done.

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode33
build/common.gypi:33: 'profiling%': '<(profiling)',
On 2011/02/01 02:45:59, Evan Martin wrote:
> And this one...

Done.

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode68
build/common.gypi:68: 'profiling%': '<(profiling)',
On 2011/02/01 02:45:59, Evan Martin wrote:
> and this one...

Done.

http://codereview.chromium.org/6250070/diff/2001/build/common.gypi#newcode174
build/common.gypi:174: 'profiling%': '<(profiling)',
On 2011/02/01 02:45:59, Evan Martin wrote:
> And then move this one a bit lower, where you'll see there are some introduced
> there that are commented.  So move this line down to there and move the
comment
> from above to join it.

Done.

http://codereview.chromium.org/6250070/diff/2001/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/6250070/diff/2001/chrome/app/chrome_main.cc#ne...
chrome/app/chrome_main.cc:639: Profiling::StartAtStart();
I want to be able to profile the startup code itself so it has to come really
early. There are some issues with profiling other processes but the renderer
sometimes works, as does the gpu. This gets triggered by the command line
option.
On 2011/02/01 02:45:59, Evan Martin wrote:
> I've been trying to move code out of this monster function.
> Are you trying to handle profiling right at startup?  Do you want this to
happen
> for all Chrome processes?

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:16: const std::string
default_profile_name("chrome-profile-{pid}");
On 2011/02/01 01:21:45, jamesr wrote:
> nit: this should be static and some sort of kThingy

Done.

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:35: if (Profiling::BeingProfiled()) {
On 2011/02/01 02:45:59, Evan Martin wrote:
> Can do an early-exit here to reduce indentation

Done.

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:37: const int kProfilingFlushSeconds = 10;
On 2011/02/01 01:21:45, jamesr wrote:
> nit: static

Done.

http://codereview.chromium.org/6250070/diff/2001/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:39: if (!flush_seconds) {
This should only be called from the main thread. I don't know how safe the
underlying stuff is (obviously there is safety in the lower level functions but
I'm not sure about the top level entry points).
I've added a comment to the .h file.
On 2011/02/01 02:45:59, Evan Martin wrote:
> Is it possible for this to race between multiple threads?  This feels like it
> should maybe be state for the current object.  Or it should just be grabbed in
> Profiling::Start and stashed into a file-static int.

[evanm, 2011-02-01 19:39:59]

I'm still not quite clear on the startup parts.

http://www.chromium.org/developers/design-documents/startup describes a bit more
background.  I think from your description you want the StartAtStart call in
ChromeMain and then not anywhere else -- I think the RendererMain one is
redundant (?)

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc
File base/debug/profiler.cc (right):

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc#newco...
base/debug/profiler.cc:29: ReplaceSubstringsAfterOffset(&full_name, 0,
"{count}", count);
The last argument here is a string, right?  So the .c_str() above isn't
necesssary (?)

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc#newco...
base/debug/profiler.cc:39: ProfilerFlush();
2spc

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi#newcode25
build/common.gypi:25: 'touchui%': 0,
probably didn't mean to make this change

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi#newcode282
build/common.gypi:282: # Enable sampling based profiler.
Could you reference where to find more info on this?  Either reference a wiki
page here, or reference a wiki page in the header and point at the header?

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:41: static int flush_seconds = 0;
If this really can only be called from one thread (which is what the static
implies), you can just use MessageLoop::current() rather than taking in a loop
as an argument.

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h
File chrome/common/profiling.h (right):

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h#ne...
chrome/common/profiling.h:17: // All of the API should only be called from the
main thread of the process.cdddfd
typos in here

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h#ne...
chrome/common/profiling.h:20: // Called early in a process' life to allow
profiling of the startup process.
Maybe mention that this examines command-line flags to judge whether to run. 
"StartAtStart" isn't very descriptive and the comment doesn't fill in what it
does -- a name like "StartAtStartupIfNecessary" or something would help (that
name isn't so good, but hopefully you get my idea)

[DaveMoore, 2011-02-01 21:33:03]

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc
File base/debug/profiler.cc (right):

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc#newco...
base/debug/profiler.cc:29: ReplaceSubstringsAfterOffset(&full_name, 0,
"{count}", count);
ProfilerStart in tcmalloc takes a const char*
On 2011/02/01 19:39:59, evanm wrote:
> The last argument here is a string, right?  So the .c_str() above isn't
> necesssary (?)

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc#newco...
base/debug/profiler.cc:39: ProfilerFlush();
On 2011/02/01 19:39:59, evanm wrote:
> 2spc 

Done.

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi#newcode25
build/common.gypi:25: 'touchui%': 0,
On 2011/02/01 19:39:59, evanm wrote:
> probably didn't mean to make this change

Done.

http://codereview.chromium.org/6250070/diff/5002/build/common.gypi#newcode282
build/common.gypi:282: # Enable sampling based profiler.
On 2011/02/01 19:39:59, evanm wrote:
> Could you reference where to find more info on this?  Either reference a wiki
> page here, or reference a wiki page in the header and point at the header?

Done.

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.cc#n...
chrome/common/profiling.cc:41: static int flush_seconds = 0;
On 2011/02/01 19:39:59, evanm wrote:
> If this really can only be called from one thread (which is what the static
> implies), you can just use MessageLoop::current() rather than taking in a loop
> as an argument.

Done.

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h
File chrome/common/profiling.h (right):

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h#ne...
chrome/common/profiling.h:17: // All of the API should only be called from the
main thread of the process.cdddfd
On 2011/02/01 19:39:59, evanm wrote:
> typos in here

Done.

http://codereview.chromium.org/6250070/diff/5002/chrome/common/profiling.h#ne...
chrome/common/profiling.h:20: // Called early in a process' life to allow
profiling of the startup process.
Changed method name to ProcessStarted() to match MainMessageLoopStarted().
On 2011/02/01 19:39:59, evanm wrote:
> Maybe mention that this examines command-line flags to judge whether to run. 
> "StartAtStart" isn't very descriptive and the comment doesn't fill in what it
> does -- a name like "StartAtStartupIfNecessary" or something would help (that
> name isn't so good, but hopefully you get my idea)

Done.

[evanm, 2011-02-01 21:41:24]

LGTM

I wonder if Profiler belongs in the base namespace, though...

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc
File base/debug/profiler.cc (right):

http://codereview.chromium.org/6250070/diff/5002/base/debug/profiler.cc#newco...
base/debug/profiler.cc:29: ReplaceSubstringsAfterOffset(&full_name, 0,
"{count}", count);
On 2011/02/01 21:33:04, davemoore wrote:
> ProfilerStart in tcmalloc takes a const char*

No, above this line.  You have two calls in a row to the same function with
different argument types

[brettw, 2011-02-01 22:32:59]

Base additions LGTM (I didn't check the rest).

[willchan no longer on Chromium, 2011-10-03 19:40:03]

I just looked at this code. We're scheduling file writes on the UI thread. When
profiling gets enabled, can we use a different thread?