code review 5616072: runtime: delete UpdateMemStats, replace with ReadMemSta...

runtime: delete UpdateMemStats, replace with ReadMemStats(&stats).

Unexports runtime.MemStats and rename MemStatsType to MemStats.
The new accessor requires passing a pointer to a user-allocated
MemStats structure.

Fixes issue 2572.

[remyoudompheng, 2012-02-06 15:00:25]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitzgoog, 2012-02-06 15:56:21]

I like this.

But now it highlights that weird type name.  Could we
s/MemStatsType/MemStats/ and rename the copying accessor?  (Remy, I'm just
asking the group here, not giving review feedback...)



On Mon, Feb 6, 2012 at 7:00 AM, <remyoudompheng@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
> remy@archlinux.org),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> runtime: delete UpdateMemStats, replace with MemStat(&stats).
>
> Fixes issue 2572.
>
> Please review this at
http://codereview.appspot.com/**5616072/<http://codereview.appspot.com/5616072/>
>
> Affected files:
>  M src/cmd/godoc/godoc.go
>  M src/pkg/encoding/gob/timing_**test.go
>  M src/pkg/expvar/expvar.go
>  M src/pkg/fmt/fmt_test.go
>  M src/pkg/net/rpc/server_test.go
>  M src/pkg/reflect/all_test.go
>  M src/pkg/runtime/gc_test.go
>  M src/pkg/runtime/malloc.h
>  M src/pkg/runtime/mem.go
>  M src/pkg/runtime/mgc0.c
>  M src/pkg/runtime/pprof/pprof.go
>  M src/pkg/strconv/itoa_test.go
>  M test/bench/garbage/tree2.go
>  M test/chan/select2.go
>  M test/closure.go
>  M test/gc2.go
>  M test/init1.go
>  M test/malloc1.go
>  M test/mallocrand.go
>  M test/mallocrep.go
>  M test/mallocrep1.go
>
>
>

[rsc, 2012-02-06 16:25:30]

type MemStats

func ReadMemStats(m *MemStats)

?

[bradfitz, 2012-02-06 16:31:11]

Works for me.

On Mon, Feb 6, 2012 at 8:25 AM, <rsc@golang.org> wrote:

> type MemStats
>
> func ReadMemStats(m *MemStats)
>
> ?
>
>
>
http://codereview.appspot.com/**5616072/<http://codereview.appspot.com/5616072/>
>

[gustavo_niemeyer.net, 2012-02-06 16:32:36]

> type MemStats
>
> func ReadMemStats(m *MemStats)

Get would be my suggested color. I've often used that pattern for
top-level functions because of the same conflict.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[bradfitz, 2012-02-06 16:33:53]

On Mon, Feb 6, 2012 at 8:32 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> > type MemStats
> >
> > func ReadMemStats(m *MemStats)
>
> Get would be my suggested color. I've often used that pattern for
> top-level functions because of the same conflict.


I assumed Load (despite being a letter longer) was chosen because the
signature is different, with an out parameter rather than returning.  I
like that as a convention (Load*), even though it'll never really come up
often in Go.

[rsc, 2012-02-06 16:35:25]

On Mon, Feb 6, 2012 at 11:32, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Get would be my suggested color. I've often used that pattern for
> top-level functions because of the same conflict.

Sorry; Java poisoned the Get well.

Russ

[gustavo_niemeyer.net, 2012-02-06 16:36:06]

> I assumed Load (despite being a letter longer) was chosen because the
> signature is different, with an out parameter rather than returning.  I like
> that as a convention (Load*), even though it'll never really come up often
> in Go.

You're absolutely right. In the cases I've used Get it was actually
returning the value (e.g. GetLogger). Load would be more sensible
indeed.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[gustavo_niemeyer.net, 2012-02-06 16:36:58]

> Sorry; Java poisoned the Get well.

Agreed, that's a bad use case for it. Please ignore me.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[remyoudompheng, 2012-02-06 17:16:08]

Hello bradfitz@google.com, rsc@golang.org, bradfitz@golang.org,
gustavo@niemeyer.net (cc: golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-02-06 17:26:44]

Hello bradfitz@google.com, rsc@golang.org, bradfitz@golang.org,
gustavo@niemeyer.net (cc: golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[remyoudompheng, 2012-02-06 17:27:31]

On 2012/02/06 16:25:30, rsc wrote:
> type MemStats
> 
> func ReadMemStats(m *MemStats)
> 
> ?

This looks prettier indeed.

[bradfitz, 2012-02-06 17:31:07]

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go
File src/pkg/runtime/mem.go (right):

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go#newcode11
src/pkg/runtime/mem.go:11: // Not locked during update; approximate.
this comment is old now and could go.

also, the "// General statistics" is in the wrong place.  this probably predates
the godoc convention of putting comments above the type.

A proper comment on this type would be nice.

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go#newcode66
src/pkg/runtime/mem.go:66: // ReadMemStats gathers memory statistics and outputs
the result
Maybe something shorter, like:

// ReadMemStats populates m with memory statistics.

But I'm not the wording expert.

[rsc, 2012-02-06 17:56:38]

This is a definite improvement; thanks.

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go
File src/pkg/runtime/mem.go (right):

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go#newcode11
src/pkg/runtime/mem.go:11: // Not locked during update; approximate.
// General statistics can stay here.  It is just for these uint64s,
like the headings on the blocks below.  There does need to be a doc comment
though.

// A MemStats records statistics about the memory allocator.
type MemStats struct {
    // General statistics.
    Alloc uint64...

http://codereview.appspot.com/5616072/diff/5/src/pkg/runtime/mem.go#newcode66
src/pkg/runtime/mem.go:66: // ReadMemStats gathers memory statistics and outputs
the result
On 2012/02/06 17:31:07, bradfitz wrote:
> Maybe something shorter, like:
> 
> // ReadMemStats populates m with memory statistics.
> 
> But I'm not the wording expert.

sgtm

[remyoudompheng, 2012-02-06 17:59:36]

Hello bradfitz@google.com, rsc@golang.org, bradfitz@golang.org,
gustavo@niemeyer.net (cc: golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[rsc, 2012-02-06 18:00:59]

LGTM

[remyoudompheng, 2012-02-06 18:16:39]

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

runtime: delete UpdateMemStats, replace with ReadMemStats(&stats).

Unexports runtime.MemStats and rename MemStatsType to MemStats.
The new accessor requires passing a pointer to a user-allocated
MemStats structure.

Fixes issue 2572.

R=bradfitz, rsc, bradfitz, gustavo
CC=golang-dev, remy
http://codereview.appspot.com/5616072

[bradfitz, 2012-02-06 18:20:18]

On Mon, Feb 6, 2012 at 8:33 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
> On Mon, Feb 6, 2012 at 8:32 AM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:
>
>> > type MemStats
>> >
>> > func ReadMemStats(m *MemStats)
>>
>> Get would be my suggested color. I've often used that pattern for
>> top-level functions because of the same conflict.
>
>
> I assumed Load (despite being a letter longer) was chosen because the
> signature is different, with an out parameter rather than returning.  I
> like that as a convention (Load*), even though it'll never really come up
> often in Go.
>

s/Load/Read/g.  I can't type and/or think, apparently.