Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce allocs in AddPeer #81

Merged
merged 2 commits into from May 12, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion bucket.go
Expand Up @@ -43,14 +43,27 @@ func newBucket() *bucket {
// returns all peers in the bucket
// it is safe for the caller to modify the returned objects as it is a defensive copy
func (b *bucket) peers() []PeerInfo {
var ps []PeerInfo
ps := make([]PeerInfo, 0, b.len())
for e := b.list.Front(); e != nil; e = e.Next() {
p := e.Value.(*PeerInfo)
ps = append(ps, *p)
}
return ps
}

// returns the first peer in the bucket that satisfies the given predicate.
// It is NOT safe for the predicate to mutate the given `PeerInfo`
// as we pass in a pointer to it.
func (b *bucket) selectFirst(predicate func(p *PeerInfo) bool) peer.ID {
for e := b.list.Front(); e != nil; e = e.Next() {
p := e.Value.(*PeerInfo)
if predicate(p) {
return p.Id
}
}
return ""
}

// return the Ids of all the peers in the bucket.
func (b *bucket) peerIds() []peer.ID {
ps := make([]peer.ID, 0, b.list.Len())
Expand Down
20 changes: 10 additions & 10 deletions table.go
Expand Up @@ -152,16 +152,16 @@ func (rt *RoutingTable) addPeer(p peer.ID, queryPeer bool) (bool, error) {

// the bucket to which the peer belongs is full. Let's try to find a peer
// in that bucket with a LastSuccessfulOutboundQuery value above the maximum threshold and replace it.
allPeers := bucket.peers()
for _, pc := range allPeers {
if time.Since(pc.LastUsefulAt) > rt.usefulnessGracePeriod {
// let's evict it and add the new peer
if bucket.remove(pc.Id) {
bucket.pushFront(&PeerInfo{Id: p, LastUsefulAt: lastUsefulAt, LastSuccessfulOutboundQueryAt: time.Now(),
dhtId: ConvertPeerID(p)})
rt.PeerAdded(p)
return true, nil
}
pid := bucket.selectFirst(func(pi *PeerInfo) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ot nit: shouldn't we pick the least useful peer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stebalien What's the meaning of "ot nit" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, "off topic nit". That is, a small suggestion that isn't even related to the current patch (just something I noticed while reviewing it).

(I'm basically saying "feel free to ignore this for now, but we should fix this in a future patch").

return time.Since(pi.LastUsefulAt) > rt.usefulnessGracePeriod
})
if pid != "" {
// let's evict it and add the new peer
if bucket.remove(pid) {
bucket.pushFront(&PeerInfo{Id: p, LastUsefulAt: lastUsefulAt, LastSuccessfulOutboundQueryAt: time.Now(),
dhtId: ConvertPeerID(p)})
rt.PeerAdded(p)
return true, nil
}
}

Expand Down
6 changes: 6 additions & 0 deletions table_test.go
Expand Up @@ -60,6 +60,11 @@ func TestBucket(t *testing.T) {
require.EqualValues(t, t2, p.LastSuccessfulOutboundQueryAt)
require.EqualValues(t, t3, p.LastUsefulAt)

p.Id = peer.ID("randid")
require.EqualValues(t, "randid", b.selectFirst(func(pi *PeerInfo) bool {
return pi.Id == "randid"
}))

spl := b.split(0, ConvertPeerID(local))
llist := b.list
for e := llist.Front(); e != nil; e = e.Next() {
Expand All @@ -78,6 +83,7 @@ func TestBucket(t *testing.T) {
t.Fatalf("split failed. found id with cpl == 0 in non 0 bucket")
}
}

}

func TestRemovePeer(t *testing.T) {
Expand Down