runtime: change map iteration randomization to use intra-bucket offset

Map iteration previously started from a random bucket, but walked each
bucket from the beginning. Now, iteration always starts from the first
bucket and walks each bucket starting at a random offset. For
performance, the random offset is selected at the start of iteration
and reused for each bucket.

Iteration over a map with 8 or fewer elements--a single bucket--will
now be non-deterministic. There will now be only 8 different possible
map iterations.

Significant benchmark changes, on my OS X laptop (rough but consistent):

benchmark                              old ns/op     new ns/op     delta
BenchmarkMapIter                       128           121           -5.47%
BenchmarkMapIterEmpty                  4.26          4.45          +4.46%
BenchmarkNewEmptyMap                   114           111           -2.63%

Fixes #6719.

R=khr, bradfitz
CC=golang-codereviews
https://golang.org/cl/47370043

diff --git a/src/cmd/gc/reflect.c b/src/cmd/gc/reflect.c
index 5afcdee48989da77b54147736172274b9eaeea91..f54c4cd39451ee60926ee27efe9a7c761217fb1f 100644 (file)
--- a/src/cmd/gc/reflect.c
+++ b/src/cmd/gc/reflect.c
@@ -252,7 +252,7 @@ hiter(Type *t)
        //    h *Hmap
        //    buckets *Bucket
        //    bptr *Bucket
-       //    other [5]uintptr
+       //    other [4]uintptr
        // }
        // must match ../../pkg/runtime/hashmap.c:hash_iter.
        field[0] = typ(TFIELD);
@@ -289,8 +289,8 @@ hiter(Type *t)
        field[6] = typ(TFIELD);
        field[6]->type = typ(TARRAY);
        field[6]->type->type = types[TUINTPTR];
-       field[6]->type->bound = 5;
-       field[6]->type->width = 5 * widthptr;
+       field[6]->type->bound = 4;
+       field[6]->type->width = 4 * widthptr;
        field[6]->sym = mal(sizeof(Sym));
        field[6]->sym->name = "other";
        
@@ -306,8 +306,8 @@ hiter(Type *t)
        }
        field[6]->down = T;
        off += field[6]->type->width;
-       if(off != 11 * widthptr)
-               yyerror("hash_iter size not correct %d %d", off, 11 * widthptr);
+       if(off != 10 * widthptr)
+               yyerror("hash_iter size not correct %d %d", off, 10 * widthptr);
        t->hiter = i;
        i->map = t;
        return i;

diff --git a/src/pkg/runtime/hashmap.c b/src/pkg/runtime/hashmap.c
index 101c4281f6ed55510b51b567b96d5338f6ce3c44..c359e2a14dcbc123f0cdd7bddcf6d1798a102f23 100644 (file)
--- a/src/pkg/runtime/hashmap.c
+++ b/src/pkg/runtime/hashmap.c
@@ -746,9 +746,8 @@ struct hash_iter
        byte *buckets; // bucket ptr at hash_iter initialization time
        struct Bucket *bptr; // current bucket
 
-       // end point for iteration
-       uintptr endbucket;
-       bool wrapped;
+       uint32 offset; // intra-bucket offset to start from during iteration
+       bool done;
 
        // state of table at time iterator is initialized
        uint8 B;
@@ -768,8 +767,8 @@ hash_iter_init(MapType *t, Hmap *h, struct hash_iter *it)
 {
        uint32 old;
 
-       if(sizeof(struct hash_iter) / sizeof(uintptr) != 11) {
-               runtime·throw("hash_iter size incorrect"); // see ../../cmd/gc/range.c
+       if(sizeof(struct hash_iter) / sizeof(uintptr) != 10) {
+               runtime·throw("hash_iter size incorrect"); // see ../../cmd/gc/reflect.c
        }
        it->t = t;
        it->h = h;
@@ -779,8 +778,9 @@ hash_iter_init(MapType *t, Hmap *h, struct hash_iter *it)
        it->buckets = h->buckets;
 
        // iterator state
-       it->bucket = it->endbucket = runtime·fastrand1() & (((uintptr)1 << h->B) - 1);
-       it->wrapped = false;
+       it->bucket = 0;
+       it->offset = runtime·fastrand1() & (BUCKETSIZE - 1);
+       it->done = false;
        it->bptr = nil;
 
        // Remember we have an iterator.
@@ -795,8 +795,8 @@ hash_iter_init(MapType *t, Hmap *h, struct hash_iter *it)
 
        if(h->buckets == nil) {
                // Empty map. Force next hash_next to exit without
-               // evalulating h->bucket.
-               it->wrapped = true;
+               // evaluating h->bucket.
+               it->done = true;
        }
 }
 
@@ -810,7 +810,7 @@ hash_next(struct hash_iter *it)
        uintptr bucket, oldbucket;
        uintptr hash;
        Bucket *b;
-       uintptr i;
+       uintptr i, offi;
        intptr check_bucket;
        bool eq;
        byte *k, *v;
@@ -825,7 +825,7 @@ hash_next(struct hash_iter *it)
 
 next:
        if(b == nil) {
-               if(bucket == it->endbucket && it->wrapped) {
+               if(it->done) {
                        // end of iteration
                        it->key = nil;
                        it->value = nil;
@@ -851,14 +851,15 @@ next:
                bucket++;
                if(bucket == ((uintptr)1 << it->B)) {
                        bucket = 0;
-                       it->wrapped = true;
+                       it->done = true;
                }
                i = 0;
        }
-       k = b->data + h->keysize * i;
-       v = b->data + h->keysize * BUCKETSIZE + h->valuesize * i;
-       for(; i < BUCKETSIZE; i++, k += h->keysize, v += h->valuesize) {
-               if(b->tophash[i] != Empty && b->tophash[i] != EvacuatedEmpty) {
+       for(; i < BUCKETSIZE; i++) {
+               offi = (i + it->offset) & (BUCKETSIZE - 1);
+               k = b->data + h->keysize * offi;
+               v = b->data + h->keysize * BUCKETSIZE + h->valuesize * offi;
+               if(b->tophash[offi] != Empty && b->tophash[offi] != EvacuatedEmpty) {
                        if(check_bucket >= 0) {
                                // Special case: iterator was started during a grow and the
                                // grow is not done yet.  We're working on a bucket whose
@@ -884,12 +885,12 @@ next:
                                        // NOTE: this case is why we need two evacuate tophash
                                        // values, evacuatedX and evacuatedY, that differ in
                                        // their low bit.
-                                       if(check_bucket >> (it->B - 1) != (b->tophash[i] & 1)) {
+                                       if(check_bucket >> (it->B - 1) != (b->tophash[offi] & 1)) {
                                                continue;
                                        }
                                }
                        }
-                       if(b->tophash[i] != EvacuatedX && b->tophash[i] != EvacuatedY) {
+                       if(b->tophash[offi] != EvacuatedX && b->tophash[offi] != EvacuatedY) {
                                // this is the golden data, we can return it.
                                it->key = IK(h, k);
                                it->value = IV(h, v);

diff --git a/src/pkg/runtime/map_test.go b/src/pkg/runtime/map_test.go
index b8586483fdf8e38301663de065d66978cfed79e1..9c703ba362e99eab738a668102175467e9ed350f 100644 (file)
--- a/src/pkg/runtime/map_test.go
+++ b/src/pkg/runtime/map_test.go
@@ -411,8 +411,7 @@ func TestMapNanGrowIterator(t *testing.T) {
 }
 
 func TestMapIterOrder(t *testing.T) {
-       // TODO: For issue 6719, add 3 and 7 to this list.
-       for _, n := range [...]int{9, 15} {
+       for _, n := range [...]int{3, 7, 9, 15} {
                // Make m be {0: true, 1: true, ..., n-1: true}.
                m := make(map[int]bool)
                for i := 0; i < n; i++ {