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

2 files changed, 20 insertions, 20 deletions

diff --git a/src/pkg/runtime/hashmap.c b/src/pkg/runtime/hashmap.c
index 101c4281f6..c359e2a14d 100644
--- 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 b8586483fd..9c703ba362 100644
--- 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++ {