runtime: make map reads multithreaded safe.

Doing grow work on reads is not multithreaded safe.
Changed code to do grow work only on inserts & deletes.

This is a short-term fix, eventually we'll want to do
grow work in parallel to recover the space of the old
table.

Fixes #5120.

R=bradfitz, khr
CC=golang-dev
https://golang.org/cl/8242043

diff --git a/src/cmd/gc/range.c b/src/cmd/gc/range.c
index e80a8c723b464706b8adc06084ddd842733e6dce..8af45b9d2742a72a9e3338b33cc8edf7293f8eb8 100644 (file)
--- a/src/cmd/gc/range.c
+++ b/src/cmd/gc/range.c
@@ -181,9 +181,9 @@ walkrange(Node *n)
        case TMAP:
                th = typ(TARRAY);
                th->type = ptrto(types[TUINT8]);
-               // see ../../pkg/runtime/hashmap.h:/hash_iter
+               // see ../../pkg/runtime/hashmap.c:/hash_iter
                // Size of hash_iter in # of pointers.
-               th->bound = 10;
+               th->bound = 11;
                hit = temp(th);
 
                fn = syslook("mapiterinit", 1);

diff --git a/src/pkg/runtime/hashmap.c b/src/pkg/runtime/hashmap.c
index a2ad1a0812a7da0751ae91b23688bc3a793cc16e..3f26a157bdeeac8662a7bfc341e48e7b3e9c86da 100644 (file)
--- a/src/pkg/runtime/hashmap.c
+++ b/src/pkg/runtime/hashmap.c
@@ -480,7 +480,7 @@ hash_lookup(MapType *t, Hmap *h, byte **keyp)
 {
        void *key;
        uintptr hash;
-       uintptr bucket;
+       uintptr bucket, oldbucket;
        Bucket *b;
        uint8 top;
        uintptr i;
@@ -495,9 +495,15 @@ hash_lookup(MapType *t, Hmap *h, byte **keyp)
        hash = h->hash0;
        t->key->alg->hash(&hash, t->key->size, key);
        bucket = hash & (((uintptr)1 << h->B) - 1);
-       if(h->oldbuckets != nil)
-               grow_work(t, h, bucket);
-       b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+       if(h->oldbuckets != nil) {
+               oldbucket = bucket & (((uintptr)1 << (h->B - 1)) - 1);
+               b = (Bucket*)(h->oldbuckets + oldbucket * h->bucketsize);
+               if(evacuated(b)) {
+                       b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+               }
+       } else {
+               b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+       }
        top = hash >> (sizeof(uintptr)*8 - 8);
        if(top == 0)
                top = 1;
@@ -741,6 +747,7 @@ struct hash_iter
        uintptr bucket;
        struct Bucket *bptr;
        uintptr i;
+       intptr check_bucket;
 };
 
 // iterator state:
@@ -750,6 +757,9 @@ struct hash_iter
 static void
 hash_iter_init(MapType *t, Hmap *h, struct hash_iter *it)
 {
+       if(sizeof(struct hash_iter) / sizeof(uintptr) != 11) {
+               runtime·throw("hash_iter size incorrect"); // see ../../cmd/gc/range.c
+       }
        it->t = t;
        it->h = h;
 
@@ -762,8 +772,8 @@ hash_iter_init(MapType *t, Hmap *h, struct hash_iter *it)
        it->wrapped = false;
        it->bptr = nil;
 
-       // Remember we have an iterator at this level.
-       h->flags |= Iterator;
+       // Remember we have an iterator.
+       h->flags |= Iterator | OldIterator;  // careful: see issue 5120.
 
        if(h->buckets == nil) {
                // Empty map. Force next hash_next to exit without
@@ -779,9 +789,11 @@ hash_next(struct hash_iter *it)
 {
        Hmap *h;
        MapType *t;
-       uintptr bucket;
+       uintptr bucket, oldbucket;
+       uintptr hash;
        Bucket *b;
        uintptr i;
+       intptr check_bucket;
        bool eq;
        byte *k, *v;
        byte *rk, *rv;
@@ -791,6 +803,7 @@ hash_next(struct hash_iter *it)
        bucket = it->bucket;
        b = it->bptr;
        i = it->i;
+       check_bucket = it->check_bucket;
 
 next:
        if(b == nil) {
@@ -802,10 +815,21 @@ next:
                }
                if(h->oldbuckets != nil && it->B == h->B) {
                        // Iterator was started in the middle of a grow, and the grow isn't done yet.
-                       // Make sure the bucket we're about to read is valid.
-                       grow_work(t, h, bucket);
+                       // If the bucket we're looking at hasn't been filled in yet (i.e. the old
+                       // bucket hasn't been evacuated) then we need to iterate through the old
+                       // bucket and only return the ones that will be migrated to this bucket.
+                       oldbucket = bucket & (((uintptr)1 << (it->B - 1)) - 1);
+                       b = (Bucket*)(h->oldbuckets + oldbucket * h->bucketsize);
+                       if(!evacuated(b)) {
+                               check_bucket = bucket;
+                       } else {
+                               b = (Bucket*)(it->buckets + bucket * h->bucketsize);
+                               check_bucket = -1;
+                       }
+               } else {
+                       b = (Bucket*)(it->buckets + bucket * h->bucketsize);
+                       check_bucket = -1;
                }
-               b = (Bucket*)(it->buckets + bucket * h->bucketsize);
                bucket++;
                if(bucket == ((uintptr)1 << it->B)) {
                        bucket = 0;
@@ -817,6 +841,30 @@ next:
        v = b->data + h->keysize * BUCKETSIZE + h->valuesize * i;
        for(; i < BUCKETSIZE; i++, k += h->keysize, v += h->valuesize) {
                if(b->tophash[i] != 0) {
+                       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
+                               // oldbucket has not been evacuated yet.  So we iterate
+                               // through the oldbucket, skipping any keys that will go
+                               // to the other new bucket (each oldbucket expands to two
+                               // buckets during a grow).
+                               t->key->alg->equal(&eq, t->key->size, IK(h, k), IK(h, k));
+                               if(!eq) {
+                                       // Hash is meaningless if k != k (NaNs).  Return all
+                                       // NaNs during the first of the two new buckets.
+                                       if(bucket >= ((uintptr)1 << (it->B - 1))) {
+                                               continue;
+                                       }
+                               } else {
+                                       // If the item in the oldbucket is not destined for
+                                       // the current new bucket in the iteration, skip it.
+                                       hash = h->hash0;
+                                       t->key->alg->hash(&hash, t->key->size, IK(h, k));
+                                       if((hash & (((uintptr)1 << it->B) - 1)) != check_bucket) {
+                                               continue;
+                                       }
+                               }
+                       }
                        if(!evacuated(b)) {
                                // this is the golden data, we can return it.
                                it->key = IK(h, k);
@@ -849,6 +897,7 @@ next:
                        it->bucket = bucket;
                        it->bptr = b;
                        it->i = i + 1;
+                       it->check_bucket = check_bucket;
                        return;
                }
        }

diff --git a/src/pkg/runtime/hashmap_fast.c b/src/pkg/runtime/hashmap_fast.c
index 2169f4c300e25452e3fbb52ba3fa7e573db36533..55914ca7f0b9b9be88bfe8713483882b6e79ae89 100644 (file)
--- a/src/pkg/runtime/hashmap_fast.c
+++ b/src/pkg/runtime/hashmap_fast.c
@@ -17,7 +17,7 @@ void
 HASH_LOOKUP1(MapType *t, Hmap *h, KEYTYPE key, byte *value)
 {
        uintptr hash;
-       uintptr bucket;
+       uintptr bucket, oldbucket;
        Bucket *b;
        uint8 top;
        uintptr i;
@@ -55,9 +55,15 @@ HASH_LOOKUP1(MapType *t, Hmap *h, KEYTYPE key, byte *value)
                hash = h->hash0;
                HASHFUNC(&hash, sizeof(KEYTYPE), &key);
                bucket = hash & (((uintptr)1 << h->B) - 1);
-               if(h->oldbuckets != nil)
-                       grow_work(t, h, bucket);
-               b = (Bucket*)(h->buckets + bucket * (offsetof(Bucket, data[0]) + BUCKETSIZE * sizeof(KEYTYPE) + BUCKETSIZE * h->valuesize));
+               if(h->oldbuckets != nil) {
+                       oldbucket = bucket & (((uintptr)1 << (h->B - 1)) - 1);
+                       b = (Bucket*)(h->oldbuckets + oldbucket * h->bucketsize);
+                       if(evacuated(b)) {
+                               b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+                       }
+               } else {
+                       b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+               }
                top = hash >> (sizeof(uintptr)*8 - 8);
                if(top == 0)
                        top = 1;
@@ -81,7 +87,7 @@ void
 HASH_LOOKUP2(MapType *t, Hmap *h, KEYTYPE key, byte *value, bool res)
 {
        uintptr hash;
-       uintptr bucket;
+       uintptr bucket, oldbucket;
        Bucket *b;
        uint8 top;
        uintptr i;
@@ -123,9 +129,15 @@ HASH_LOOKUP2(MapType *t, Hmap *h, KEYTYPE key, byte *value, bool res)
                hash = h->hash0;
                HASHFUNC(&hash, sizeof(KEYTYPE), &key);
                bucket = hash & (((uintptr)1 << h->B) - 1);
-               if(h->oldbuckets != nil)
-                       grow_work(t, h, bucket);
-               b = (Bucket*)(h->buckets + bucket * (offsetof(Bucket, data[0]) + BUCKETSIZE * sizeof(KEYTYPE) + BUCKETSIZE * h->valuesize));
+               if(h->oldbuckets != nil) {
+                       oldbucket = bucket & (((uintptr)1 << (h->B - 1)) - 1);
+                       b = (Bucket*)(h->oldbuckets + oldbucket * h->bucketsize);
+                       if(evacuated(b)) {
+                               b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+                       }
+               } else {
+                       b = (Bucket*)(h->buckets + bucket * h->bucketsize);
+               }
                top = hash >> (sizeof(uintptr)*8 - 8);
                if(top == 0)
                        top = 1;

diff --git a/src/pkg/runtime/map_test.go b/src/pkg/runtime/map_test.go
index cc8863b2eeb4749c18d22e6c98e73e7a3871dab4..fcce9a4fe6d6effa4bbd26104545cf318a9abe56 100644 (file)
--- a/src/pkg/runtime/map_test.go
+++ b/src/pkg/runtime/map_test.go
@@ -234,9 +234,6 @@ func TestIterGrowWithGC(t *testing.T) {
 }
 
 func TestConcurrentReadsAfterGrowth(t *testing.T) {
-       // TODO(khr): fix and enable this test.
-       t.Skip("Known currently broken; golang.org/issue/5179")
-
        if os.Getenv("GOMAXPROCS") == "" {
                defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(16))
        }