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

fix(kad): improve memory allocation when iterating over kbuckets #5715

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Dec 5, 2024

Description

Proposal to fix #5712.

I have changed to ClosestIter structure to only allocate when kbucket_size is higher than K_VALUE and only once along the life of ClosestIter. I think I did not break anything but I would really like some experienced people with Kademlia to take a look (@guillaumemichel 😉).

Notes & open questions

I have kept the sort_by in the Iterator implementation of ClosestIter but I think we could remove every allocation whatsoever if we used min_by but I'm not sure that it would not change the research behaviour that's why I did not changed it. If experienced developers think that it would be equivalent I will do it.
It was a dumb question. I did not fully understood how the ClosestIter was working.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@stormshield-frb stormshield-frb changed the title fix(kad): improve memory allocation when iterating over kbuckets DRAFT fix(kad): improve memory allocation when iterating over kbuckets Dec 5, 2024
@guillaumemichel guillaumemichel marked this pull request as draft December 5, 2024 11:59
@guillaumemichel guillaumemichel changed the title DRAFT fix(kad): improve memory allocation when iterating over kbuckets fix(kad): improve memory allocation when iterating over kbuckets Dec 5, 2024
@stormshield-frb stormshield-frb marked this pull request as ready for review December 6, 2024 08:32
@elenaf9
Copy link
Contributor

elenaf9 commented Dec 11, 2024

@guillaumemichel do you have some time to review this? I am unfortunately not so familiar with kademlia.

@guillaumemichel
Copy link
Contributor

Yes, I will try to review before the end of week.

@vbhattaccmu
Copy link

Hi @guillaumemichel do we have any estimate when this branch will get merged? Currently we are running into memory growth on our p2p light clients as well and are using a version of libp2p after this change was introduced. Having this change on latest commit will help us a lot. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huge increase in memory allocation
5 participants