code review 37540043: runtime: heap dump experiment

runtime: WriteHeapDump dumps the heap to a file.

See http://golang.org/s/go13heapdump for the file format.

[rsc, 2014-03-07 01:21:44]

https://codereview.appspot.com/37540043/diff/160001/src/pkg/runtime/string.goc
File src/pkg/runtime/string.goc (right):

https://codereview.appspot.com/37540043/diff/160001/src/pkg/runtime/string.go...
src/pkg/runtime/string.goc:265: runtime·strlen(byte *s)
For reasons that predate my involvement (perhaps to draw a distinction between C
and Go strings), we already have this function but it is called findnull.

[khr, 2014-03-12 22:51:17]

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

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[khr, 2014-03-12 23:07:22]

On 2014/03/12 22:51:17, khr wrote:
> Hello mailto:rsc@golang.org (cc: mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://khr%40golang.org@code.google.com/p/go/

BTW, I've got some tools to examine the dumps checked in at
github.com/randall77/hprof.  You can go get them and try them out.

For example, the following program:

package main

import (
	"fmt"
	"os"
	"runtime/debug"
)

type Data struct {
	a, b string
}

func main() {
	var c [4]chan Data

	for i := 0; i < 4; i++ {
		c[i] = make(chan Data, 43)
		for j := 0; j < i+1; j++ {
			c[i] <- Data{fmt.Sprintf("XXXXXXXXXXXXXXXXXXXX%d_%d", i, j),
				fmt.Sprintf("YYYYYYYYYYYYYYYY%d!%d", i, j)}
		}
	}
	f, err := os.Create("dump")
	if err != nil {
		panic(err)
	}
	debug.DumpHeap(f.Fd())
	f.Close()
	<-c[2]
}

generates the following graph: http://keithandkatie.com/heapchan.png

$ go build heapchan.go
$ ./heapchan
$ go build dumptodot.go readdump.go
$ ./dumptodot dump heapchan > dump.dot
$ dot -Tpng dump.dot > dump.png

[bradfitz, 2014-03-12 23:08:40]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:150: // Dump description of the heap and the
objects in it to the given
wrong doc style.  complete sentence, with subject of "DumpHeap".

// DumpHeap writes a description of the heap and the
// objects in it to the given file descriptor.

You might also want to note the format, or link to more info.

You could make a golang.org/s/ short link that you can redirect later.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:155: func DumpHeap(fd uintptr)
thanks for making this take an fd.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/runtime.h...
src/pkg/runtime/runtime.h:969: void	runtime·iterateItabs(void
(*callback)(Itab*));
intentional capital letter in interateItabs compared to others? looks unique.

[rsc, 2014-03-13 01:04:03]

i'm very excited to see this going in. lots of minor comments but it's great
overall.

please start a doc or a wiki page describing the heap dump format. if you do
that i promise to write a trivial heapdump-to-text program from the spec, to
test that the spec matches what gets written.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:150: // Dump description of the heap and the
objects in it to the given
On 2014/03/12 23:08:40, bradfitz wrote:
> wrong doc style.  complete sentence, with subject of "DumpHeap".
> 
> // DumpHeap writes a description of the heap and the
> // objects in it to the given file descriptor.
> 
> You might also want to note the format, or link to more info.
> 
> You could make a golang.org/s/ short link that you can redirect later.

yes please. use golang.org/s/go13heapdump. we can create it later.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:153: // This feature is experimental and is
subject to change
Let's omit this. We definitely want a heap dump, and the fact that the format
might be different in a future release is okay, because we have the versioned
header. The API is good.

The main problem with saying things like this is that even one of these breaks
the overall 'If you update to a newer version of Go, your program will keep
working' guarantee. If we write a different file format, no big deal. But if we
delete the API or change the signature then the program doesn't build anymore.
We won't make a change like that, so we don't need a disclaimer.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:155: func DumpHeap(fd uintptr)
runtime/pprof uses WriteHeapProfile; let's make this WriteHeapDump for
consistency.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:10: // This feature is experimental and is subject to
change
please remove (rationale in earlier comment).

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:38: static uintptr dump;
dumpfd, just to be clear when used

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:46: static void
sorry, but can you put a blank line before each function definition?
things start to run together in my brain.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:159: dumpint(1);
can we make an enum of these? it will be easier to maintain and not accidentally
reuse one as we add more.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:167: dumpstackroot(byte *from, byte *to, byte *frame)
might be easier on callers to derive 'to' from 'from' in the body of this
function.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:175: dumpdataroot(byte* from, byte *to)
same

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:200: dumpthread(G *gp, uintptr sp)
rename to dumpgoroutine

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:205: // TODO: other G fields interesting?
yes please

goid
gopc
status
issystem
isbackground
waitsince
waitreason

since we're here let's emit the panic and defer lists and contents too.
they should be short and if they're not that's useful to know.
especially since defer is pooled and can be a source of conservative pointers,
it would be good to be able to tie content back to a specific goroutine.

when you dump the defer FuncVal* please dump both the FuncVal* and its fn field.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:217: dumpint(sp); // frame identified by sp
recording sp and fp is good but i think there should also be an ordinary
sequence number (0 = innermost will be easiest) emitted first as the identifier,
just to avoid tools running into confusion when two frames share the same sp (as
can happen on arm for leaf functions).

then dumpstackroot's last argument is the frame sequence number.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:219: name = runtime·funcname(fn);
dumpint(fn->entry);
dumpint(pc);

so we know where the function was stopped.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:280: dumpframe(s->sp, s->fp, s->fn);
pass s->pc here; we'll let the readers back it up.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:294: // Scan locals.
i really like the fact that dumpobj writes the whole object memory and leaves
interpretation separate, because it lets us do things like on x86-64 look for
integers that are obviously pointers, or vice versa. i wonder if we should do
the same thing here. can dump the whole frame in a single write and then emit
what the stack maps say about the data words in terms of offsets (this one is a
pointer, this one is dead, this is an interface, and so on). we're already
dumping all the live objects; the full frames won't add much to the overall
dump.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:353: // treat ctxt pointer as being held by caller
frame
not sure what caller means here. topmost?

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:394: dumproots(void)
i compared this against markroot in mgc0.c and it looks like the finalizer queue
(blocks waiting to be finalized, case RootFinalizers) is missing. everything
else looks good.

please add a comment to markroot saying
// If you add a case here, please update the heap dumper in heapdump.c.

looking for globals that are special enough to be worth repeating, even though
they're already there in some form, it might be useful to dump the list of timer
structs because those closures can pin things. same comment about FuncVal - dump
both the FuncVal* and its fn field.

similarly, let's dump the address of each of the g and m structs (using allg and
allm) so that we can identify those blocks in the heap.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:451: dumpfinalizer(p, (byte*)spf->fn);
it would be nice to know what the three values mean, and that will be easier to
infer if all three things go into a single record (that is, dumpfinalizer(p,
(byte*)spf->fn, (byte*)spf->fint, (byte*)spf->ot).
right now you could figure that out from the order of the triples but nicer to
be explicit.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:510: dumpint(runtime·Hchansize);
please dump runtime.mheap.arena_start and arena_used, so that we can distinguish
dangling pointers into the heap from pointers to non-heap memory.

let's dump most of the MStats too, stopping after numgc. that will require
calling updatememstats after stoptheworld.

also
dumpint(thechar);
dumpstring(GOEXPERIMENT);
dumpint(runtime.ncpu);
you'll need to #include "arch_GOARCH.h" and "zaexperiment.h".

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:525: dumpitabs(void)
why? i'm just curious. the itabs are allocated separately from the heap and
contain no heap pointers.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:527: runtime·iterateItabs(itab_callback);
please make it iterate_itabs if it stays.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:542: // TODO: Ms?
at least the addresses; see note in dumpparams (and maybe that's where to do
it).

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset + prog[1] +
offsetof(String, str));
can you arrange to emit the string length here?
it will be important, as last night's bug fix showed.
probably need a new FieldKind but that's not a bad thing.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:635: callback(arg, FieldKindPtr, offset + prog[1] +
offsetof(Slice, array));
similarly, slice capacity and possibly a new FieldKind.

[dvyukov, 2014-03-13 11:13:37]

Cool!

I am curious can it be used to debug the flaky finalizer issues that we have?
E.g.:
https://code.google.com/p/go/issues/detail?id=7480
https://code.google.com/p/go/issues/detail?id=7343

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:1: // Copyright 2013 The Go Authors. All rights
reserved.
s/2013/2014/

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
Are you sure this is correct now?
I think that now free objects in mcache/mcentral spans are marked as allocated,
so that we don't need to mark them on allocation.

[dvyukov, 2014-03-13 11:17:17]

On 2014/03/13 11:13:37, dvyukov wrote:
> Cool!
> 
> I am curious can it be used to debug the flaky finalizer issues that we have?
> E.g.:
> https://code.google.com/p/go/issues/detail?id=7480
> https://code.google.com/p/go/issues/detail?id=7343


And also why
https://codereview.appspot.com/71080043/
was flaky?

[rsc, 2014-03-13 13:32:03]

On Thu, Mar 13, 2014 at 7:13 AM, <dvyukov@google.com> wrote:

> Cool!
>
> I am curious can it be used to debug the flaky finalizer issues that we
> have? E.g.:
> https://code.google.com/p/go/issues/detail?id=7480
> https://code.google.com/p/go/issues/detail?id=7343


Yes. I found one this way.

Russ

[rsc, 2014-03-13 13:33:53]

On Thu, Mar 13, 2014 at 7:17 AM, <dvyukov@google.com> wrote:

> And also why
> https://codereview.appspot.com/71080043/
> was flaky?
>

I believe that one is not due to the heap. Otherwise an immediate second
garbage collection would have no effect. I think that one is due to
missynchronization in bgsweep where sometimes the bgsweep goroutine can
miss a wakeup and so it does not do anything in the background during a
collection cycle.

Russ

[khr, 2014-03-13 22:59:08]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:167: dumpstackroot(byte *from, byte *to, byte *frame)
On 2014/03/13 01:04:04, rsc wrote:
> might be easier on callers to derive 'to' from 'from' in the body of this
> function.

This won't be possible for, e.g., the context ptr root.  It doesn't live in a
memory location but we do want to attribute it to a frame.
We could give &gp->sched.ctxt as the pointer, but that doesn't make much sense
in the dump.  Either nil or (as I currently have) the sp.

Or maybe we should attach ctxt somewhere else - G, maybe?

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:175: dumpdataroot(byte* from, byte *to)
On 2014/03/13 01:04:04, rsc wrote:
> same

I will do as you suggest here.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:217: dumpint(sp); // frame identified by sp
On 2014/03/13 01:04:04, rsc wrote:
> recording sp and fp is good but i think there should also be an ordinary
> sequence number (0 = innermost will be easiest) emitted first as the
identifier,
> just to avoid tools running into confusion when two frames share the same sp
(as
> can happen on arm for leaf functions).
> 
> then dumpstackroot's last argument is the frame sequence number.

I like the simplicity of having sp be a unique frame key.  I'd rather skip over
frameless routines - they contain nothing interesting heap-wise.  It will only
happen on the arm, and then only at method prolog, which might as well be viewed
as suspending at the parent.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:294: // Scan locals.
On 2014/03/13 01:04:04, rsc wrote:
> i really like the fact that dumpobj writes the whole object memory and leaves
> interpretation separate, because it lets us do things like on x86-64 look for
> integers that are obviously pointers, or vice versa. i wonder if we should do
> the same thing here. can dump the whole frame in a single write and then emit
> what the stack maps say about the data words in terms of offsets (this one is
a
> pointer, this one is dead, this is an interface, and so on). we're already
> dumping all the live objects; the full frames won't add much to the overall
> dump.

That sounds like a good idea.  Then people can inspect non-ptr values in their
frames.

Should we also do so for data & bss?  data works, but I'm afraid bss might be
really big.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
On 2014/03/13 11:13:38, dvyukov wrote:
> Are you sure this is correct now?
> I think that now free objects in mcache/mcentral spans are marked as
allocated,
> so that we don't need to mark them on allocation.

Yes, this test currently only filters out FlagNoGC allocations, I'll update the
comment.  Objects in the free list will get dumped, that's fine.  So will
unreachable but not yet GCd objects.  The reader will have to do a reachability
pass to eliminate the latter, as a side effect it will eliminate the former
also.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:525: dumpitabs(void)
On 2014/03/13 01:04:04, rsc wrote:
> why? i'm just curious. the itabs are allocated separately from the heap and
> contain no heap pointers.

It is needed for precise analysis of KindIface fields in a reader.  We need to
know, given the itab field of an interface, whether the data field is a pointer
or not.

Similarly, dumpefacetypes makes sure we have dumped all types that might appear
in the type field of an empty interface, so we can resolve KindEface fields.

[rsc, 2014-03-13 23:05:39]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:167: dumpstackroot(byte *from, byte *to, byte *frame)
On 2014/03/13 22:59:09, khr wrote:
> On 2014/03/13 01:04:04, rsc wrote:
> > might be easier on callers to derive 'to' from 'from' in the body of this
> > function.
> 
> This won't be possible for, e.g., the context ptr root.  It doesn't live in a
> memory location but we do want to attribute it to a frame.
> We could give &gp->sched.ctxt as the pointer, but that doesn't make much sense
> in the dump.  Either nil or (as I currently have) the sp.
> 
> Or maybe we should attach ctxt somewhere else - G, maybe?

Since gp->sched.ctxt is not on the stack, let's not treat it as a stack root.
Let's leave 'stack roots' for things in stack memory only. I'm sure that will
confuse me otherwise. Doing it in the G (dumpgoroutine) sounds good.

[rsc, 2014-03-13 23:11:58]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:217: dumpint(sp); // frame identified by sp
On 2014/03/13 22:59:09, khr wrote:
> On 2014/03/13 01:04:04, rsc wrote:
> > recording sp and fp is good but i think there should also be an ordinary
> > sequence number (0 = innermost will be easiest) emitted first as the
> identifier,
> > just to avoid tools running into confusion when two frames share the same sp
> (as
> > can happen on arm for leaf functions).
> > 
> > then dumpstackroot's last argument is the frame sequence number.
> 
> I like the simplicity of having sp be a unique frame key.  I'd rather skip
over
> frameless routines - they contain nothing interesting heap-wise.  It will only
> happen on the arm, and then only at method prolog, which might as well be
viewed
> as suspending at the parent.

I'd rather record the sp as the frame ID and warn readers to deal with the fact
that the ARM might have two frames with the same ID than start dropping frames.
I agree that it can be viewed that way, but I'd like to preserve as much
information as possible about the state of the world. I'm sure I will be
confused otherwise, since normally we don't see a call instruction at the top of
a stack trace. Even though the original motivation is for a heap dump to
understand the object graph, I think this is also a good snapshot for debugging
the garbage collector and such. For example I might while debugging arrange to
write a new heap dump out at each collection. And for that I really the
information to be as true as possible.

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:294: // Scan locals.
On 2014/03/13 22:59:09, khr wrote:
> On 2014/03/13 01:04:04, rsc wrote:
> > i really like the fact that dumpobj writes the whole object memory and
leaves
> > interpretation separate, because it lets us do things like on x86-64 look
for
> > integers that are obviously pointers, or vice versa. i wonder if we should
do
> > the same thing here. can dump the whole frame in a single write and then
emit
> > what the stack maps say about the data words in terms of offsets (this one
is
> a
> > pointer, this one is dead, this is an interface, and so on). we're already
> > dumping all the live objects; the full frames won't add much to the overall
> > dump.
> 
> That sounds like a good idea.  Then people can inspect non-ptr values in their
> frames.
> 
> Should we also do so for data & bss?  data works, but I'm afraid bss might be
> really big.

I agree it would be nice but also that bss might be too big. Certainly we'd want
to exclude the 'mheap' variable, which is actually in the noptrbss. Maybe we can
allow data and bss but not bother with noptrdata and noptrbss?

[rsc, 2014-03-13 23:12:53]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:217: dumpint(sp); // frame identified by sp
Also, unwinding the frame would suggest that the frame has returned, not that it
hasn't started.

[khr, 2014-03-14 01:11:17]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset + prog[1] +
offsetof(String, str));
On 2014/03/13 01:04:04, rsc wrote:
> can you arrange to emit the string length here?
> it will be important, as last night's bug fix showed.
> probably need a new FieldKind but that's not a bad thing.

We can do this, but keep in mind that we can only get this information for heap
objects and globals.  We don't currently have it for stacks.

[rsc, 2014-03-14 04:25:21]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset + prog[1] +
offsetof(String, str));
On 2014/03/14 01:11:17, khr wrote:
> On 2014/03/13 01:04:04, rsc wrote:
> > can you arrange to emit the string length here?
> > it will be important, as last night's bug fix showed.
> > probably need a new FieldKind but that's not a bad thing.
> 
> We can do this, but keep in mind that we can only get this information for
heap
> objects and globals.  We don't currently have it for stacks.

Oh I see. The length is only important if we are also recording that this is a
string. But if we're not, it doesn't matter as much. (It can still cause a false
retention, actually, but not a corruption.) Okay, then let's leave this as a
plain pointer. Same for slice below.

[dvyukov, 2014-03-14 05:52:06]

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
On 2014/03/13 22:59:09, khr wrote:
> On 2014/03/13 11:13:38, dvyukov wrote:
> > Are you sure this is correct now?
> > I think that now free objects in mcache/mcentral spans are marked as
> allocated,
> > so that we don't need to mark them on allocation.
> 
> Yes, this test currently only filters out FlagNoGC allocations, I'll update
the
> comment.  Objects in the free list will get dumped, that's fine.  So will
> unreachable but not yet GCd objects.  The reader will have to do a
reachability
> pass to eliminate the latter, as a side effect it will eliminate the former
> also.

This really looks like the tool's job. Why does one ever wants to see garbage in
the dump (not saying about freed memory)?
Dumps can be really huge, both in terms of file size and visual representation.
And currently interesting objects can be only 1/3 of that.

[khr1, 2014-03-14 15:48:33]

I can filter out things in the freelist, that's fine.  Unreachable but not
yet GCd objects are harder, as I don't want to run a whole GC before
dumping.



On Thu, Mar 13, 2014 at 10:52 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/37540043/diff/430001/src/
> pkg/runtime/heapdump.c
> File src/pkg/runtime/heapdump.c (right):
>
> https://codereview.appspot.com/37540043/diff/430001/src/
> pkg/runtime/heapdump.c#newcode482
> src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
> On 2014/03/13 22:59:09, khr wrote:
>
>> On 2014/03/13 11:13:38, dvyukov wrote:
>> > Are you sure this is correct now?
>> > I think that now free objects in mcache/mcentral spans are marked as
>> allocated,
>> > so that we don't need to mark them on allocation.
>>
>
>  Yes, this test currently only filters out FlagNoGC allocations, I'll
>>
> update the
>
>> comment.  Objects in the free list will get dumped, that's fine.  So
>>
> will
>
>> unreachable but not yet GCd objects.  The reader will have to do a
>>
> reachability
>
>> pass to eliminate the latter, as a side effect it will eliminate the
>>
> former
>
>> also.
>>
>
> This really looks like the tool's job. Why does one ever wants to see
> garbage in the dump (not saying about freed memory)?
> Dumps can be really huge, both in terms of file size and visual
> representation. And currently interesting objects can be only 1/3 of
> that.
>
> https://codereview.appspot.com/37540043/
>

[khr1, 2014-03-14 15:49:22]

And to the extent that this tool will be used for debugging GC, running the
GC before dumping defeats the purpose.


On Fri, Mar 14, 2014 at 8:48 AM, Keith Randall <khr@google.com> wrote:

> I can filter out things in the freelist, that's fine.  Unreachable but not
> yet GCd objects are harder, as I don't want to run a whole GC before
> dumping.
>
>
>
> On Thu, Mar 13, 2014 at 10:52 PM, <dvyukov@google.com> wrote:
>
>>
>> https://codereview.appspot.com/37540043/diff/430001/src/
>> pkg/runtime/heapdump.c
>> File src/pkg/runtime/heapdump.c (right):
>>
>> https://codereview.appspot.com/37540043/diff/430001/src/
>> pkg/runtime/heapdump.c#newcode482
>> src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
>> On 2014/03/13 22:59:09, khr wrote:
>>
>>> On 2014/03/13 11:13:38, dvyukov wrote:
>>> > Are you sure this is correct now?
>>> > I think that now free objects in mcache/mcentral spans are marked as
>>> allocated,
>>> > so that we don't need to mark them on allocation.
>>>
>>
>>  Yes, this test currently only filters out FlagNoGC allocations, I'll
>>>
>> update the
>>
>>> comment.  Objects in the free list will get dumped, that's fine.  So
>>>
>> will
>>
>>> unreachable but not yet GCd objects.  The reader will have to do a
>>>
>> reachability
>>
>>> pass to eliminate the latter, as a side effect it will eliminate the
>>>
>> former
>>
>>> also.
>>>
>>
>> This really looks like the tool's job. Why does one ever wants to see
>> garbage in the dump (not saying about freed memory)?
>> Dumps can be really huge, both in terms of file size and visual
>> representation. And currently interesting objects can be only 1/3 of
>> that.
>>
>> https://codereview.appspot.com/37540043/
>>
>
>

[khr1, 2014-03-14 15:56:16]

On Thu, Mar 13, 2014 at 9:25 PM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/37540043/diff/430001/src/
> pkg/runtime/heapdump.c
> File src/pkg/runtime/heapdump.c (right):
>
> https://codereview.appspot.com/37540043/diff/430001/src/
> pkg/runtime/heapdump.c#newcode623
> src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset +
> prog[1] + offsetof(String, str));
> On 2014/03/14 01:11:17, khr wrote:
>
>> On 2014/03/13 01:04:04, rsc wrote:
>> > can you arrange to emit the string length here?
>> > it will be important, as last night's bug fix showed.
>> > probably need a new FieldKind but that's not a bad thing.
>>
>
>  We can do this, but keep in mind that we can only get this information
>>
> for heap
>
>> objects and globals.  We don't currently have it for stacks.
>>
>
> Oh I see. The length is only important if we are also recording that
> this is a string. But if we're not, it doesn't matter as much. (It can
> still cause a false retention, actually, but not a corruption.) Okay,
> then let's leave this as a plain pointer. Same for slice below.
>
>
Actually, we have enough bits to fix this.  We could have our two bits/word
encode the following:
0: not ptr
1: ptr
2: multi-word item

If a word is marked as a multi-word item, the next word's bytes encode:
0: string
1: slice
2: iface
3: eface

That should fix the false retention problem for stacks.  It also lets us
use markonly for string pointers which would speed things up a bit.

And there's one more encoding still available for distinguishing dead words
vs. scalar words.

https://codereview.appspot.com/37540043/
>

[rsc, 2014-03-14 15:57:51]

On Fri, Mar 14, 2014 at 11:56 AM, Keith Randall <khr@google.com> wrote:

> On Thu, Mar 13, 2014 at 9:25 PM, <rsc@golang.org> wrote:
>
>>
>> https://codereview.appspot.com/37540043/diff/430001/src/
>> pkg/runtime/heapdump.c
>> File src/pkg/runtime/heapdump.c (right):
>>
>> https://codereview.appspot.com/37540043/diff/430001/src/
>> pkg/runtime/heapdump.c#newcode623
>> src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset +
>> prog[1] + offsetof(String, str));
>> On 2014/03/14 01:11:17, khr wrote:
>>
>>> On 2014/03/13 01:04:04, rsc wrote:
>>> > can you arrange to emit the string length here?
>>> > it will be important, as last night's bug fix showed.
>>> > probably need a new FieldKind but that's not a bad thing.
>>>
>>
>>  We can do this, but keep in mind that we can only get this information
>>>
>> for heap
>>
>>> objects and globals.  We don't currently have it for stacks.
>>>
>>
>> Oh I see. The length is only important if we are also recording that
>> this is a string. But if we're not, it doesn't matter as much. (It can
>> still cause a false retention, actually, but not a corruption.) Okay,
>> then let's leave this as a plain pointer. Same for slice below.
>>
>>
> Actually, we have enough bits to fix this.  We could have our two
> bits/word encode the following:
> 0: not ptr
> 1: ptr
> 2: multi-word item
>
> If a word is marked as a multi-word item, the next word's bytes encode:
> 0: string
> 1: slice
> 2: iface
> 3: eface
>
> That should fix the false retention problem for stacks.  It also lets us
> use markonly for string pointers which would speed things up a bit.
>

> And there's one more encoding still available for distinguishing dead
> words vs. scalar words.
>

Sounds great. Should we do it for 1.3 (in the next week or two) or should
we leave it for 1.4?

Russ

[dvyukov, 2014-03-14 16:08:38]

On Fri, Mar 14, 2014 at 7:57 PM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Mar 14, 2014 at 11:56 AM, Keith Randall <khr@google.com> wrote:
>>
>> On Thu, Mar 13, 2014 at 9:25 PM, <rsc@golang.org> wrote:
>>>
>>>
>>>
>>>
https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
>>> File src/pkg/runtime/heapdump.c (right):
>>>
>>>
>>>
https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
>>> src/pkg/runtime/heapdump.c:623: callback(arg, FieldKindPtr, offset +
>>> prog[1] + offsetof(String, str));
>>> On 2014/03/14 01:11:17, khr wrote:
>>>>
>>>> On 2014/03/13 01:04:04, rsc wrote:
>>>> > can you arrange to emit the string length here?
>>>> > it will be important, as last night's bug fix showed.
>>>> > probably need a new FieldKind but that's not a bad thing.
>>>
>>>
>>>> We can do this, but keep in mind that we can only get this information
>>>
>>> for heap
>>>>
>>>> objects and globals.  We don't currently have it for stacks.
>>>
>>>
>>> Oh I see. The length is only important if we are also recording that
>>> this is a string. But if we're not, it doesn't matter as much. (It can
>>> still cause a false retention, actually, but not a corruption.) Okay,
>>> then let's leave this as a plain pointer. Same for slice below.
>>>
>>
>> Actually, we have enough bits to fix this.  We could have our two
>> bits/word encode the following:
>> 0: not ptr
>> 1: ptr
>> 2: multi-word item
>>
>> If a word is marked as a multi-word item, the next word's bytes encode:
>> 0: string
>> 1: slice
>> 2: iface
>> 3: eface

Hey, wait. We have not finished that discussion about GC future, but
Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
pointer) (potentially in 1.4).


>> That should fix the false retention problem for stacks.  It also lets us
>> use markonly for string pointers which would speed things up a bit.
>>
>>
>> And there's one more encoding still available for distinguishing dead
>> words vs. scalar words.
>
>
> Sounds great. Should we do it for 1.3 (in the next week or two) or should we
> leave it for 1.4?

[dvyukov, 2014-03-14 16:10:54]

On Fri, Mar 14, 2014 at 7:49 PM, Keith Randall <khr@google.com> wrote:
> And to the extent that this tool will be used for debugging GC, running the
> GC before dumping defeats the purpose.

But there is another use case -- for end user to examine his live
heap. And for this use case any garbage is very distracting. Can we
make it a bool argument -- dump everything or only alive?

> On Fri, Mar 14, 2014 at 8:48 AM, Keith Randall <khr@google.com> wrote:
>>
>> I can filter out things in the freelist, that's fine.  Unreachable but not
>> yet GCd objects are harder, as I don't want to run a whole GC before
>> dumping.
>>
>>
>>
>> On Thu, Mar 13, 2014 at 10:52 PM, <dvyukov@google.com> wrote:
>>>
>>>
>>>
>>>
https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump.c
>>> File src/pkg/runtime/heapdump.c (right):
>>>
>>>
>>>
https://codereview.appspot.com/37540043/diff/430001/src/pkg/runtime/heapdump....
>>> src/pkg/runtime/heapdump.c:482: if((bits & bitAllocated) == 0)
>>> On 2014/03/13 22:59:09, khr wrote:
>>>>
>>>> On 2014/03/13 11:13:38, dvyukov wrote:
>>>> > Are you sure this is correct now?
>>>> > I think that now free objects in mcache/mcentral spans are marked as
>>>> allocated,
>>>> > so that we don't need to mark them on allocation.
>>>
>>>
>>>> Yes, this test currently only filters out FlagNoGC allocations, I'll
>>>
>>> update the
>>>>
>>>> comment.  Objects in the free list will get dumped, that's fine.  So
>>>
>>> will
>>>>
>>>> unreachable but not yet GCd objects.  The reader will have to do a
>>>
>>> reachability
>>>>
>>>> pass to eliminate the latter, as a side effect it will eliminate the
>>>
>>> former
>>>>
>>>> also.
>>>
>>>
>>> This really looks like the tool's job. Why does one ever wants to see
>>> garbage in the dump (not saying about freed memory)?
>>> Dumps can be really huge, both in terms of file size and visual
>>> representation. And currently interesting objects can be only 1/3 of
>>> that.
>>>
>>> https://codereview.appspot.com/37540043/
>>
>>
>

[rsc, 2014-03-14 16:14:16]

If you want to dump only live data, call runtime.GC before dumping the heap.

[rsc, 2014-03-14 16:15:13]

Also, in any event, this is only the raw dump. There will be tools to
process it for sure. So it doesn't really affect end users.

[rsc, 2014-03-14 16:18:49]

[again, replying all]

On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> Hey, wait. We have not finished that discussion about GC future, but
> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
> pointer) (potentially in 1.4).
>

Okay, but that's obviously not happening for 1.3, and of course Carl is no
longer working on this. Keith was already talking about adding the dead bit
in the next week or two so that we can have a testing mode that poisons
dead pointers. If the string/slice bit goes in as part of that, it's fine
with me. Adding it here does not preclude making changes in Go 1.4. Let's
leave the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
tables.

Russ

[dvyukov, 2014-03-14 16:37:46]

On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <rsc@golang.org> wrote:
> [again, replying all]
>
> On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> Hey, wait. We have not finished that discussion about GC future, but
>> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
>> pointer) (potentially in 1.4).
>
>
> Okay, but that's obviously not happening for 1.3, and of course Carl is no
> longer working on this. Keith was already talking about adding the dead bit
> in the next week or two so that we can have a testing mode that poisons dead
> pointers. If the string/slice bit goes in as part of that, it's fine with
> me. Adding it here does not preclude making changes in Go 1.4. Let's leave
> the 1-bit discussions for the 1.4 planning. Right now we have 2-bit tables.


I understand that now we have 2-bit tables, but I don't want to submit
something just before the release that can significantly limit
implementation choices in future.

[rsc, 2014-03-14 16:55:05]

On Fri, Mar 14, 2014 at 12:36 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <rsc@golang.org> wrote:
> > [again, replying all]
> >
> > On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> >>
> >> Hey, wait. We have not finished that discussion about GC future, but
> >> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
> >> pointer) (potentially in 1.4).
> >
> >
> > Okay, but that's obviously not happening for 1.3, and of course Carl is
> no
> > longer working on this. Keith was already talking about adding the dead
> bit
> > in the next week or two so that we can have a testing mode that poisons
> dead
> > pointers. If the string/slice bit goes in as part of that, it's fine with
> > me. Adding it here does not preclude making changes in Go 1.4. Let's
> leave
> > the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
> tables.
>
>
> I understand that now we have 2-bit tables, but I don't want to submit
> something just before the release that can significantly limit
> implementation choices in future.
>

I promise that we can make changes to all of this in 1.4.

Russ

[dvyukov, 2014-03-14 16:57:18]

On Fri, Mar 14, 2014 at 8:55 PM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Mar 14, 2014 at 12:36 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <rsc@golang.org> wrote:
>> > [again, replying all]
>> >
>> > On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov <dvyukov@google.com>
>> > wrote:
>> >>
>> >> Hey, wait. We have not finished that discussion about GC future, but
>> >> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
>> >> pointer) (potentially in 1.4).
>> >
>> >
>> > Okay, but that's obviously not happening for 1.3, and of course Carl is
>> > no
>> > longer working on this. Keith was already talking about adding the dead
>> > bit
>> > in the next week or two so that we can have a testing mode that poisons
>> > dead
>> > pointers. If the string/slice bit goes in as part of that, it's fine
>> > with
>> > me. Adding it here does not preclude making changes in Go 1.4. Let's
>> > leave
>> > the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
>> > tables.
>>
>>
>> I understand that now we have 2-bit tables, but I don't want to submit
>> something just before the release that can significantly limit
>> implementation choices in future.
>
>
> I promise that we can make changes to all of this in 1.4.

OK

[khr1, 2014-03-15 06:06:21]

So there's a problem with fixing false retention for slices.  To fix the
problem I need to check if the capacity of a slice is 0 or not.  There are
slices on the stack that are live, but the capacity is junk.  There is
probably some optimization that avoids storing the capacity to the stack if
it isn't going to be used.  It happens in code like the following:

func f(n int) int {
        s := make([]int, n)
        runtime.GC()
        return s[0]
}

Since s.cap isn't ever used, it is never saved to (the local variable
storage for) s upon the return from make.  Inside the gc call s doesn't
have a valid cap.

So we either fix this bug, or allow false retention from slices.  For 1.3,
I'm leaning towards the latter, compiler changes at this point scare me.
 Unless someone sees that it is easy to do.  I'll prepare my changelist
with the slice stuff disabled for now.

I'll open some bugs for this stuff.



On Fri, Mar 14, 2014 at 9:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Fri, Mar 14, 2014 at 8:55 PM, Russ Cox <rsc@golang.org> wrote:
> > On Fri, Mar 14, 2014 at 12:36 PM, Dmitry Vyukov <dvyukov@google.com>
> wrote:
> >>
> >> On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <rsc@golang.org> wrote:
> >> > [again, replying all]
> >> >
> >> > On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov <dvyukov@google.com>
> >> > wrote:
> >> >>
> >> >> Hey, wait. We have not finished that discussion about GC future, but
> >> >> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
> >> >> pointer) (potentially in 1.4).
> >> >
> >> >
> >> > Okay, but that's obviously not happening for 1.3, and of course Carl
> is
> >> > no
> >> > longer working on this. Keith was already talking about adding the
> dead
> >> > bit
> >> > in the next week or two so that we can have a testing mode that
> poisons
> >> > dead
> >> > pointers. If the string/slice bit goes in as part of that, it's fine
> >> > with
> >> > me. Adding it here does not preclude making changes in Go 1.4. Let's
> >> > leave
> >> > the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
> >> > tables.
> >>
> >>
> >> I understand that now we have 2-bit tables, but I don't want to submit
> >> something just before the release that can significantly limit
> >> implementation choices in future.
> >
> >
> > I promise that we can make changes to all of this in 1.4.
>
> OK
>

[khr, 2014-03-24 21:53:11]

On 2014/03/15 06:06:21, khr1 wrote:
> So there's a problem with fixing false retention for slices.  To fix the
> problem I need to check if the capacity of a slice is 0 or not.  There are
> slices on the stack that are live, but the capacity is junk.  There is
> probably some optimization that avoids storing the capacity to the stack if
> it isn't going to be used.  It happens in code like the following:
> 
> func f(n int) int {
>         s := make([]int, n)
>         runtime.GC()
>         return s[0]
> }
> 
> Since s.cap isn't ever used, it is never saved to (the local variable
> storage for) s upon the return from make.  Inside the gc call s doesn't
> have a valid cap.
> 
> So we either fix this bug, or allow false retention from slices.  For 1.3,
> I'm leaning towards the latter, compiler changes at this point scare me.
>  Unless someone sees that it is easy to do.  I'll prepare my changelist
> with the slice stuff disabled for now.
> 
> I'll open some bugs for this stuff.
> 
> 
> 
> On Fri, Mar 14, 2014 at 9:56 AM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > On Fri, Mar 14, 2014 at 8:55 PM, Russ Cox <mailto:rsc@golang.org> wrote:
> > > On Fri, Mar 14, 2014 at 12:36 PM, Dmitry Vyukov
<mailto:dvyukov@google.com>
> > wrote:
> > >>
> > >> On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <mailto:rsc@golang.org> wrote:
> > >> > [again, replying all]
> > >> >
> > >> > On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov
<mailto:dvyukov@google.com>
> > >> > wrote:
> > >> >>
> > >> >> Hey, wait. We have not finished that discussion about GC future, but
> > >> >> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
> > >> >> pointer) (potentially in 1.4).
> > >> >
> > >> >
> > >> > Okay, but that's obviously not happening for 1.3, and of course Carl
> > is
> > >> > no
> > >> > longer working on this. Keith was already talking about adding the
> > dead
> > >> > bit
> > >> > in the next week or two so that we can have a testing mode that
> > poisons
> > >> > dead
> > >> > pointers. If the string/slice bit goes in as part of that, it's fine
> > >> > with
> > >> > me. Adding it here does not preclude making changes in Go 1.4. Let's
> > >> > leave
> > >> > the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
> > >> > tables.
> > >>
> > >>
> > >> I understand that now we have 2-bit tables, but I don't want to submit
> > >> something just before the release that can significantly limit
> > >> implementation choices in future.
> > >
> > >
> > > I promise that we can make changes to all of this in 1.4.
> >
> > OK
> >

I've updated with all of your comments.  PTAL.

[khr, 2014-03-25 16:27:55]

On 2014/03/24 21:53:11, khr wrote:
> On 2014/03/15 06:06:21, khr1 wrote:
> > So there's a problem with fixing false retention for slices.  To fix the
> > problem I need to check if the capacity of a slice is 0 or not.  There are
> > slices on the stack that are live, but the capacity is junk.  There is
> > probably some optimization that avoids storing the capacity to the stack if
> > it isn't going to be used.  It happens in code like the following:
> > 
> > func f(n int) int {
> >         s := make([]int, n)
> >         runtime.GC()
> >         return s[0]
> > }
> > 
> > Since s.cap isn't ever used, it is never saved to (the local variable
> > storage for) s upon the return from make.  Inside the gc call s doesn't
> > have a valid cap.
> > 
> > So we either fix this bug, or allow false retention from slices.  For 1.3,
> > I'm leaning towards the latter, compiler changes at this point scare me.
> >  Unless someone sees that it is easy to do.  I'll prepare my changelist
> > with the slice stuff disabled for now.
> > 
> > I'll open some bugs for this stuff.
> > 
> > 
> > 
> > On Fri, Mar 14, 2014 at 9:56 AM, Dmitry Vyukov <mailto:dvyukov@google.com>
> wrote:
> > 
> > > On Fri, Mar 14, 2014 at 8:55 PM, Russ Cox <mailto:rsc@golang.org> wrote:
> > > > On Fri, Mar 14, 2014 at 12:36 PM, Dmitry Vyukov
> <mailto:dvyukov@google.com>
> > > wrote:
> > > >>
> > > >> On Fri, Mar 14, 2014 at 8:18 PM, Russ Cox <mailto:rsc@golang.org>
wrote:
> > > >> > [again, replying all]
> > > >> >
> > > >> > On Fri, Mar 14, 2014 at 12:08 PM, Dmitry Vyukov
> <mailto:dvyukov@google.com>
> > > >> > wrote:
> > > >> >>
> > > >> >> Hey, wait. We have not finished that discussion about GC future, but
> > > >> >> Carl's and my plan was to switch to 1 bit/word encoding (pointer/not
> > > >> >> pointer) (potentially in 1.4).
> > > >> >
> > > >> >
> > > >> > Okay, but that's obviously not happening for 1.3, and of course Carl
> > > is
> > > >> > no
> > > >> > longer working on this. Keith was already talking about adding the
> > > dead
> > > >> > bit
> > > >> > in the next week or two so that we can have a testing mode that
> > > poisons
> > > >> > dead
> > > >> > pointers. If the string/slice bit goes in as part of that, it's fine
> > > >> > with
> > > >> > me. Adding it here does not preclude making changes in Go 1.4. Let's
> > > >> > leave
> > > >> > the 1-bit discussions for the 1.4 planning. Right now we have 2-bit
> > > >> > tables.
> > > >>
> > > >>
> > > >> I understand that now we have 2-bit tables, but I don't want to submit
> > > >> something just before the release that can significantly limit
> > > >> implementation choices in future.
> > > >
> > > >
> > > > I promise that we can make changes to all of this in 1.4.
> > >
> > > OK
> > >
> 
> I've updated with all of your comments.  PTAL.

Bump with correct account.

[rsc, 2014-03-25 19:37:50]

LGTM

https://codereview.appspot.com/37540043/diff/640002/src/pkg/runtime/debug/gar...
File src/pkg/runtime/debug/garbage.go (right):

https://codereview.appspot.com/37540043/diff/640002/src/pkg/runtime/debug/gar...
src/pkg/runtime/debug/garbage.go:151: // it to the given file descriptor.
// The heap dump format is defined at http://golang.org/s/go13heapdump.

(I created that shortcut.)

https://codereview.appspot.com/37540043/diff/640002/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/37540043/diff/640002/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:29: FieldKindPtr = 0,
since we have no legacy code yet, can we flip things around so that FieldKindEol
is first, so that it won't be in the middle when new field kinds are added
later. that is, make it 0 and then 1..5 are the existing non-end things.

similarly, can we renumber the tags to pack them consecutively, and make TagEOF
1 (or 0 if you want)?

and of course update the wiki page.

but then when you view the spec the first thing you wonder isn't why do the tags
start 1,3,6,...?

https://codereview.appspot.com/37540043/diff/640002/src/pkg/runtime/heapdump....
src/pkg/runtime/heapdump.c:323: dumpint((uintptr)child->sp); // sp of child, or
0 if top of stack
s/top/bottom/ if I understand correctly.
In Go terminology, if A calls B and those are the only things on the stack, A's
frame is at the top and B's is at the bottom. (And Stktop is the thing above all
the call frames.)

[khr, 2014-03-25 22:09:52]

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

runtime: WriteHeapDump dumps the heap to a file.

See http://golang.org/s/go13heapdump for the file format.

LGTM=rsc
R=rsc, bradfitz, dvyukov, khr
CC=golang-codereviews
https://codereview.appspot.com/37540043