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