cmd/compile: map delete should clear value always

Map delete must clear value every time because
newly added map optimizations of compound-assignment
operators (CL #91557) rely on this behavior of map delete.

It slows down map delete operation for non-reference types:

name                   old time/op    new time/op    delta
MapDelete/Int32/100      23.9ns ± 2%    27.8ns ± 4%  +16.04%  (p=0.000 n=20+20)
MapDelete/Int32/1000     21.5ns ± 2%    25.2ns ± 2%  +17.06%  (p=0.000 n=20+19)
MapDelete/Int32/10000    24.2ns ± 6%    27.2ns ± 5%  +12.39%  (p=0.000 n=19+19)
MapDelete/Int64/100      24.2ns ± 4%    27.7ns ± 2%  +14.55%  (p=0.000 n=20+20)
MapDelete/Int64/1000     22.1ns ± 2%    24.8ns ± 2%  +12.36%  (p=0.000 n=10+20)

Fixes #25936

Change-Id: I8499b790cb5bb019938161b3e50f3243d9bbb79c
Reviewed-on: https://go-review.googlesource.com/120255
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/runtime/map.go b/src/runtime/map.go
index cc1358a97738bf914ba3af92f9e0566bc7a001f4..0e00f12974d7ba63e8097df0af403023489064c2 100644 (file)
--- a/src/runtime/map.go
+++ b/src/runtime/map.go
@@ -707,14 +707,13 @@ search:
                        } else if t.key.kind&kindNoPointers == 0 {
                                memclrHasPointers(k, t.key.size)
                        }
-                       // Only clear value if there are pointers in it.
-                       if t.indirectvalue || t.elem.kind&kindNoPointers == 0 {
-                               v := add(unsafe.Pointer(b), dataOffset+bucketCnt*uintptr(t.keysize)+i*uintptr(t.valuesize))
-                               if t.indirectvalue {
-                                       *(*unsafe.Pointer)(v) = nil
-                               } else {
-                                       memclrHasPointers(v, t.elem.size)
-                               }
+                       v := add(unsafe.Pointer(b), dataOffset+bucketCnt*uintptr(t.keysize)+i*uintptr(t.valuesize))
+                       if t.indirectvalue {
+                               *(*unsafe.Pointer)(v) = nil
+                       } else if t.elem.kind&kindNoPointers == 0 {
+                               memclrHasPointers(v, t.elem.size)
+                       } else {
+                               memclrNoHeapPointers(v, t.elem.size)
                        }
                        b.tophash[i] = empty
                        h.count--

diff --git a/src/runtime/map_fast32.go b/src/runtime/map_fast32.go
index 296569772bff7154a42dba1ea168ce5b874f951c..bf0b23604bb0c496921a355d6c3bf9e540a613a7 100644 (file)
--- a/src/runtime/map_fast32.go
+++ b/src/runtime/map_fast32.go
@@ -293,10 +293,11 @@ search:
                        if t.key.kind&kindNoPointers == 0 {
                                memclrHasPointers(k, t.key.size)
                        }
-                       // Only clear value if there are pointers in it.
+                       v := add(unsafe.Pointer(b), dataOffset+bucketCnt*4+i*uintptr(t.valuesize))
                        if t.elem.kind&kindNoPointers == 0 {
-                               v := add(unsafe.Pointer(b), dataOffset+bucketCnt*4+i*uintptr(t.valuesize))
                                memclrHasPointers(v, t.elem.size)
+                       } else {
+                               memclrNoHeapPointers(v, t.elem.size)
                        }
                        b.tophash[i] = empty
                        h.count--

diff --git a/src/runtime/map_fast64.go b/src/runtime/map_fast64.go
index aa3eff8ac8f9d19e500092a59c9083f6ea3d23cf..4bde9e2be0726e92c6981cf5e6f58e88c09a3dbe 100644 (file)
--- a/src/runtime/map_fast64.go
+++ b/src/runtime/map_fast64.go
@@ -293,10 +293,11 @@ search:
                        if t.key.kind&kindNoPointers == 0 {
                                memclrHasPointers(k, t.key.size)
                        }
-                       // Only clear value if there are pointers in it.
+                       v := add(unsafe.Pointer(b), dataOffset+bucketCnt*8+i*uintptr(t.valuesize))
                        if t.elem.kind&kindNoPointers == 0 {
-                               v := add(unsafe.Pointer(b), dataOffset+bucketCnt*8+i*uintptr(t.valuesize))
                                memclrHasPointers(v, t.elem.size)
+                       } else {
+                               memclrNoHeapPointers(v, t.elem.size)
                        }
                        b.tophash[i] = empty
                        h.count--

diff --git a/src/runtime/map_faststr.go b/src/runtime/map_faststr.go
index fa21dcae7ef69cfca09edbe9286e275411ee584b..415bbff143ff53bb4250226635a3bea0cee0c3bf 100644 (file)
--- a/src/runtime/map_faststr.go
+++ b/src/runtime/map_faststr.go
@@ -314,10 +314,11 @@ search:
                        }
                        // Clear key's pointer.
                        k.str = nil
-                       // Only clear value if there are pointers in it.
+                       v := add(unsafe.Pointer(b), dataOffset+bucketCnt*2*sys.PtrSize+i*uintptr(t.valuesize))
                        if t.elem.kind&kindNoPointers == 0 {
-                               v := add(unsafe.Pointer(b), dataOffset+bucketCnt*2*sys.PtrSize+i*uintptr(t.valuesize))
                                memclrHasPointers(v, t.elem.size)
+                       } else {
+                               memclrNoHeapPointers(v, t.elem.size)
                        }
                        b.tophash[i] = empty
                        h.count--

diff --git a/src/runtime/map_test.go b/src/runtime/map_test.go
index 0f20c84e77c51c997ac9473e29e26b6982de3c3c..4713ce25ec533bcc1c80c147ea017c7db330c6e7 100644 (file)
--- a/src/runtime/map_test.go
+++ b/src/runtime/map_test.go
@@ -435,11 +435,11 @@ func TestEmptyKeyAndValue(t *testing.T) {
 // ("quick keys") as well as long keys.
 func TestSingleBucketMapStringKeys_DupLen(t *testing.T) {
        testMapLookups(t, map[string]string{
-               "x":                      "x1val",
-               "xx":                     "x2val",
-               "foo":                    "fooval",
-               "bar":                    "barval", // same key length as "foo"
-               "xxxx":                   "x4val",
+               "x":    "x1val",
+               "xx":   "x2val",
+               "foo":  "fooval",
+               "bar":  "barval", // same key length as "foo"
+               "xxxx": "x4val",
                strings.Repeat("x", 128): "longval1",
                strings.Repeat("y", 128): "longval2",
        })
@@ -1045,3 +1045,89 @@ func TestDeferDeleteSlow(t *testing.T) {
                t.Errorf("want 0 elements, got %d", len(m))
        }
 }
+
+// TestIncrementAfterDeleteValueInt and other test Issue 25936.
+// Value types int, int32, int64 are affected. Value type string
+// works as expected.
+func TestIncrementAfterDeleteValueInt(t *testing.T) {
+       const key1 = 12
+       const key2 = 13
+
+       m := make(map[int]int)
+       m[key1] = 99
+       delete(m, key1)
+       m[key2]++
+       if n2 := m[key2]; n2 != 1 {
+               t.Errorf("incremented 0 to %d", n2)
+       }
+}
+
+func TestIncrementAfterDeleteValueInt32(t *testing.T) {
+       const key1 = 12
+       const key2 = 13
+
+       m := make(map[int]int32)
+       m[key1] = 99
+       delete(m, key1)
+       m[key2]++
+       if n2 := m[key2]; n2 != 1 {
+               t.Errorf("incremented 0 to %d", n2)
+       }
+}
+
+func TestIncrementAfterDeleteValueInt64(t *testing.T) {
+       const key1 = 12
+       const key2 = 13
+
+       m := make(map[int]int64)
+       m[key1] = 99
+       delete(m, key1)
+       m[key2]++
+       if n2 := m[key2]; n2 != 1 {
+               t.Errorf("incremented 0 to %d", n2)
+       }
+}
+
+func TestIncrementAfterDeleteKeyStringValueInt(t *testing.T) {
+       const key1 = ""
+       const key2 = "x"
+
+       m := make(map[string]int)
+       m[key1] = 99
+       delete(m, key1)
+       m[key2] += 1
+       if n2 := m[key2]; n2 != 1 {
+               t.Errorf("incremented 0 to %d", n2)
+       }
+}
+
+func TestIncrementAfterDeleteKeyValueString(t *testing.T) {
+       const key1 = ""
+       const key2 = "x"
+
+       m := make(map[string]string)
+       m[key1] = "99"
+       delete(m, key1)
+       m[key2] += "1"
+       if n2 := m[key2]; n2 != "1" {
+               t.Errorf("appended '1' to empty (nil) string, got %s", n2)
+       }
+}
+
+// TestIncrementAfterBulkClearKeyStringValueInt tests that map bulk
+// deletion (mapclear) still works as expected. Note that it was not
+// affected by Issue 25936.
+func TestIncrementAfterBulkClearKeyStringValueInt(t *testing.T) {
+       const key1 = ""
+       const key2 = "x"
+
+       m := make(map[string]int)
+       m[key1] = 99
+       for k := range m {
+               delete(m, k)
+       }
+       m[key2]++
+       if n2 := m[key2]; n2 != 1 {
+               t.Errorf("incremented 0 to %d", n2)
+       }
+}