NPeersForCpl and collapse empty buckets #77

aarshkshah1992 · 2020-04-21T10:28:32Z

In meta-issue libp2p/go-libp2p-kad-dht#556, this if for:

Stop refreshing once we stop filling buckets go-libp2p-kad-dht#550
- In order to know when to stop refreshing a CPL, we first need to know the peers we have for a given CPL.
Collapse empty buckets when possible. #72
- We should collapse the last/second last bucket if it's empty.

willscott · 2020-04-22T14:21:16Z

table.go

+		for _, p := range rt.buckets[cpl].peerIds() {
+			peers = append(peers, p)
+		}


Suggested change

for _, p := range rt.buckets[cpl].peerIds() {

peers = append(peers, p)

}

return rt.buckets[cpl].peerIds()

We don't need this function anymore.

willscott · 2020-04-22T14:24:06Z

table.go

@@ -85,6 +85,58 @@ func (rt *RoutingTable) Close() error {
 	return nil
 }

+// NPeersForCPL returns the number of peers we have for a given Cpl
+func (rt *RoutingTable) NPeersForCpl(cpl uint) int {


can you get away without this function, and just call
len(GetPeersForCpl) instead?

Have removed GetPeersForCpl as we don't need it anymore.

We've removed GetPeersForCpl as we don't need it anymore.

I guess this is a tiny bit more efficient then just len(GetPeersForCpl) and we can always add GetPeersForCpl back in if we need it. Your call 😄

aschmahmann

Left a couple comments here. libp2p/go-libp2p-kad-dht#601 is up next.

aschmahmann · 2020-04-22T14:31:36Z

table.go

+		for _, p := range b.peerIds() {
+			if CommonPrefixLen(rt.local, ConvertPeerID(p)) == int(cpl) {


Should be using b.peers and p.kadID instead of b.peerIds and ConvertPeerID(p). Gotta cache those hashes 😄

This is done.

aschmahmann · 2020-04-22T14:32:05Z

table.go

+		for _, p := range b.peerIds() {
+			if CommonPrefixLen(rt.local, ConvertPeerID(p)) == int(cpl) {


Should be using b.peers and p.kadID instead of b.peerIds and ConvertPeerID(p). Gotta cache those hashes 😄

We've removed this function.

table.go

aschmahmann · 2020-04-22T14:37:14Z

table.go

+// IsBucketFull returns true if the Logical bucket for a given Cpl is full
+func (rt *RoutingTable) IsBucketFull(cpl uint) bool {


Is this function used for anything, or just here for convenience/to be more efficient then len(GetPeersForCpl) == bucketSize? The one place it's used in the libp2p/go-libp2p-kad-dht#601 seems like an oversight to me.

We've removed this function now.

aschmahmann · 2020-04-22T14:45:43Z

table.go

+		// if we're querying the peer first time after adding it, let's give it a
+		// usefulness bump. This will ONLY happen once.
+		if peer.LastUsefulAt.IsZero() && queryPeer {
+			peer.LastUsefulAt = lastUsefulAt
+		}


Is this fix related to the rest of the PR, or just a "while I'm already here" kind of thing?

Hey,

This change is not related to the rest of the PR.
I think I wasn't being mindful when I put this change in as I didn't realize I hadn't changed the branch. Apologies.

No problem, while you're here and changing this do you mind add more information to the TryAddPeer comment about when we set LastUsefulAt? There is currently only information about when we set LastSuccessfulOutboundQuery

table_test.go

aschmahmann · 2020-04-22T15:01:19Z

table.go

+		// if the second last bucket just became empty, remove and replace it with the last bucket.
+		if bucketID == len(rt.buckets)-2 && len(bucket.peers()) == 0 {


I don't think this is good enough, as is shown in my test comment. What happens if we have buckets 1,2,3,4 and remove the peers from buckets 2 then 3 then 4? Here we'll collapse bucket 4 into bucket 3 and bucket 3 into bucket 2, leaving us with two total buckets instead of just one.

Perhaps doing this in a for loop would be better so we can close all buckets at the end that are empty.

I agree and this has been fixed.

aschmahmann · 2020-04-22T15:02:22Z

table.go

+		// remove this bucket if it was the last bucket and it's now empty
+		// provided it isn't the ONLY bucket we have.
+		if len(rt.buckets) > 1 && bucketID == len(rt.buckets)-1 && len(bucket.peers()) == 0 {
+			rt.buckets[bucketID] = nil


Might be just blanking here, but why are we doing this? Does it help the garbage collector out in some way?

It helps the GC garbage collect the struct pointed to by the slice element as I understand from here:

https://github.com/golang/go/wiki/SliceTricks (Look at the section on Delete).
However, to truly grok the mechanics and reasons behind this, I've posted a question on one of our go forums and will post the answer here when I get one.

aschmahmann

Left a brief comment, but otherwise LGTM

aschmahmann · 2020-05-08T17:46:13Z

table.go

+		// if we're querying the peer first time after adding it, let's give it a
+		// usefulness bump. This will ONLY happen once.
+		if peer.LastUsefulAt.IsZero() && queryPeer {
+			peer.LastUsefulAt = lastUsefulAt
+		}


No problem, while you're here and changing this do you mind add more information to the TryAddPeer comment about when we set LastUsefulAt? There is currently only information about when we set LastSuccessfulOutboundQuery

aschmahmann · 2020-05-08T19:19:28Z

table.go

@@ -85,6 +85,58 @@ func (rt *RoutingTable) Close() error {
 	return nil
 }

+// NPeersForCPL returns the number of peers we have for a given Cpl
+func (rt *RoutingTable) NPeersForCpl(cpl uint) int {


I guess this is a tiny bit more efficient then just len(GetPeersForCpl) and we can always add GetPeersForCpl back in if we need it. Your call 😄

aarshkshah1992 · 2020-05-11T06:58:25Z

@aschmahmann Thanks for the review. I've added better documentation around LastUsefulAt . Merging this !

nPeerForCpl and collapse empty buckets

cc56205

aarshkshah1992 requested review from Stebalien and aschmahmann April 21, 2020 10:28

aarshkshah1992 mentioned this pull request Apr 21, 2020

Fix Routing Table Refresh after we have a SearchByKadId RPC libp2p/go-libp2p-kad-dht#556

Open

7 tasks

aarshkshah1992 added 3 commits April 21, 2020 16:38

is bucket full

c52371e

get peers for a cpl

40bab45

first peer query after connection

9ee07ae

aarshkshah1992 mentioned this pull request Apr 22, 2020

Routing Table Refresh manager libp2p/go-libp2p-kad-dht#601

Merged

willscott reviewed Apr 22, 2020

View reviewed changes

aschmahmann requested changes Apr 22, 2020

View reviewed changes

aarshkshah1992 added 2 commits April 23, 2020 23:21

changes as per review

ee0822c

changes as per review

f3c8e92

Stebalien requested a review from aschmahmann May 4, 2020 06:19

aschmahmann approved these changes May 8, 2020

View reviewed changes

changes as per review

2e988e5

aarshkshah1992 merged commit eb2adab into master May 11, 2020

aarshkshah1992 deleted the feat/npeer-and-collapse-buckets branch May 11, 2020 07:03

Stebalien mentioned this pull request May 26, 2020

Release 0.6.0 ipfs/kubo#7347

Closed

77 tasks

aarshkshah1992 mentioned this pull request Jun 3, 2020

Collapse empty buckets when possible. #72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NPeersForCpl and collapse empty buckets #77

NPeersForCpl and collapse empty buckets #77

aarshkshah1992 commented Apr 21, 2020 •

edited

Loading

willscott Apr 22, 2020

aarshkshah1992 Apr 23, 2020

willscott Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann May 8, 2020

aschmahmann left a comment

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann May 8, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann Apr 22, 2020

aarshkshah1992 Apr 23, 2020

aschmahmann left a comment •

edited

Loading

aschmahmann May 8, 2020

aschmahmann May 8, 2020

aarshkshah1992 commented May 11, 2020

		for _, p := range b.peerIds() {
		if CommonPrefixLen(rt.local, ConvertPeerID(p)) == int(cpl) {

		// IsBucketFull returns true if the Logical bucket for a given Cpl is full
		func (rt *RoutingTable) IsBucketFull(cpl uint) bool {

		// if the second last bucket just became empty, remove and replace it with the last bucket.
		if bucketID == len(rt.buckets)-2 && len(bucket.peers()) == 0 {

NPeersForCpl and collapse empty buckets #77

NPeersForCpl and collapse empty buckets #77

Conversation

aarshkshah1992 commented Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschmahmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschmahmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 11, 2020

aarshkshah1992 commented Apr 21, 2020 •

edited

Loading

aschmahmann left a comment •

edited

Loading