code review 4313041: runtime/pprof: cpu profiling support

runtime/pprof: cpu profiling support

[rsc, 2011-03-22 20:46:40]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2011-03-22 20:47:59]

This is the Go interface to the previous CL.

Russ

[r, 2011-03-22 21:15:55]

LGTM

yay

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.go
File src/pkg/runtime/pprof/pprof.go (right):

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
src/pkg/runtime/pprof/pprof.go:17: "sync"
is this a safe import to add?  try a make clean and build

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
src/pkg/runtime/pprof/pprof.go:127: // convert sample count to seconds.  Instead
of requiring
s/count/&s/

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
src/pkg/runtime/pprof/pprof.go:129: const hz = 100
this isn't very forward-looking. maybe ok.

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
src/pkg/runtime/pprof/pprof.go:158: // to the writer passed to StartCPUProfile.
s/.$/, at least until another call to StartCPUProfile with  that writer./
(you've made it sound like the writer is poisoned)

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
File src/pkg/runtime/pprof/pprof_test.go (right):

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
src/pkg/runtime/pprof/pprof_test.go:44: t.Fatalf("unexpected header %#x",
val[:5])
i'm a fan of "[% x]" myself

[bradfitz, 2011-03-22 21:26:01]

Cool.

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
File src/pkg/runtime/pprof/pprof_test.go (right):

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
src/pkg/runtime/pprof/pprof_test.go:19: return
maybe t.Logf("skipping on %s", runtime.GOOS) ?

http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
src/pkg/runtime/pprof/pprof_test.go:33: StopCPUProfile()
select on this and a time.After() in a few seconds to catch the case of
StopCPUProfile hanging forever?

[rsc, 2011-03-23 17:41:32]

>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
> src/pkg/runtime/pprof/pprof.go:17: "sync"
> is this a safe import to add?  try a make clean and build

Yes; sync is already depended on by testing.

>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
> src/pkg/runtime/pprof/pprof.go:129: const hz = 100
> this isn't very forward-looking. maybe ok.

I spent a while trying different rates and trying to
figure out a good interface.  The range of useful
rates seems to be something that the profiling package
probably knows more about than the end user,
so I chose not to force every call site to pick its
own rate.  If we need to adjust it later, it will be
in just one place.  (Also as the comment says,
on Linux at least you can't go much beyond 100 Hz.)

>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
> src/pkg/runtime/pprof/pprof_test.go:44: t.Fatalf("unexpected header
> %#x", val[:5])
> i'm a fan of "[% x]" myself

This is []uintptr, not []byte.  But agreed.

Russ

[r2, 2011-03-23 17:48:02]

On Mar 23, 2011, at 10:41 AM, Russ Cox wrote:

>>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
>> src/pkg/runtime/pprof/pprof.go:17: "sync"
>> is this a safe import to add?  try a make clean and build
> 
> Yes; sync is already depended on by testing.
> 
>>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof.g...
>> src/pkg/runtime/pprof/pprof.go:129: const hz = 100
>> this isn't very forward-looking. maybe ok.
> 
> I spent a while trying different rates and trying to
> figure out a good interface.  The range of useful
> rates seems to be something that the profiling package
> probably knows more about than the end user,
> so I chose not to force every call site to pick its
> own rate.  If we need to adjust it later, it will be
> in just one place.  (Also as the comment says,
> on Linux at least you can't go much beyond 100 Hz.)
> 
>>
http://codereview.appspot.com/4313041/diff/3001/src/pkg/runtime/pprof/pprof_t...
>> src/pkg/runtime/pprof/pprof_test.go:44: t.Fatalf("unexpected header
>> %#x", val[:5])
>> i'm a fan of "[% x]" myself
> 
> This is []uintptr, not []byte.  But agreed.

Hmmm.  Are you suggesting fmt do %[dox] for all integer slices?

-rob

[rsc, 2011-03-23 17:53:32]

> Hmmm.  Are you suggesting fmt do %[dox] for all integer slices?

It already does.
What doesn't work is "% x" because there is nothing to space.
Not a bug.

Russ

[rsc, 2011-03-23 17:54:33]

*** Submitted as 37171a06ed48 ***

runtime/pprof: cpu profiling support

R=r, bradfitzgo, r2
CC=golang-dev
http://codereview.appspot.com/4313041