code review 719041: runtime: malloc sampling, pprof interface

runtime: malloc sampling, pprof interface

[rsc, 2010-03-24 07:27:03]

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

I'd like you to review this change.

[r, 2010-03-24 14:32:06]

LGTM

http://codereview.appspot.com/719041/diff/2001/3003
File src/pkg/http/pprof/pprof.go (right):

http://codereview.appspot.com/719041/diff/2001/3003#newcode61
src/pkg/http/pprof/pprof.go:61: fmt.Fprintf(c, "num_symbols: 1\n")
1?

http://codereview.appspot.com/719041/diff/2001/3006
File src/pkg/runtime/mprof.cgo (right):

http://codereview.appspot.com/719041/diff/2001/3006#newcode203
src/pkg/runtime/mprof.cgo:203: m->nomemprof--;
can you invert the sense of the variable? it's confusing.

http://codereview.appspot.com/719041/diff/2001/3008
File src/pkg/runtime/pprof/pprof.go (right):

http://codereview.appspot.com/719041/diff/2001/3008#newcode25
src/pkg/runtime/pprof/pprof.go:25: for {
i think you can make this cleaner by putting the make() outside
p = make
var n, ok
for n, ok = ...; !ok {
}

if MemProfile returned a zero instead of an ok, you could make it shorter. not
sure you want that.

http://codereview.appspot.com/719041/diff/2001/3008#newcode40
src/pkg/runtime/pprof/pprof.go:40: r := &p[i]
this is for speed? does it make a difference?
why not just for _, r := range p {

http://codereview.appspot.com/719041/diff/2001/3008#newcode54
src/pkg/runtime/pprof/pprof.go:54: r := &p[i]
ditto

http://codereview.appspot.com/719041/diff/2001/3009
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/719041/diff/2001/3009#newcode103
src/pkg/runtime/proc.c:103: 
tab

[rsc, 2010-03-24 16:35:22]

http://codereview.appspot.com/719041/diff/2001/3003
File src/pkg/http/pprof/pprof.go (right):

http://codereview.appspot.com/719041/diff/2001/3003#newcode61
src/pkg/http/pprof/pprof.go:61: fmt.Fprintf(c, "num_symbols: 1\n")
Added a comment

	// We don't know how many symbols we have, but we
	// do have symbol information.  Pprof only cares whether
	// this number is 0 (no symbols available) or > 0.
	fmt.Fprintf(c, "num_symbols: 1\n")

http://codereview.appspot.com/719041/diff/2001/3006
File src/pkg/runtime/mprof.cgo (right):

http://codereview.appspot.com/719041/diff/2001/3006#newcode203
src/pkg/runtime/mprof.cgo:203: m->nomemprof--;
On 2010/03/24 14:32:07, r wrote:
> can you invert the sense of the variable? it's confusing.

It is, but it stacks this way, so that multiple things on
a call stack can do ++ and -- and not step on each other.

http://codereview.appspot.com/719041/diff/2001/3008
File src/pkg/runtime/pprof/pprof.go (right):

http://codereview.appspot.com/719041/diff/2001/3008#newcode25
src/pkg/runtime/pprof/pprof.go:25: for {
On 2010/03/24 14:32:07, r wrote:
> i think you can make this cleaner by putting the make() outside
> p = make
> var n, ok
> for n, ok = ...; !ok {
> }
> 
> if MemProfile returned a zero instead of an ok, you could make it shorter. not
> sure you want that.

Added a comment atop the loop.

	// Find out how many records there are (MemProfile(nil, false)),
	// allocate that many records, and get the data.
	// There's a race—more records might be added between
	// the two calls—so allocate a few extra records for safety
	// and also try again if we're very unlucky.
	// The loop should only execute one iteration in the common case.
	var p []runtime.MemProfileRecord
	n, ok := runtime.MemProfile(nil, false)
	for {
		// Allocate room for a slightly bigger profile,
		// in case a few more entries have been added
		// since the call to MemProfile.
		p = make([]runtime.MemProfileRecord, n+50)
		n, ok = runtime.MemProfile(p, false)
		if ok {
			p = p[0:n]
			break
		}
		// Profile grew; try again.
	}

http://codereview.appspot.com/719041/diff/2001/3008#newcode40
src/pkg/runtime/pprof/pprof.go:40: r := &p[i]
On 2010/03/24 14:32:07, r wrote:
> this is for speed? does it make a difference?
> why not just for _, r := range p {

I don't know whether it makes a difference but
it does avoid a 300-byte copy on every iteration.

[rsc, 2010-03-24 16:40:14]

*** Submitted as http://code.google.com/p/go/source/detail?r=9e1fd312d1bd ***

runtime: malloc sampling, pprof interface

R=r
CC=golang-dev
http://codereview.appspot.com/719041