-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: Better GC in membacked peerstore #2960
Conversation
a7ce157
to
c7626c0
Compare
a61a20f
to
f33d954
Compare
@@ -114,11 +114,12 @@ func (pa *peerAddrs) NextExpiry() time.Time { | |||
if len(pa.expiringHeap) == 0 { | |||
return time.Time{} | |||
} | |||
return pa.expiringHeap[len(pa.expiringHeap)-1].Expires | |||
return pa.expiringHeap[0].Expires |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is right? The min value should be at the end, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added a couple of tests. See: TestPeerAddrsNextExpiry
If you're confused, as I was, by the fact that heap.Interface.Pop
requires removal from the end, that's because heap.Pop
swaps the first and last element before calling heap.Interface.Pop
.
from container/heap/heap.go
// Pop removes and returns the minimum element (according to Less) from the heap.
// The complexity is O(log n) where n = h.Len().
// Pop is equivalent to [Remove](h, 0).
func Pop(h Interface) any {
n := h.Len() - 1
h.Swap(0, n)
down(h, 0, n)
return h.Pop()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, thanks. That makes sense.
f33d954
to
f495482
Compare
ccf5a69
to
0a6a579
Compare
I've removed the change that limited the number of signed peer records we could store in the peer store. I removed the limit because:
I suggest we introduce the limit in a separate PR, and provide an option to opt out of the limit. |
d0aad5c
to
ce54f4b
Compare
ce54f4b
to
ca8f4d6
Compare
Changes look good. I appreciate the extra tests. Thank you. |
--------- Co-authored-by: sukun <sukunrt@gmail.com>
--------- Co-authored-by: sukun <sukunrt@gmail.com>
--------- Co-authored-by: sukun <sukunrt@gmail.com>
--------- Co-authored-by: sukun <sukunrt@gmail.com>
--------- Co-authored-by: sukun <sukunrt@gmail.com>
--------- Co-authored-by: sukun <sukunrt@gmail.com>
Closes #1698
Makes GC ~60x faster (on my synthetic benchmark) under certain circumstances. So by that logic, it should be fine to run this more often.
Adds a min heap list to track the next address to expire. This lets us short circuit gc if we see the next address to expire isn't expired yet.
Also bumps the gc interval from 60 minutes to 1 minute. This should be fine since we don't traverse every address anymore.
This relies on existing tests to assert nothing has broken.
Of course there are many more improvements that could be made. See the linked issue for inspiration. The next lowest hanging fruit is to do something similar to this across peers.