From 8ca0ca28612a37df47deb72bc9d7e188e5c2b96d Mon Sep 17 00:00:00 2001 From: Andrew Gillis <11790789+gammazero@users.noreply.github.com> Date: Fri, 17 Jan 2025 05:51:11 -1000 Subject: [PATCH] Do not return unused values from wantlists (#792) - Do not return unused value from Remove or from Contains - Do not export functions unnecessarily.* Do not return unused values from wantlists - Remove unnecessary setting empty dict to nil The presence map in blockPresenceManager is never empty during normal use, so do not bother setting the map to nil for CG is its size becomes zero. --- .../blockpresencemanager.go | 17 ++-- .../internal/messagequeue/messagequeue.go | 71 +++++++-------- bitswap/client/wantlist/wantlist.go | 19 ++-- bitswap/client/wantlist/wantlist_test.go | 86 ++++++------------- 4 files changed, 74 insertions(+), 119 deletions(-) diff --git a/bitswap/client/internal/blockpresencemanager/blockpresencemanager.go b/bitswap/client/internal/blockpresencemanager/blockpresencemanager.go index b88b12bd3..288de6975 100644 --- a/bitswap/client/internal/blockpresencemanager/blockpresencemanager.go +++ b/bitswap/client/internal/blockpresencemanager/blockpresencemanager.go @@ -15,7 +15,9 @@ type BlockPresenceManager struct { } func New() *BlockPresenceManager { - return &BlockPresenceManager{} + return &BlockPresenceManager{ + presence: make(map[cid.Cid]map[peer.ID]bool), + } } // ReceiveFrom is called when a peer sends us information about which blocks @@ -24,10 +26,6 @@ func (bpm *BlockPresenceManager) ReceiveFrom(p peer.ID, haves []cid.Cid, dontHav bpm.Lock() defer bpm.Unlock() - if bpm.presence == nil { - bpm.presence = make(map[cid.Cid]map[peer.ID]bool) - } - for _, c := range haves { bpm.updateBlockPresence(p, c, true) } @@ -39,7 +37,8 @@ func (bpm *BlockPresenceManager) ReceiveFrom(p peer.ID, haves []cid.Cid, dontHav func (bpm *BlockPresenceManager) updateBlockPresence(p peer.ID, c cid.Cid, present bool) { _, ok := bpm.presence[c] if !ok { - bpm.presence[c] = make(map[peer.ID]bool) + bpm.presence[c] = map[peer.ID]bool{p: present} + return } // Make sure not to change HAVE to DONT_HAVE @@ -121,9 +120,6 @@ func (bpm *BlockPresenceManager) RemoveKeys(ks []cid.Cid) { for _, c := range ks { delete(bpm.presence, c) } - if len(bpm.presence) == 0 { - bpm.presence = nil - } } // RemovePeer removes the given peer from every cid key in the presence map. @@ -137,9 +133,6 @@ func (bpm *BlockPresenceManager) RemovePeer(p peer.ID) { delete(bpm.presence, c) } } - if len(bpm.presence) == 0 { - bpm.presence = nil - } } // HasKey indicates whether the BlockPresenceManager is tracking the given key diff --git a/bitswap/client/internal/messagequeue/messagequeue.go b/bitswap/client/internal/messagequeue/messagequeue.go index 6a6e44280..a8ab99830 100644 --- a/bitswap/client/internal/messagequeue/messagequeue.go +++ b/bitswap/client/internal/messagequeue/messagequeue.go @@ -125,32 +125,32 @@ func newRecallWantList() recallWantlist { } } -// Add want to the pending list -func (r *recallWantlist) Add(c cid.Cid, priority int32, wtype pb.Message_Wantlist_WantType) { +// add want to the pending list +func (r *recallWantlist) add(c cid.Cid, priority int32, wtype pb.Message_Wantlist_WantType) { r.pending.Add(c, priority, wtype) } -// Remove wants from both the pending list and the list of sent wants -func (r *recallWantlist) Remove(c cid.Cid) { +// remove wants from both the pending list and the list of sent wants +func (r *recallWantlist) remove(c cid.Cid) { r.pending.Remove(c) r.sent.Remove(c) delete(r.sentAt, c) } -// Remove wants by type from both the pending list and the list of sent wants -func (r *recallWantlist) RemoveType(c cid.Cid, wtype pb.Message_Wantlist_WantType) { +// remove wants by type from both the pending list and the list of sent wants +func (r *recallWantlist) removeType(c cid.Cid, wtype pb.Message_Wantlist_WantType) { r.pending.RemoveType(c, wtype) r.sent.RemoveType(c, wtype) - if _, ok := r.sent.Contains(c); !ok { + if !r.sent.Has(c) { delete(r.sentAt, c) } } -// MarkSent moves the want from the pending to the sent list +// markSent moves the want from the pending to the sent list // // Returns true if the want was marked as sent. Returns false if the want wasn't // pending. -func (r *recallWantlist) MarkSent(e bswl.Entry) bool { +func (r *recallWantlist) markSent(e bswl.Entry) bool { if !r.pending.RemoveType(e.Cid, e.WantType) { return false } @@ -158,33 +158,34 @@ func (r *recallWantlist) MarkSent(e bswl.Entry) bool { return true } -// SentAt records the time at which a want was sent -func (r *recallWantlist) SentAt(c cid.Cid, at time.Time) { +// setSentAt records the time at which a want was sent +func (r *recallWantlist) setSentAt(c cid.Cid, at time.Time) { // The want may have been canceled in the interim - if _, ok := r.sent.Contains(c); ok { + if r.sent.Has(c) { if _, ok := r.sentAt[c]; !ok { r.sentAt[c] = at } } } -// ClearSentAt clears out the record of the time a want was sent. +// clearSentAt clears out the record of the time a want was sent. // We clear the sent at time when we receive a response for a key as we // only need the first response for latency measurement. -func (r *recallWantlist) ClearSentAt(c cid.Cid) { +func (r *recallWantlist) clearSentAt(c cid.Cid) { delete(r.sentAt, c) } -// Refresh moves wants from the sent list back to the pending list. +// refresh moves wants from the sent list back to the pending list. // If a want has been sent for longer than the interval, it is moved back to the pending list. // Returns the number of wants that were refreshed. -func (r *recallWantlist) Refresh(now time.Time, interval time.Duration) int { +func (r *recallWantlist) refresh(now time.Time, interval time.Duration) int { var refreshed int for _, want := range r.sent.Entries() { - sentAt, ok := r.sentAt[want.Cid] + wantCid := want.Cid + sentAt, ok := r.sentAt[wantCid] if ok && now.Sub(sentAt) >= interval { - r.pending.Add(want.Cid, want.Priority, want.WantType) - r.sent.Remove(want.Cid) + r.sent.Remove(wantCid) + r.pending.Add(wantCid, want.Priority, want.WantType) refreshed++ } } @@ -320,7 +321,7 @@ func (mq *MessageQueue) AddBroadcastWantHaves(wantHaves []cid.Cid) { mq.wllock.Lock() for _, c := range wantHaves { - mq.bcstWants.Add(c, mq.priority, pb.Message_Wantlist_Have) + mq.bcstWants.add(c, mq.priority, pb.Message_Wantlist_Have) mq.priority-- // We're adding a want-have for the cid, so clear any pending cancel @@ -343,7 +344,7 @@ func (mq *MessageQueue) AddWants(wantBlocks []cid.Cid, wantHaves []cid.Cid) { mq.wllock.Lock() for _, c := range wantHaves { - mq.peerWants.Add(c, mq.priority, pb.Message_Wantlist_Have) + mq.peerWants.add(c, mq.priority, pb.Message_Wantlist_Have) mq.priority-- // We're adding a want-have for the cid, so clear any pending cancel @@ -351,7 +352,7 @@ func (mq *MessageQueue) AddWants(wantBlocks []cid.Cid, wantHaves []cid.Cid) { mq.cancels.Remove(c) } for _, c := range wantBlocks { - mq.peerWants.Add(c, mq.priority, pb.Message_Wantlist_Block) + mq.peerWants.add(c, mq.priority, pb.Message_Wantlist_Block) mq.priority-- // We're adding a want-block for the cid, so clear any pending cancel @@ -383,12 +384,12 @@ func (mq *MessageQueue) AddCancels(cancelKs []cid.Cid) { // Remove keys from broadcast and peer wants, and add to cancels for _, c := range cancelKs { // Check if a want for the key was sent - _, wasSentBcst := mq.bcstWants.sent.Contains(c) - _, wasSentPeer := mq.peerWants.sent.Contains(c) + wasSentBcst := mq.bcstWants.sent.Has(c) + wasSentPeer := mq.peerWants.sent.Has(c) // Remove the want from tracking wantlists - mq.bcstWants.Remove(c) - mq.peerWants.Remove(c) + mq.bcstWants.remove(c) + mq.peerWants.remove(c) // Only send a cancel if a want was sent if wasSentBcst || wasSentPeer { @@ -524,7 +525,7 @@ func (mq *MessageQueue) runQueue() { func (mq *MessageQueue) rebroadcastWantlist(now time.Time, interval time.Duration) { mq.wllock.Lock() // Transfer wants from the rebroadcast lists into the pending lists. - toRebroadcast := mq.bcstWants.Refresh(now, interval) + mq.peerWants.Refresh(now, interval) + toRebroadcast := mq.bcstWants.refresh(now, interval) + mq.peerWants.refresh(now, interval) mq.wllock.Unlock() // If some wants were transferred from the rebroadcast list @@ -614,7 +615,7 @@ func (mq *MessageQueue) simulateDontHaveWithTimeout(wantlist []bsmsg.Entry) { // Unlikely, but just in case check that the block hasn't been // received in the interim c := entry.Cid - if _, ok := mq.peerWants.sent.Contains(c); ok { + if mq.peerWants.sent.Has(c) { wants = append(wants, c) } } @@ -652,7 +653,7 @@ func (mq *MessageQueue) handleResponse(ks []cid.Cid) { if (earliest.IsZero() || at.Before(earliest)) && now.Sub(at) < mq.maxValidLatency { earliest = at } - mq.bcstWants.ClearSentAt(c) + mq.bcstWants.clearSentAt(c) } if at, ok := mq.peerWants.sentAt[c]; ok { if (earliest.IsZero() || at.Before(earliest)) && now.Sub(at) < mq.maxValidLatency { @@ -661,7 +662,7 @@ func (mq *MessageQueue) handleResponse(ks []cid.Cid) { // Clear out the sent time for the CID because we only want to // record the latency between the request and the first response // for that CID (not subsequent responses) - mq.peerWants.ClearSentAt(c) + mq.peerWants.clearSentAt(c) } } @@ -752,7 +753,7 @@ func (mq *MessageQueue) extractOutgoingMessage(supportsHave bool) (bsmsg.BitSwap // place if possible. for _, e := range peerEntries { if e.WantType == pb.Message_Wantlist_Have { - mq.peerWants.RemoveType(e.Cid, pb.Message_Wantlist_Have) + mq.peerWants.removeType(e.Cid, pb.Message_Wantlist_Have) } else { filteredPeerEntries = append(filteredPeerEntries, e) } @@ -817,7 +818,7 @@ FINISH: // message that we've decided to cancel at the last minute. mq.wllock.Lock() for i, e := range peerEntries[:sentPeerEntries] { - if !mq.peerWants.MarkSent(e) { + if !mq.peerWants.markSent(e) { // It changed. mq.msg.Remove(e.Cid) peerEntries[i].Cid = cid.Undef @@ -825,7 +826,7 @@ FINISH: } for i, e := range bcstEntries[:sentBcstEntries] { - if !mq.bcstWants.MarkSent(e) { + if !mq.bcstWants.markSent(e) { mq.msg.Remove(e.Cid) bcstEntries[i].Cid = cid.Undef } @@ -849,13 +850,13 @@ FINISH: for _, e := range peerEntries[:sentPeerEntries] { if e.Cid.Defined() { // Check if want was canceled in the interim - mq.peerWants.SentAt(e.Cid, now) + mq.peerWants.setSentAt(e.Cid, now) } } for _, e := range bcstEntries[:sentBcstEntries] { if e.Cid.Defined() { // Check if want was canceled in the interim - mq.bcstWants.SentAt(e.Cid, now) + mq.bcstWants.setSentAt(e.Cid, now) } } diff --git a/bitswap/client/wantlist/wantlist.go b/bitswap/client/wantlist/wantlist.go index 245085af9..a79dbd9fc 100644 --- a/bitswap/client/wantlist/wantlist.go +++ b/bitswap/client/wantlist/wantlist.go @@ -71,14 +71,8 @@ func (w *Wantlist) Add(c cid.Cid, priority int32, wantType pb.Message_Wantlist_W } // Remove removes the given cid from the wantlist. -func (w *Wantlist) Remove(c cid.Cid) bool { - _, ok := w.set[c] - if !ok { - return false - } - +func (w *Wantlist) Remove(c cid.Cid) { w.delete(c) - return true } // Remove removes the given cid from the wantlist, respecting the type: @@ -108,9 +102,14 @@ func (w *Wantlist) put(c cid.Cid, e Entry) { w.set[c] = e } -// Contains returns the entry, if present, for the given CID, plus whether it -// was present. -func (w *Wantlist) Contains(c cid.Cid) (Entry, bool) { +func (w *Wantlist) Has(c cid.Cid) bool { + _, ok := w.set[c] + return ok +} + +// Get returns the entry, if present, for the given CID, plus whether it was +// present. +func (w *Wantlist) Get(c cid.Cid) (Entry, bool) { e, ok := w.set[c] return e, ok } diff --git a/bitswap/client/wantlist/wantlist_test.go b/bitswap/client/wantlist/wantlist_test.go index 901fe0d67..db5542ecf 100644 --- a/bitswap/client/wantlist/wantlist_test.go +++ b/bitswap/client/wantlist/wantlist_test.go @@ -26,54 +26,37 @@ func init() { } type wli interface { - Contains(cid.Cid) (Entry, bool) + Get(cid.Cid) (Entry, bool) + Has(cid.Cid) bool } func assertHasCid(t *testing.T, w wli, c cid.Cid) { - e, ok := w.Contains(c) - if !ok { - t.Fatal("expected to have ", c) - } - if !e.Cid.Equals(c) { - t.Fatal("returned entry had wrong cid value") - } + e, ok := w.Get(c) + require.True(t, ok) + require.Equal(t, c, e.Cid) } func TestBasicWantlist(t *testing.T) { wl := New() - if !wl.Add(testcids[0], 5, pb.Message_Wantlist_Block) { - t.Fatal("expected true") - } + require.True(t, wl.Add(testcids[0], 5, pb.Message_Wantlist_Block)) assertHasCid(t, wl, testcids[0]) - if !wl.Add(testcids[1], 4, pb.Message_Wantlist_Block) { - t.Fatal("expected true") - } + require.True(t, wl.Add(testcids[1], 4, pb.Message_Wantlist_Block)) assertHasCid(t, wl, testcids[0]) assertHasCid(t, wl, testcids[1]) - if wl.Len() != 2 { - t.Fatal("should have had two items") - } + require.Equal(t, 2, wl.Len()) - if wl.Add(testcids[1], 4, pb.Message_Wantlist_Block) { - t.Fatal("add shouldnt report success on second add") - } + require.False(t, wl.Add(testcids[1], 4, pb.Message_Wantlist_Block), "add should not report success on second add") assertHasCid(t, wl, testcids[0]) assertHasCid(t, wl, testcids[1]) - if wl.Len() != 2 { - t.Fatal("should have had two items") - } + require.Equal(t, 2, wl.Len()) - if !wl.RemoveType(testcids[0], pb.Message_Wantlist_Block) { - t.Fatal("should have gotten true") - } + require.True(t, wl.RemoveType(testcids[0], pb.Message_Wantlist_Block)) assertHasCid(t, wl, testcids[1]) - if _, has := wl.Contains(testcids[0]); has { - t.Fatal("shouldnt have this cid") - } + require.False(t, wl.Has(testcids[0]), "should not have this cid") } func TestAddHaveThenBlock(t *testing.T) { @@ -82,13 +65,9 @@ func TestAddHaveThenBlock(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Have) wl.Add(testcids[0], 5, pb.Message_Wantlist_Block) - e, ok := wl.Contains(testcids[0]) - if !ok { - t.Fatal("expected to have ", testcids[0]) - } - if e.WantType != pb.Message_Wantlist_Block { - t.Fatal("expected to be ", pb.Message_Wantlist_Block) - } + e, ok := wl.Get(testcids[0]) + require.True(t, ok) + require.Equal(t, pb.Message_Wantlist_Block, e.WantType) } func TestAddBlockThenHave(t *testing.T) { @@ -97,13 +76,9 @@ func TestAddBlockThenHave(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Block) wl.Add(testcids[0], 5, pb.Message_Wantlist_Have) - e, ok := wl.Contains(testcids[0]) - if !ok { - t.Fatal("expected to have ", testcids[0]) - } - if e.WantType != pb.Message_Wantlist_Block { - t.Fatal("expected to be ", pb.Message_Wantlist_Block) - } + e, ok := wl.Get(testcids[0]) + require.True(t, ok) + require.Equal(t, pb.Message_Wantlist_Block, e.WantType) } func TestAddHaveThenRemoveBlock(t *testing.T) { @@ -112,10 +87,7 @@ func TestAddHaveThenRemoveBlock(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Have) wl.RemoveType(testcids[0], pb.Message_Wantlist_Block) - _, ok := wl.Contains(testcids[0]) - if ok { - t.Fatal("expected not to have ", testcids[0]) - } + require.False(t, wl.Has(testcids[0])) } func TestAddBlockThenRemoveHave(t *testing.T) { @@ -124,13 +96,9 @@ func TestAddBlockThenRemoveHave(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Block) wl.RemoveType(testcids[0], pb.Message_Wantlist_Have) - e, ok := wl.Contains(testcids[0]) - if !ok { - t.Fatal("expected to have ", testcids[0]) - } - if e.WantType != pb.Message_Wantlist_Block { - t.Fatal("expected to be ", pb.Message_Wantlist_Block) - } + e, ok := wl.Get(testcids[0]) + require.True(t, ok) + require.Equal(t, pb.Message_Wantlist_Block, e.WantType) } func TestAddHaveThenRemoveAny(t *testing.T) { @@ -139,10 +107,7 @@ func TestAddHaveThenRemoveAny(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Have) wl.Remove(testcids[0]) - _, ok := wl.Contains(testcids[0]) - if ok { - t.Fatal("expected not to have ", testcids[0]) - } + require.False(t, wl.Has(testcids[0])) } func TestAddBlockThenRemoveAny(t *testing.T) { @@ -151,10 +116,7 @@ func TestAddBlockThenRemoveAny(t *testing.T) { wl.Add(testcids[0], 5, pb.Message_Wantlist_Block) wl.Remove(testcids[0]) - _, ok := wl.Contains(testcids[0]) - if ok { - t.Fatal("expected not to have ", testcids[0]) - } + require.False(t, wl.Has(testcids[0])) } func TestSortEntries(t *testing.T) {