Skip to content

Commit

Permalink
Do not return unused values from wantlists (#792)
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@gammazero
gammazero authored Jan 17, 2025
1 parent 7fa9846 commit 8ca0ca2
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 119 deletions.
17 changes: 5 additions & 12 deletions bitswap/client/internal/blockpresencemanager/blockpresencemanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
71 changes: 36 additions & 35 deletions bitswap/client/internal/messagequeue/messagequeue.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,66 +125,67 @@ 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
}
r.sent.Add(e.Cid, e.Priority, e.WantType)
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++
}
}
Expand Down Expand Up @@ -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
Expand All @@ -343,15 +344,15 @@ 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
// for the 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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -817,15 +818,15 @@ 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
}
}

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
}
Expand All @@ -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)
}
}

Expand Down
19 changes: 9 additions & 10 deletions bitswap/client/wantlist/wantlist.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit 8ca0ca2

Please sign in to comment.