From 382eb8356615bae9687fa186c1c4927afe1f8d6c Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 22:16:08 -0700 Subject: [PATCH 1/3] revert to storing peer addresses in a map We switched to a slice to reduce the amount of memory the peerstore ended up taking up unfortunately, this really killed us in allocations. Looking at go-ipfs profiles, I'm worried that memory fragmentation is killing us so I'd like to revert to the old behavior. Note: The real solution here is dealing with "address abusers". --- p2p/host/peerstore/pstoremem/addr_book.go | 68 ++++++++--------------- 1 file changed, 24 insertions(+), 44 deletions(-) diff --git a/p2p/host/peerstore/pstoremem/addr_book.go b/p2p/host/peerstore/pstoremem/addr_book.go index 311c436ccb..1cd1a2a6c8 100644 --- a/p2p/host/peerstore/pstoremem/addr_book.go +++ b/p2p/host/peerstore/pstoremem/addr_book.go @@ -26,21 +26,19 @@ func (e *expiringAddr) ExpiredBy(t time.Time) bool { return t.After(e.Expires) } -type addrSlice []expiringAddr - var _ pstore.AddrBook = (*memoryAddrBook)(nil) // memoryAddrBook manages addresses. type memoryAddrBook struct { addrmu sync.Mutex - addrs map[peer.ID]addrSlice + addrs map[peer.ID]map[string]expiringAddr subManager *AddrSubManager } func NewAddrBook() pstore.AddrBook { return &memoryAddrBook{ - addrs: make(map[peer.ID]addrSlice), + addrs: make(map[peer.ID]map[string]expiringAddr), subManager: NewAddrSubManager(), } } @@ -76,20 +74,17 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du return } - oldAddrs := mab.addrs[p] - amap := make(map[string]expiringAddr, len(oldAddrs)) - for _, ea := range oldAddrs { - amap[string(ea.Addr.Bytes())] = ea + amap := mab.addrs[p] + if amap == nil { + amap = make(map[string]expiringAddr, len(addrs)) + mab.addrs[p] = amap } - - // only expand ttls exp := time.Now().Add(ttl) for _, addr := range addrs { if addr == nil { log.Warningf("was passed nil multiaddr for %s", p) continue } - addrstr := string(addr.Bytes()) a, found := amap[addrstr] if !found || exp.After(a.Expires) { @@ -98,11 +93,6 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du mab.subManager.BroadcastAddr(p, addr) } } - newAddrs := make([]expiringAddr, 0, len(amap)) - for _, ea := range amap { - newAddrs = append(newAddrs, ea) - } - mab.addrs[p] = newAddrs } // SetAddr calls mgr.SetAddrs(p, addr, ttl) @@ -116,10 +106,10 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du mab.addrmu.Lock() defer mab.addrmu.Unlock() - oldAddrs := mab.addrs[p] - amap := make(map[string]expiringAddr, len(oldAddrs)) - for _, ea := range oldAddrs { - amap[string(ea.Addr.Bytes())] = ea + amap := mab.addrs[p] + if amap == nil { + amap = make(map[string]expiringAddr, len(addrs)) + mab.addrs[p] = amap } exp := time.Now().Add(ttl) @@ -129,21 +119,16 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du continue } // re-set all of them for new ttl. - addrs := string(addr.Bytes()) + addrstr := string(addr.Bytes()) if ttl > 0 { - amap[addrs] = expiringAddr{Addr: addr, Expires: exp, TTL: ttl} + amap[addrstr] = expiringAddr{Addr: addr, Expires: exp, TTL: ttl} mab.subManager.BroadcastAddr(p, addr) } else { - delete(amap, addrs) + delete(amap, addrstr) } } - newAddrs := make([]expiringAddr, 0, len(amap)) - for _, ea := range amap { - newAddrs = append(newAddrs, ea) - } - mab.addrs[p] = newAddrs } // UpdateAddrs updates the addresses associated with the given peer that have @@ -156,16 +141,17 @@ func (mab *memoryAddrBook) UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL t return } - addrs, found := mab.addrs[p] + amap, found := mab.addrs[p] if !found { return } exp := time.Now().Add(newTTL) - for i := range addrs { - if aexp := &addrs[i]; oldTTL == aexp.TTL { - aexp.TTL = newTTL - aexp.Expires = exp + for k, addr := range amap { + if oldTTL == addr.TTL { + addr.TTL = newTTL + addr.Expires = exp + amap[k] = addr } } } @@ -180,27 +166,21 @@ func (mab *memoryAddrBook) Addrs(p peer.ID) []ma.Multiaddr { return nil } - maddrs, found := mab.addrs[p] + amap, found := mab.addrs[p] if !found { return nil } now := time.Now() - good := make([]ma.Multiaddr, 0, len(maddrs)) - cleaned := make([]expiringAddr, 0, len(maddrs)) - for _, m := range maddrs { + good := make([]ma.Multiaddr, 0, len(amap)) + for k, m := range amap { if !m.ExpiredBy(now) { - cleaned = append(cleaned, m) good = append(good, m.Addr) + } else { + delete(amap, k) } } - // clean up the expired ones. - if len(cleaned) == 0 { - delete(mab.addrs, p) - } else { - mab.addrs[p] = cleaned - } return good } From a0a98a9d92895eeb88576f2f5a0901687014a47d Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 2 Oct 2018 10:49:55 -0700 Subject: [PATCH 2/3] add garbage collection for in-memory peerstore There are better ways to do this but pausing dialing once an hour likely isn't going to break anything and is the simplest approach. --- p2p/host/peerstore/pstoremem/addr_book.go | 35 +++++++++++++++-------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/p2p/host/peerstore/pstoremem/addr_book.go b/p2p/host/peerstore/pstoremem/addr_book.go index 1cd1a2a6c8..e72edd6d22 100644 --- a/p2p/host/peerstore/pstoremem/addr_book.go +++ b/p2p/host/peerstore/pstoremem/addr_book.go @@ -33,6 +33,8 @@ type memoryAddrBook struct { addrmu sync.Mutex addrs map[peer.ID]map[string]expiringAddr + nextGC time.Time + subManager *AddrSubManager } @@ -43,12 +45,27 @@ func NewAddrBook() pstore.AddrBook { } } +func (mab *memoryAddrBook) gc() { + now := time.Now() + if !now.After(mab.nextGC) { + return + } + for p, amap := range mab.addrs { + for k, addr := range amap { + if addr.ExpiredBy(now) { + delete(amap, k) + } + } + if len(amap) == 0 { + delete(mab.addrs, p) + } + } + mab.nextGC = time.Now().Add(pstore.AddressTTL) +} + func (mab *memoryAddrBook) PeersWithAddrs() peer.IDSlice { mab.addrmu.Lock() defer mab.addrmu.Unlock() - if mab.addrs == nil { - return nil - } pids := make(peer.IDSlice, 0, len(mab.addrs)) for pid := range mab.addrs { @@ -93,6 +110,7 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du mab.subManager.BroadcastAddr(p, addr) } } + mab.gc() } // SetAddr calls mgr.SetAddrs(p, addr, ttl) @@ -129,6 +147,7 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du delete(amap, addrstr) } } + mab.gc() } // UpdateAddrs updates the addresses associated with the given peer that have @@ -137,10 +156,6 @@ func (mab *memoryAddrBook) UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL t mab.addrmu.Lock() defer mab.addrmu.Unlock() - if mab.addrs == nil { - return - } - amap, found := mab.addrs[p] if !found { return @@ -154,6 +169,7 @@ func (mab *memoryAddrBook) UpdateAddrs(p peer.ID, oldTTL time.Duration, newTTL t amap[k] = addr } } + mab.gc() } // Addresses returns all known (and valid) addresses for a given @@ -161,11 +177,6 @@ func (mab *memoryAddrBook) Addrs(p peer.ID) []ma.Multiaddr { mab.addrmu.Lock() defer mab.addrmu.Unlock() - // not initialized? nothing to give. - if mab.addrs == nil { - return nil - } - amap, found := mab.addrs[p] if !found { return nil From 708455127bbd4ef3b6c63ee544850a9a20981947 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 3 Oct 2018 17:42:54 -0700 Subject: [PATCH 3/3] memoryAddrBook: document gc precondition --- p2p/host/peerstore/pstoremem/addr_book.go | 1 + 1 file changed, 1 insertion(+) diff --git a/p2p/host/peerstore/pstoremem/addr_book.go b/p2p/host/peerstore/pstoremem/addr_book.go index e72edd6d22..f0f8a2c87c 100644 --- a/p2p/host/peerstore/pstoremem/addr_book.go +++ b/p2p/host/peerstore/pstoremem/addr_book.go @@ -45,6 +45,7 @@ func NewAddrBook() pstore.AddrBook { } } +// gc garbage collects the in-memory address book. The caller *must* hold the addrmu lock. func (mab *memoryAddrBook) gc() { now := time.Now() if !now.After(mab.nextGC) {