code review 12034047: runtime: cut struct Hmap back to 48-byte allocation

runtime: cut struct Hmap back to 48-byte allocation

struct Hmap is the header for a map value.

CL 8377046 made flags a uint32 so that it could be updated atomically,
but that bumped the struct to 56 bytes, which allocates as 64 bytes (on amd64).

hash0 is initialized from runtime.fastrand1, which returns a uint32,
so the top 32 bits were always zero anyway. Declare it as a uint32
to reclaim 4 bytes and bring the Hmap size back down to a 48-byte allocation.

Fixes issue 5237.

[rsc, 2013-07-30 19:13:32]

Hello golang-dev@googlegroups.com, khr (cc: bradfitz, dvyukov,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[khr1, 2013-07-30 19:20:11]

LGTM.


On Tue, Jul 30, 2013 at 12:13 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev1, khr,
>
> Message:
> Hello golang-dev@googlegroups.com, khr (cc: bradfitz, dvyukov,
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: cut struct Hmap back to 48-byte allocation
>
> struct Hmap is the header for a map value.
>
> CL 8377046 made flags a uint32 so that it could be updated atomically,
> but that bumped the struct to 56 bytes, which allocates as 64 bytes (on
> amd64).
>
> hash0 is initialized from runtime.fastrand1, which returns a uint32,
> so the top 32 bits were always zero anyway. Declare it as a uint32
> to reclaim 4 bytes and bring the Hmap size back down to a 48-byte
> allocation.
>
> Fixes issue 5237.
>
> Please review this at
https://codereview.appspot.**com/12034047/<https://codereview.appspot.com/120...
>
> Affected files:
>   M src/pkg/runtime/hashmap.c
>
>
> Index: src/pkg/runtime/hashmap.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/hashmap.c
> +++ b/src/pkg/runtime/hashmap.c
> @@ -96,12 +96,12 @@
>  {
>         uintgo  count;        // # live cells == size of map.  Must be
> first (used by len() builtin)
>         uint32  flags;
> +       uint32 hash0;        // hash seed
>         uint8   B;            // log_2 of # of buckets (can hold up to
> LOAD * 2^B items)
>         uint8   keysize;      // key size in bytes
>         uint8   valuesize;    // value size in bytes
>         uint16  bucketsize;   // bucket size in bytes
>
> -       uintptr hash0;        // hash seed
>         byte    *buckets;     // array of 2^B Buckets. may be nil if
> count==0.
>         byte    *oldbuckets;  // previous bucket array of half the size,
> non-nil only when growing
>         uintptr nevacuate;    // progress counter for evacuation (buckets
> less than this have been evacuated)
>
>
>

[dvyukov, 2013-07-30 19:34:33]

LGTM

[bradfitz, 2013-07-30 19:41:56]

Also include something like: ?

diff -r f04c9f103e15 src/pkg/runtime/map_test.go
--- a/src/pkg/runtime/map_test.go Tue Jul 30 19:47:16 2013 +0400
+++ b/src/pkg/runtime/map_test.go Tue Jul 30 12:41:24 2013 -0700
@@ -15,6 +15,45 @@
  "testing"
 )

+func TestMapSize(t *testing.T) {
+ var m map[struct{}]struct{}
+ size := bytesPerRun(100, func() {
+ m = make(map[struct{}]struct{})
+ })
+ if size > 48 {
+ t.Errorf("size = %v; want <= 48", size)
+ }
+}
+
+// like testing.AllocsPerRun, but for bytes of memory, not number of
allocations.
+func bytesPerRun(runs int, f func()) (avg float64) {
+ defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1))
+
+ // Warm up the function
+ f()
+
+ // Measure the starting statistics
+ var memstats runtime.MemStats
+ runtime.ReadMemStats(&memstats)
+ sum := 0 - memstats.Alloc
+
+ // Run the function the specified number of times
+ for i := 0; i < runs; i++ {
+ f()
+ }
+
+ // Read the final statistics
+ runtime.ReadMemStats(&memstats)
+ sum += memstats.Alloc
+
+ // Average the mallocs over the runs (not counting the warm-up).
+ // We are forced to return a float64 because the API is silly, but do
+ // the division as integers so we can ask if AllocsPerRun()==1
+ // instead of AllocsPerRun()<2.
+ return float64(sum / uint64(runs))
+}
+
+
 // negative zero is a good test because:
 //  1) 0 and -0 are equal, yet have distinct representations.
 //  2) 0 is represented as all zeros, -0 isn't.



On Tue, Jul 30, 2013 at 12:34 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> LGTM
>

[rsc, 2013-07-31 02:48:07]

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

runtime: cut struct Hmap back to 48-byte allocation

struct Hmap is the header for a map value.

CL 8377046 made flags a uint32 so that it could be updated atomically,
but that bumped the struct to 56 bytes, which allocates as 64 bytes (on amd64).

hash0 is initialized from runtime.fastrand1, which returns a uint32,
so the top 32 bits were always zero anyway. Declare it as a uint32
to reclaim 4 bytes and bring the Hmap size back down to a 48-byte allocation.

Fixes issue 5237.

R=golang-dev, khr, khr
CC=bradfitz, dvyukov, golang-dev
https://codereview.appspot.com/12034047