code review 153750043: cmd/pprof: add Go implementation

cmd/pprof: add Go implementation

Update issue 8798

This is a new implementation of pprof,
written in Go instead of in Perl.
It was written primarily by Raul Silvera and
is in use for profiling programs of all languages
inside Google.

The internal structure is a bit package-heavy,
but it matches the copy used inside Google, and
since it is in an internal directory, we can make
changes to it later if we need to.

The only "new" file here is src/cmd/pprof/pprof.go,
which stitches together the Google pprof and the
Go command libraries for object file access.

I am explicitly NOT interested in style or review
comments on the rest of the files
(that is, src/cmd/pprof/internal/...).
Those are intended to stay as close to the Google
copies as possible, like we did with the pprof Perl script.

Still to do:

- Basic tests.
- Real command documentation.
- Hook up disassemblers.

[rsc, 2014-09-29 16:18:32]

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

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-09-29 16:28:31]

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:5: package main
no package comment

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:25: if err := driver.PProf(flags{}, fetch.Fetcher,
symbolize, new(objTool), plugin.StandardUI(), nil); err != nil {
pull out nil into a named variable so readers can see what it means here at the
call site?  (refraining from commenting on this func's signature)

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:103: func (t *objTool) Open(name string, start uint64)
(plugin.ObjFile, error) {
why pointer and why a named receiver? and same for all the ones below.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:195: f.file.Close()
not returning this error? comment about why it's being ignored?

[r, 2014-09-29 17:35:41]

the author should be in CONTRIBUTORS

[rsc, 2014-09-29 18:02:40]

On Mon, Sep 29, 2014 at 1:35 PM, <r@golang.org> wrote:

> the author should be in CONTRIBUTORS
>

prepared CL 146450043 but forgot to mail

[brainman, 2014-09-29 23:19:38]

This even runs on windows! Thank you Russ.

Alex

[r, 2014-09-29 23:44:28]

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:30: // symbolize attempts to symbolize profile p. First
uses binutils on
s/First/& it/

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:69: func (flags) Bool(o string, d bool, c string) *bool {
c string? not s string?

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:129: type file struct {
// file represents...

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:136: func (f *file) Name() string {
// Name returns...

but why does it exist? must be about some interface you didn't mention above

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:140: func (f *file) Base() uint64 {
// Base etc. etc.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:195: f.file.Close()
On 2014/09/29 16:28:31, bradfitz wrote:
> not returning this error? comment about why it's being ignored?

if i can't close a file, what am i supposed to do?

[dave_cheney.net, 2014-09-30 00:21:39]

Does this code need to be in the main repository ? Could it be moved
to go.tools ?

On Tue, Sep 30, 2014 at 9:44 AM,  <r@golang.org> wrote:
>
> https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
> File src/cmd/pprof/pprof.go (right):
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:30: // symbolize attempts to symbolize profile p.
> First uses binutils on
> s/First/& it/
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:69: func (flags) Bool(o string, d bool, c string)
> *bool {
> c string? not s string?
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:129: type file struct {
> // file represents...
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:136: func (f *file) Name() string {
> // Name returns...
>
> but why does it exist? must be about some interface you didn't mention
> above
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:140: func (f *file) Base() uint64 {
> // Base etc. etc.
>
>
https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
> src/cmd/pprof/pprof.go:195: f.file.Close()
> On 2014/09/29 16:28:31, bradfitz wrote:
>>
>> not returning this error? comment about why it's being ignored?
>
>
> if i can't close a file, what am i supposed to do?
>
>
> https://codereview.appspot.com/153750043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[rsc, 2014-09-30 00:52:34]

On Mon, Sep 29, 2014 at 8:21 PM, Dave Cheney <dave@cheney.net> wrote:

> Does this code need to be in the main repository ? Could it be moved
> to go.tools ?
>

Yes, it does. It uses cmd/internal.

Also, things built in the distribution should be in the main repo by
default. It complicates things quite a bit that there are distributed
binaries that are in go.tools instead of in the main tree. I would like to
see fewer, not more.

Russ

[dave_cheney.net, 2014-09-30 00:54:21]

Fair enough. Sorry for the noise.

On Tue, Sep 30, 2014 at 10:52 AM, Russ Cox <rsc@golang.org> wrote:
> On Mon, Sep 29, 2014 at 8:21 PM, Dave Cheney <dave@cheney.net> wrote:
>>
>> Does this code need to be in the main repository ? Could it be moved
>> to go.tools ?
>
>
> Yes, it does. It uses cmd/internal.
>
> Also, things built in the distribution should be in the main repo by
> default. It complicates things quite a bit that there are distributed
> binaries that are in go.tools instead of in the main tree. I would like to
> see fewer, not more.
>
> Russ

[rsc, 2014-09-30 16:43:24]

PTAL

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:5: package main
On 2014/09/29 16:28:31, bradfitz wrote:
> no package comment

it is a TODO in CL description. will do in a separate CL.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:25: if err := driver.PProf(flags{}, fetch.Fetcher,
symbolize, new(objTool), plugin.StandardUI(), nil); err != nil {
On 2014/09/29 16:28:31, bradfitz wrote:
> pull out nil into a named variable so readers can see what it means here at
the
> call site?  (refraining from commenting on this func's signature)

I don't understand. It sounds like you are asking for

var noError error = nil
if err := driver.PProf(flags{}, fetch.Fetcher, symbolize, new(objTool),
plugin.StandardUI(), nil); err != noError {

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:30: // symbolize attempts to symbolize profile p. First
uses binutils on
On 2014/09/29 23:44:28, r wrote:
> s/First/& it/

Rewrote comment. (The first 100 lines of this file are copied from the internal
pprof too.)

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:69: func (flags) Bool(o string, d bool, c string) *bool {
On 2014/09/29 23:44:28, r wrote:
> c string? not s string?

I don't know. I copied this from the internal pprof so I'd rather not change it.
I guess it is o = option-name, d = default, c = usage.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:99: // objTool implements plugin.ObjTool using Go
libraries
FWIW, this is the place in the file where the code I wrote begins.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:103: func (t *objTool) Open(name string, start uint64)
(plugin.ObjFile, error) {
On 2014/09/29 16:28:31, bradfitz wrote:
> why pointer and why a named receiver? and same for all the ones below.

It seems plausible that there will be state later (set by SetConfig). I'll leave
the pointer but drop the receiver name.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:129: type file struct {
On 2014/09/29 23:44:28, r wrote:
> // file represents...

// file implements plugin.ObjFile using Go libraries
// (instead of invoking GNU binutils).
// A file represents a single executable being analyzed.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:136: func (f *file) Name() string {
On 2014/09/29 23:44:28, r wrote:
> // Name returns...
> 
> but why does it exist? must be about some interface you didn't mention above

I mention the interface above now, so I'm going to leave the methods
uncommented.
The interface is in plugin.go.

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:195: f.file.Close()
On 2014/09/29 23:44:28, r wrote:
> On 2014/09/29 16:28:31, bradfitz wrote:
> > not returning this error? comment about why it's being ignored?
> 
> if i can't close a file, what am i supposed to do?

exactly. checking Close is only useful if you wrote to the file. nothing writes
to this file.

[bradfitz, 2014-09-30 16:47:46]

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:25: if err := driver.PProf(flags{}, fetch.Fetcher,
symbolize, new(objTool), plugin.StandardUI(), nil); err != nil {
On 2014/09/30 16:43:24, rsc wrote:
> On 2014/09/29 16:28:31, bradfitz wrote:
> > pull out nil into a named variable so readers can see what it means here at
> the
> > call site?  (refraining from commenting on this func's signature)
> 
> I don't understand. It sounds like you are asking for
> 
> var noError error = nil
> if err := driver.PProf(flags{}, fetch.Fetcher, symbolize, new(objTool),
> plugin.StandardUI(), nil); err != noError {
>

No, I meant "what is that nil after plugin.StandardUI() ?".

I'd prefer:

    var frobber *blah.Frobber // nil means default frobber

    ... driver.Pprof(....., frobber)

[rsc, 2014-09-30 17:29:22]

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/40001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:25: if err := driver.PProf(flags{}, fetch.Fetcher,
symbolize, new(objTool), plugin.StandardUI(), nil); err != nil {
got it.

	var extraCommands map[string]*commands.Command // no added Go-specific commands
	if err := driver.PProf(flags{}, fetch.Fetcher, symbolize, new(objTool),
plugin.StandardUI(), extraCommands); err != nil {
		fmt.Fprintf(os.Stderr, "%v\n", err)
	}

[r, 2014-09-30 17:29:48]

LGTM with behind-the-scenes grumbling.

https://codereview.appspot.com/153750043/diff/60001/src/cmd/pprof/pprof.go
File src/cmd/pprof/pprof.go (right):

https://codereview.appspot.com/153750043/diff/60001/src/cmd/pprof/pprof.go#ne...
src/cmd/pprof/pprof.go:45: ui.PrintErr("expecting
-symbolize=[local|remote|none][:force]")
i know you didn't write this and i know i'm not supposed to complain so i'll
stop this sentence here.

[rsc, 2014-09-30 17:42:00]

*** Submitted as https://code.google.com/p/go/source/detail?r=c1a3f7652520 ***

cmd/pprof: add Go implementation

Update issue 8798

This is a new implementation of pprof,
written in Go instead of in Perl.
It was written primarily by Raul Silvera and
is in use for profiling programs of all languages
inside Google.

The internal structure is a bit package-heavy,
but it matches the copy used inside Google, and
since it is in an internal directory, we can make
changes to it later if we need to.

The only "new" file here is src/cmd/pprof/pprof.go,
which stitches together the Google pprof and the
Go command libraries for object file access.

I am explicitly NOT interested in style or review
comments on the rest of the files
(that is, src/cmd/pprof/internal/...).
Those are intended to stay as close to the Google
copies as possible, like we did with the pprof Perl script.

Still to do:

- Basic tests.
- Real command documentation.
- Hook up disassemblers.

LGTM=r
R=r, bradfitz, alex.brainman, dave
CC=golang-codereviews
https://codereview.appspot.com/153750043

[gobot, 2014-09-30 17:51:45]

This CL appears to have broken the plan9-amd64-aram builder.
See http://build.golang.org/log/4f60dd6f747dbc30ae8053968760f88318dd9fff