Repository: libp2p/go-libp2p
Pull request #3093 (2024-12-08 08:50:31)

peerstore: remove sync.Pool for expiringAddrs
depends on #3092 

My intuition here was way off. The method spends most of the time either doing multiaddr ops or manipulating the heap(see the flamegraph). We don't need the sync.Pool. There's only a 5% difference without the pool. 


```
goos: linux
goarch: amd64
pkg: github.com/libp2p/go-libp2p/p2p/host/peerstore/pstoremem
cpu: AMD Ryzen 7 7840U w/ Radeon  780M Graphics     
                                          │   old.txt   │              new.txt               │
                                          │   sec/op    │   sec/op     vs base               │
InMemoryPeerstore/GetAndClearAddrs-1-16     8.351µ ± 2%   8.711µ ± 6%  +4.31% (p=0.019 n=10)
InMemoryPeerstore/GetAndClearAddrs-10-16    49.04µ ± 1%   49.75µ ± 1%  +1.45% (p=0.000 n=10)
InMemoryPeerstore/GetAndClearAddrs-100-16   462.6µ ± 1%   495.5µ ± 3%  +7.10% (p=0.000 n=10)
geomean                                     57.44µ        59.88µ       +4.26%
```

![addrbook-bm](https://github.com/user-attachments/assets/7a3f1345-4fe7-414d-b3cc-e751ca96a404)



Patch:
diff --git a/p2p/host/peerstore/pstoremem/addr_book.go b/p2p/host/peerstore/pstoremem/addr_book.go
index 2520f6ef..32758f2a 100644
--- a/p2p/host/peerstore/pstoremem/addr_book.go
+++ b/p2p/host/peerstore/pstoremem/addr_book.go
@@ -42,22 +42,6 @@ func ttlIsConnected(ttl time.Duration) bool {
 	return ttl >= peerstore.ConnectedAddrTTL
 }
 
-var expiringAddrPool = sync.Pool{New: func() any { return &expiringAddr{} }}
-
-func getExpiringAddrs() *expiringAddr {
-	a := expiringAddrPool.Get().(*expiringAddr)
-	a.heapIndex = -1
-	return a
-}
-
-func putExpiringAddrs(ea *expiringAddr) {
-	if ea == nil {
-		return
-	}
-	*ea = expiringAddr{}
-	expiringAddrPool.Put(ea)
-}
-
 type peerRecordState struct {
 	Envelope *record.Envelope
 	Seq      uint64
@@ -280,7 +264,6 @@ func (mab *memoryAddrBook) gc() {
 		if !ok {
 			return
 		}
-		putExpiringAddrs(ea)
 		mab.maybeDeleteSignedPeerRecordUnlocked(ea.Peer)
 	}
 }
@@ -382,8 +365,7 @@ func (mab *memoryAddrBook) addAddrsUnlocked(p peer.ID, addrs []ma.Multiaddr, ttl
 		a, found := mab.addrs.FindAddr(p, addr)
 		if !found {
 			// not found, announce it.
-			entry := getExpiringAddrs()
-			*entry = expiringAddr{Addr: addr, Expiry: exp, TTL: ttl, Peer: p}
+			entry := &expiringAddr{Addr: addr, Expiry: exp, TTL: ttl, Peer: p}
 			mab.addrs.Insert(entry)
 			mab.subManager.BroadcastAddr(p, addr)
 		} else {
@@ -433,7 +415,6 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du
 			if ttl > 0 {
 				if a.IsConnected() && !ttlIsConnected(ttl) && mab.addrs.NumUnconnectedAddrs() >= mab.maxUnconnectedAddrs {
 					mab.addrs.Delete(a)
-					putExpiringAddrs(a)
 				} else {
 					a.Addr = addr
 					a.Expiry = exp
@@ -443,15 +424,13 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du
 				}
 			} else {
 				mab.addrs.Delete(a)
-				putExpiringAddrs(a)
 			}
 		} else {
 			if ttl > 0 {
 				if !ttlIsConnected(ttl) && mab.addrs.NumUnconnectedAddrs() >= mab.maxUnconnectedAddrs {
 					continue
 				}
-				entry := getExpiringAddrs()
-				*entry = expiringAddr{Addr: addr, Expiry: exp, TTL: ttl, Peer: p}
+				entry := &expiringAddr{Addr: addr, Expiry: exp, TTL: ttl, Peer: p}
 				mab.addrs.Insert(entry)
 				mab.subManager.BroadcastAddr(p, addr)
 			}
@@ -472,12 +451,10 @@ func (mab *memoryAddrBook) UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL t
 		if oldTTL == a.TTL {
 			if newTTL == 0 {
 				mab.addrs.Delete(a)
-				putExpiringAddrs(a)
 			} else {
 				// We are over limit, drop these addresses.
 				if ttlIsConnected(oldTTL) && !ttlIsConnected(newTTL) && mab.addrs.NumUnconnectedAddrs() >= mab.maxUnconnectedAddrs {
 					mab.addrs.Delete(a)
-					putExpiringAddrs(a)
 				} else {
 					a.TTL = newTTL
 					a.Expiry = exp
@@ -541,7 +518,6 @@ func (mab *memoryAddrBook) ClearAddrs(p peer.ID) {
 	delete(mab.signedPeerRecords, p)
 	for _, a := range mab.addrs.Addrs[p] {
 		mab.addrs.Delete(a)
-		putExpiringAddrs(a)
 	}
 }
 


Test patch:
diff --git a/p2p/host/peerstore/test/benchmarks_suite.go b/p2p/host/peerstore/test/benchmarks_suite.go
index 8fc1f4fc..01669a1b 100644
--- a/p2p/host/peerstore/test/benchmarks_suite.go
+++ b/p2p/host/peerstore/test/benchmarks_suite.go
@@ -36,11 +36,20 @@ func BenchmarkPeerstore(b *testing.B, factory PeerstoreFactory, variant string)
 			ps, cleanup := factory()
 			defer cleanup()
 			b.ResetTimer()
+			itersPerBM := 10
 			for i := 0; i < b.N; i++ {
-				pp := peers[i%N]
-				ps.AddAddrs(pp.ID, pp.Addr, pstore.RecentlyConnectedAddrTTL)
-				ps.Addrs(pp.ID)
-				ps.ClearAddrs(pp.ID)
+				for j := 0; j < itersPerBM; j++ {
+					pp := peers[(i+j)%N]
+					ps.AddAddrs(pp.ID, pp.Addr, pstore.RecentlyConnectedAddrTTL)
+				}
+				for j := 0; j < itersPerBM; j++ {
+					pp := peers[(i+j)%N]
+					ps.Addrs(pp.ID)
+				}
+				for j := 0; j < itersPerBM; j++ {
+					pp := peers[(i+j)%N]
+					ps.ClearAddrs(pp.ID)
+				}
 			}
 		})