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

feat(share/p2p/peer-manager): limit amount of stored pools in peer-manager #3005

Merged
merged 13 commits into from
Dec 18, 2023

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Dec 13, 2023

This PR introduces amountOfStoredPools param to Peer manager, that will limit amount of stored pools. Defautlt value is 10 pools. Peer manager will try to keep amountOfStoredPools amount of pools only for recent headers. Older blocks don't need to be routed by manual pools and could be routed to any full node, as those are expected to have the block by that time.

This will lower memory footprint of peer manager as well as resolve memory leaking issues.

Resolves #1781

@walldiss walldiss self-assigned this Dec 13, 2023
@walldiss walldiss changed the title feat(share/p2p/peer-manager): limit amount of stored pool in peer-manager !feat(share/p2p/peer-manager): limit amount of stored pool in peer-manager Dec 13, 2023
@walldiss walldiss added area:p2p kind:feat Attached to feature PRs labels Dec 13, 2023
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Initial glimpse. Did you run the node to see stable mem usage? I can help with that

share/p2p/peers/options.go Outdated Show resolved Hide resolved
share/p2p/peers/options.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (7f51c59) 50.94% compared to head (e223715) 50.68%.

Files Patch % Lines
share/p2p/peers/manager.go 82.14% 4 Missing and 1 partial ⚠️
share/getters/shrex.go 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3005      +/-   ##
==========================================
- Coverage   50.94%   50.68%   -0.26%     
==========================================
  Files         176      176              
  Lines       11190    11172      -18     
==========================================
- Hits         5701     5663      -38     
- Misses       4986     5012      +26     
+ Partials      503      497       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

share/p2p/peers/manager.go Outdated Show resolved Hide resolved
@walldiss walldiss changed the title !feat(share/p2p/peer-manager): limit amount of stored pool in peer-manager feat(share/p2p/peer-manager): limit amount of stored pool in peer-manager Dec 13, 2023
@walldiss walldiss changed the title feat(share/p2p/peer-manager): limit amount of stored pool in peer-manager feat(share/p2p/peer-manager): limit amount of stored pools in peer-manager Dec 13, 2023
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Show resolved Hide resolved
@walldiss
Copy link
Member Author

Yes, memory leak is solved.

… peer-manager-memory-leak

# Conflicts:
#	share/p2p/peers/manager.go
@walldiss walldiss enabled auto-merge (squash) December 17, 2023 08:38
@walldiss walldiss enabled auto-merge (squash) December 18, 2023 13:03
@walldiss walldiss merged commit c34b741 into celestiaorg:main Dec 18, 2023
15 of 17 checks passed
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Jan 15, 2024
…nager (celestiaorg#3005)

This PR introduces `amountOfStoredPools` param to Peer manager, that
will limit amount of stored pools. Defautlt value is 10 pools. Peer
manager will try to keep `amountOfStoredPools` amount of pools only for
recent headers. Older blocks don't need to be routed by manual pools and
could be routed to any full node, as those are expected to have the
block by that time.

This will lower memory footprint of peer manager as well as resolve
memory leaking issues.

Resolves celestiaorg#1781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:p2p kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(share/p2p/peer-manager): optimise peer manager memory footprint
4 participants