Hello alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com, khr@golang.org), I'd like you to review this change to https://code.google.com/p/go
9 years, 10 months ago
(2014-05-30 00:28:08 UTC)
#1
9 years, 10 months ago
(2014-05-30 02:24:35 UTC)
#4
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heap...
File src/pkg/runtime/debug/heapdump_test.go (right):
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heap...
src/pkg/runtime/debug/heapdump_test.go:25: const minSize = 10240
On 2014/05/30 01:57:38, khr wrote:
> Where does 10240 come from? I think 1 might be plenty.
pretty arbitrary. I measured the generated file on multiple systems, and all of
them are about ~100KB.
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c
File src/pkg/runtime/os_windows.c (right):
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows...
src/pkg/runtime/os_windows.c:184: handle = (void*)fd;
On 2014/05/30 01:57:38, khr wrote:
> On 2014/05/30 00:47:51, minux wrote:
> > note: on amd64, HANDLE is 64-bit, so this is not entirely right.
> > however, I don't want to change the prototype of runtime.write to take an
> > uintptr just for this.
> >
> > will 64-bit windows actually give out HANDLEs that won't fit in 32-bit?
> I think changing runtime.write to take a uintptr fd would be fine. It isn't
> used in that many places. os.File.Fd() returns a uintptr so we might be ok.
OK. My only concern is that that will change too many files.
But I agree, it's better to be correct.
> Does windows return a casted HANDLE when you call os.File.Fd()?
No.
9 years, 10 months ago
(2014-05-30 03:18:51 UTC)
#5
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c
File src/pkg/runtime/os_windows.c (right):
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows...
src/pkg/runtime/os_windows.c:184: handle = (void*)fd;
On 2014/05/30 02:24:34, minux wrote:
> On 2014/05/30 01:57:38, khr wrote:
> > On 2014/05/30 00:47:51, minux wrote:
> > > note: on amd64, HANDLE is 64-bit, so this is not entirely right.
> > > however, I don't want to change the prototype of runtime.write to take an
> > > uintptr just for this.
> > >
> > > will 64-bit windows actually give out HANDLEs that won't fit in 32-bit?
> > I think changing runtime.write to take a uintptr fd would be fine. It isn't
> > used in that many places. os.File.Fd() returns a uintptr so we might be ok.
> OK. My only concern is that that will change too many files.
> But I agree, it's better to be correct.
> > Does windows return a casted HANDLE when you call os.File.Fd()?
> No.
Then how is this going to work? WriteHeapDump just takes the os.File.Fd()
result and passes it to runtime.write.
On 2014/05/30 03:18:51, khr wrote: > Then how is this going to work? WriteHeapDump just ...
9 years, 10 months ago
(2014-05-30 03:44:26 UTC)
#6
On 2014/05/30 03:18:51, khr wrote:
> Then how is this going to work? WriteHeapDump just takes the os.File.Fd()
> result and passes it to runtime.write.
Because as I understand it, file HANDLE is actually index internal arrays, so
it should be fairly small.
anyway, the newest patch set updated the prototype, tested on darwin, linux
and netbsd.
On 2014/05/30 03:44:26, minux wrote: > On 2014/05/30 03:18:51, khr wrote: > > Then how ...
9 years, 10 months ago
(2014-05-30 04:02:49 UTC)
#7
On 2014/05/30 03:44:26, minux wrote:
> On 2014/05/30 03:18:51, khr wrote:
> > Then how is this going to work? WriteHeapDump just takes the os.File.Fd()
> > result and passes it to runtime.write.
> Because as I understand it, file HANDLE is actually index internal arrays, so
> it should be fairly small.
>
> anyway, the newest patch set updated the prototype, tested on darwin, linux
> and netbsd.
LGTM.
On Sat, May 31, 2014 at 1:30 AM, <gobot@golang.org> wrote: > This CL appears to ...
9 years, 10 months ago
(2014-05-31 09:13:00 UTC)
#12
On Sat, May 31, 2014 at 1:30 AM, <gobot@golang.org> wrote:
> This CL appears to have broken the nacl-amd64p32 builder.
> See http://build.golang.org/log/ef37f913fa65e7e7e7f9b11375363f07f7faccb4
ok, so heap dump is broken on nacl port.
it's because the runtime.write doesn't know about the filesystem emulated by
the syscall package.
I don't think we need to fix this as heap dump is useless on nacl GOOS
anyway
because the file system won't ever reach the disk.
i will submit a CL to skip the test on NaCl, do we want to add this note to
docs for runtime/debug.WriteHeapDump?
I don't think WriteHeapDump itself is broken under NaCl, it's more os.File's fault. Just skipping ...
9 years, 10 months ago
(2014-05-31 17:59:01 UTC)
#13
I don't think WriteHeapDump itself is broken under NaCl, it's more
os.File's fault. Just skipping the test should be fine.
On Sat, May 31, 2014 at 2:12 AM, minux <minux@golang.org> wrote:
>
> On Sat, May 31, 2014 at 1:30 AM, <gobot@golang.org> wrote:
>
>> This CL appears to have broken the nacl-amd64p32 builder.
>> See http://build.golang.org/log/ef37f913fa65e7e7e7f9b11375363f07f7faccb4
>
> ok, so heap dump is broken on nacl port.
>
> it's because the runtime.write doesn't know about the filesystem emulated
> by
> the syscall package.
>
> I don't think we need to fix this as heap dump is useless on nacl GOOS
> anyway
> because the file system won't ever reach the disk.
>
> i will submit a CL to skip the test on NaCl, do we want to add this note to
> docs for runtime/debug.WriteHeapDump?
>
On Sat, May 31, 2014 at 10:59 AM, Keith Randall <khr@google.com> wrote: > I don't ...
9 years, 10 months ago
(2014-06-01 03:25:05 UTC)
#15
On Sat, May 31, 2014 at 10:59 AM, Keith Randall <khr@google.com> wrote:
> I don't think WriteHeapDump itself is broken under NaCl, it's more
> os.File's fault. Just skipping the test should be fine.
>
you're right. i should say that it's a platform limitation rather than heap
dump is broken.
On Sat, May 31, 2014 at 7:19 PM, Russ Cox <rsc@golang.org> wrote:
> skip the test on nacl; there's no need to write anything in the docs.
>
skipped in CL 101980048, already submitted.
Issue 93640044: code review 93640044: runtime: fix empty heap dump bug on windows.
(Closed)
Created 9 years, 10 months ago by minux1
Modified 9 years, 10 months ago
Reviewers: gobot, minux, khr1
Base URL:
Comments: 9