From 051d2f47e5a9475d00746a77e3bc16db50d78948 Mon Sep 17 00:00:00 2001
From: Josh Bleecher Snyder <josh@tailscale.com>
Date: Mon, 24 May 2021 14:24:39 -0700
Subject: [PATCH] internal/deephash: re-use MapIter
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

name              old time/op    new time/op    delta
Hash-8              12.4µs ± 0%    12.4µs ± 0%    -0.33%  (p=0.002 n=10+9)
HashMapAcyclic-8    21.2µs ± 0%    21.3µs ± 0%    +0.45%  (p=0.000 n=8+8)

name              old alloc/op   new alloc/op   delta
Hash-8                793B ± 0%      408B ± 0%   -48.55%  (p=0.000 n=10+10)
HashMapAcyclic-8      128B ± 0%        0B       -100.00%  (p=0.000 n=10+10)

name              old allocs/op  new allocs/op  delta
Hash-8                9.00 ± 0%      6.00 ± 0%   -33.33%  (p=0.000 n=10+10)
HashMapAcyclic-8      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)

Depends on https://github.com/golang/go/issues/46293.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
---
 internal/deephash/deephash.go       |  5 ++++-
 internal/deephash/mapiter.go        | 12 ++++++++++++
 internal/deephash/mapiter_future.go | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/internal/deephash/deephash.go b/internal/deephash/deephash.go
index f7dafac4f..f3e596144 100644
--- a/internal/deephash/deephash.go
+++ b/internal/deephash/deephash.go
@@ -197,6 +197,7 @@ type mapHasher struct {
 	s256 hash.Hash         // sha256 hash.Hash
 	bw   *bufio.Writer     // to hasher into ebuf
 	val  valueCache        // re-usable values for map iteration
+	iter *reflect.MapIter  // re-usable map iterator
 }
 
 func (mh *mapHasher) Reset() {
@@ -226,6 +227,7 @@ var mapHasherPool = &sync.Pool{
 		mh.s256 = sha256.New()
 		mh.bw = bufio.NewWriter(mh.s256)
 		mh.val = make(valueCache)
+		mh.iter = new(reflect.MapIter)
 		return mh
 	},
 }
@@ -248,7 +250,8 @@ func hashMapAcyclic(w *bufio.Writer, v reflect.Value, visited map[uintptr]bool,
 	mh := mapHasherPool.Get().(*mapHasher)
 	defer mapHasherPool.Put(mh)
 	mh.Reset()
-	iter := v.MapRange()
+	iter := mapIter(mh.iter, v)
+	defer mapIter(mh.iter, reflect.Value{}) // avoid pinning v from mh.iter when we return
 	k := mh.val.get(v.Type().Key())
 	e := mh.val.get(v.Type().Elem())
 	for iter.Next() {
diff --git a/internal/deephash/mapiter.go b/internal/deephash/mapiter.go
index a47eb76b4..01a3ed1e5 100644
--- a/internal/deephash/mapiter.go
+++ b/internal/deephash/mapiter.go
@@ -23,3 +23,15 @@ func iterKey(iter *reflect.MapIter, _ reflect.Value) reflect.Value {
 func iterVal(iter *reflect.MapIter, _ reflect.Value) reflect.Value {
 	return iter.Value()
 }
+
+// mapIter returns a map iterator for mapVal.
+// scratch is a re-usable reflect.MapIter.
+// mapIter may re-use scratch and return it,
+// or it may allocate and return a new *reflect.MapIter.
+// If mapVal is the zero reflect.Value, mapIter may return nil.
+func mapIter(_ *reflect.MapIter, mapVal reflect.Value) *reflect.MapIter {
+	if !mapVal.IsValid() {
+		return nil
+	}
+	return mapVal.MapRange()
+}
diff --git a/internal/deephash/mapiter_future.go b/internal/deephash/mapiter_future.go
index a92fbc75f..d80221d11 100644
--- a/internal/deephash/mapiter_future.go
+++ b/internal/deephash/mapiter_future.go
@@ -25,3 +25,18 @@ func iterVal(iter *reflect.MapIter, scratch reflect.Value) reflect.Value {
 	iter.SetValue(scratch)
 	return scratch
 }
+
+// mapIter returns a map iterator for mapVal.
+// scratch is a re-usable reflect.MapIter.
+// mapIter may re-use scratch and return it,
+// or it may allocate and return a new *reflect.MapIter.
+// If mapVal is the zero reflect.Value, mapIter may return nil.
+func mapIter(scratch *reflect.MapIter, mapVal reflect.Value) *reflect.MapIter {
+	scratch.Reset(mapVal) // always Reset, to allow the caller to avoid pinning memory
+	if !mapVal.IsValid() {
+		// Returning scratch would also be OK.
+		// Do this for consistency with the non-optimized version.
+		return nil
+	}
+	return scratch
+}