sync: add new Map method LoadAndDelete

This CL implements a LoadAndDelete method in sync.Map. Benchmark:

name                                              time/op
LoadAndDeleteBalanced/*sync_test.RWMutexMap-12    98.8ns ± 1%
LoadAndDeleteBalanced/*sync.Map-12                10.3ns ±11%
LoadAndDeleteUnique/*sync_test.RWMutexMap-12      99.2ns ± 2%
LoadAndDeleteUnique/*sync.Map-12                  6.63ns ±10%
LoadAndDeleteCollision/*sync_test.DeepCopyMap-12   140ns ± 0%
LoadAndDeleteCollision/*sync_test.RWMutexMap-12   75.2ns ± 2%
LoadAndDeleteCollision/*sync.Map-12               5.21ns ± 5%

In addition, Delete is bounded and more efficient if many collisions:

DeleteCollision/*sync_test.DeepCopyMap-12   120ns ± 2%   125ns ± 1%   +3.80%  (p=0.000 n=10+9)
DeleteCollision/*sync_test.RWMutexMap-12   73.5ns ± 3%  79.5ns ± 1%   +8.03%  (p=0.000 n=10+9)
DeleteCollision/*sync.Map-12               97.8ns ± 3%   5.9ns ± 4%  -94.00%  (p=0.000 n=10+10)

Fixes #33762

Change-Id: Ic8469a7861d27ab0edeface0078aad8af9b26c2f
Reviewed-on: https://go-review.googlesource.com/c/go/+/205899
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/api/next.txt b/api/next.txt
index cab86a9904184fdb4b022bbbbe627ee8b8ce13f3..442c29a4164245d91bdd69f7823c96db89210275 100644 (file)
--- a/api/next.txt
+++ b/api/next.txt
@@ -1,2 +1,3 @@
 pkg testing, method (*T) Deadline() (time.Time, bool)
 pkg time, method (*Ticker) Reset(Duration)
+pkg sync, method (*Map) LoadAndDelete(interface{}) (interface{}, bool)

diff --git a/doc/go1.15.html b/doc/go1.15.html
index ed240d85ccae33925ff421976db0ac7f2da7ab3f..5b0459e67a291c08bd99caeea990fd1b4c6d2503 100644 (file)
--- a/doc/go1.15.html
+++ b/doc/go1.15.html
@@ -81,6 +81,20 @@ TODO
 TODO
 </p>
 
+<dl id="sync"><dt><a href="/pkg/sync/">sync</a></dt>
+  <dd>
+    <p><!-- golang.org/issue/33762 -->
+      The new method
+      <a href="/pkg/sync#Map.LoadAndDelete"><code>Map.LoadAndDelete</code></a>
+      atomically deletes a key and returns the previous value if present.
+    </p>
+    <p><!-- CL 205899 -->
+      The method
+      <a href="/pkg/sync#Map.Delete"><code>Map.Delete</code></a>
+      is more efficient.
+    </p>
+</dl><!-- sync -->
+
 <dl id="time"><dt><a href="/pkg/time/">time</a></dt>
   <dd>
     <p><!-- golang.org/issue/33184 -->

diff --git a/src/sync/map.go b/src/sync/map.go
index c6aa308856584abd2e1be8d47fa0c64ab926b5a6..a61e2ebdd67f72c07773eff50946dad1db6cbe74 100644 (file)
--- a/src/sync/map.go
+++ b/src/sync/map.go
@@ -263,8 +263,9 @@ func (e *entry) tryLoadOrStore(i interface{}) (actual interface{}, loaded, ok bo
        }
 }
 
-// Delete deletes the value for a key.
-func (m *Map) Delete(key interface{}) {
+// LoadAndDelete deletes the value for a key, returning the previous value if any.
+// The loaded result reports whether the key was present.
+func (m *Map) LoadAndDelete(key interface{}) (value interface{}, loaded bool) {
        read, _ := m.read.Load().(readOnly)
        e, ok := read.m[key]
        if !ok && read.amended {
@@ -272,23 +273,33 @@ func (m *Map) Delete(key interface{}) {
                read, _ = m.read.Load().(readOnly)
                e, ok = read.m[key]
                if !ok && read.amended {
-                       delete(m.dirty, key)
+                       e, ok = m.dirty[key]
+                       // Regardless of whether the entry was present, record a miss: this key
+                       // will take the slow path until the dirty map is promoted to the read
+                       // map.
+                       m.missLocked()
                }
                m.mu.Unlock()
        }
        if ok {
-               e.delete()
+               return e.delete()
        }
+       return nil, false
 }
 
-func (e *entry) delete() (hadValue bool) {
+// Delete deletes the value for a key.
+func (m *Map) Delete(key interface{}) {
+       m.LoadAndDelete(key)
+}
+
+func (e *entry) delete() (value interface{}, ok bool) {
        for {
                p := atomic.LoadPointer(&e.p)
                if p == nil || p == expunged {
-                       return false
+                       return nil, false
                }
                if atomic.CompareAndSwapPointer(&e.p, p, nil) {
-                       return true
+                       return *(*interface{})(p), true
                }
        }
 }

diff --git a/src/sync/map_bench_test.go b/src/sync/map_bench_test.go
index e6a8badddbafca3a9369dcded986b3a84ac5aa3c..cf0a3d7fdedc898caf689f73b17450a2c0f1fb97 100644 (file)
--- a/src/sync/map_bench_test.go
+++ b/src/sync/map_bench_test.go
@@ -144,6 +144,66 @@ func BenchmarkLoadOrStoreCollision(b *testing.B) {
        })
 }
 
+func BenchmarkLoadAndDeleteBalanced(b *testing.B) {
+       const hits, misses = 128, 128
+
+       benchMap(b, bench{
+               setup: func(b *testing.B, m mapInterface) {
+                       if _, ok := m.(*DeepCopyMap); ok {
+                               b.Skip("DeepCopyMap has quadratic running time.")
+                       }
+                       for i := 0; i < hits; i++ {
+                               m.LoadOrStore(i, i)
+                       }
+                       // Prime the map to get it into a steady state.
+                       for i := 0; i < hits*2; i++ {
+                               m.Load(i % hits)
+                       }
+               },
+
+               perG: func(b *testing.B, pb *testing.PB, i int, m mapInterface) {
+                       for ; pb.Next(); i++ {
+                               j := i % (hits + misses)
+                               if j < hits {
+                                       m.LoadAndDelete(j)
+                               } else {
+                                       m.LoadAndDelete(i)
+                               }
+                       }
+               },
+       })
+}
+
+func BenchmarkLoadAndDeleteUnique(b *testing.B) {
+       benchMap(b, bench{
+               setup: func(b *testing.B, m mapInterface) {
+                       if _, ok := m.(*DeepCopyMap); ok {
+                               b.Skip("DeepCopyMap has quadratic running time.")
+                       }
+               },
+
+               perG: func(b *testing.B, pb *testing.PB, i int, m mapInterface) {
+                       for ; pb.Next(); i++ {
+                               m.LoadAndDelete(i)
+                       }
+               },
+       })
+}
+
+func BenchmarkLoadAndDeleteCollision(b *testing.B) {
+       benchMap(b, bench{
+               setup: func(_ *testing.B, m mapInterface) {
+                       m.LoadOrStore(0, 0)
+               },
+
+               perG: func(b *testing.B, pb *testing.PB, i int, m mapInterface) {
+                       for ; pb.Next(); i++ {
+                               m.LoadAndDelete(0)
+                       }
+               },
+       })
+}
+
 func BenchmarkRange(b *testing.B) {
        const mapSize = 1 << 10
 
@@ -213,3 +273,17 @@ func BenchmarkAdversarialDelete(b *testing.B) {
                },
        })
 }
+
+func BenchmarkDeleteCollision(b *testing.B) {
+       benchMap(b, bench{
+               setup: func(_ *testing.B, m mapInterface) {
+                       m.LoadOrStore(0, 0)
+               },
+
+               perG: func(b *testing.B, pb *testing.PB, i int, m mapInterface) {
+                       for ; pb.Next(); i++ {
+                               m.Delete(0)
+                       }
+               },
+       })
+}

diff --git a/src/sync/map_reference_test.go b/src/sync/map_reference_test.go
index 9f27b07c3292ba3dfa22e7039dc9882a1b80706c..d105a24e926e3ccf3370f659455748a8c23a0cc9 100644 (file)
--- a/src/sync/map_reference_test.go
+++ b/src/sync/map_reference_test.go
@@ -16,6 +16,7 @@ type mapInterface interface {
        Load(interface{}) (interface{}, bool)
        Store(key, value interface{})
        LoadOrStore(key, value interface{}) (actual interface{}, loaded bool)
+       LoadAndDelete(key interface{}) (value interface{}, loaded bool)
        Delete(interface{})
        Range(func(key, value interface{}) (shouldContinue bool))
 }
@@ -56,6 +57,18 @@ func (m *RWMutexMap) LoadOrStore(key, value interface{}) (actual interface{}, lo
        return actual, loaded
 }
 
+func (m *RWMutexMap) LoadAndDelete(key interface{}) (value interface{}, loaded bool) {
+       m.mu.Lock()
+       value, loaded = m.dirty[key]
+       if !loaded {
+               m.mu.Unlock()
+               return nil, false
+       }
+       delete(m.dirty, key)
+       m.mu.Unlock()
+       return value, loaded
+}
+
 func (m *RWMutexMap) Delete(key interface{}) {
        m.mu.Lock()
        delete(m.dirty, key)
@@ -124,6 +137,16 @@ func (m *DeepCopyMap) LoadOrStore(key, value interface{}) (actual interface{}, l
        return actual, loaded
 }
 
+func (m *DeepCopyMap) LoadAndDelete(key interface{}) (value interface{}, loaded bool) {
+       m.mu.Lock()
+       dirty := m.dirty()
+       value, loaded = dirty[key]
+       delete(dirty, key)
+       m.clean.Store(dirty)
+       m.mu.Unlock()
+       return
+}
+
 func (m *DeepCopyMap) Delete(key interface{}) {
        m.mu.Lock()
        dirty := m.dirty()

diff --git a/src/sync/map_test.go b/src/sync/map_test.go
index b60a1c7bedee82b02f1c09789b9381ed505b6bb9..4ae989a6d5d8175108f3119888f70888ec252290 100644 (file)
--- a/src/sync/map_test.go
+++ b/src/sync/map_test.go
@@ -16,13 +16,14 @@ import (
 type mapOp string
 
 const (
-       opLoad        = mapOp("Load")
-       opStore       = mapOp("Store")
-       opLoadOrStore = mapOp("LoadOrStore")
-       opDelete      = mapOp("Delete")
+       opLoad          = mapOp("Load")
+       opStore         = mapOp("Store")
+       opLoadOrStore   = mapOp("LoadOrStore")
+       opLoadAndDelete = mapOp("LoadAndDelete")
+       opDelete        = mapOp("Delete")
 )
 
-var mapOps = [...]mapOp{opLoad, opStore, opLoadOrStore, opDelete}
+var mapOps = [...]mapOp{opLoad, opStore, opLoadOrStore, opLoadAndDelete, opDelete}
 
 // mapCall is a quick.Generator for calls on mapInterface.
 type mapCall struct {
@@ -39,6 +40,8 @@ func (c mapCall) apply(m mapInterface) (interface{}, bool) {
                return nil, false
        case opLoadOrStore:
                return m.LoadOrStore(c.k, c.v)
+       case opLoadAndDelete:
+               return m.LoadAndDelete(c.k)
        case opDelete:
                m.Delete(c.k)
                return nil, false