-
Notifications
You must be signed in to change notification settings - Fork 180
feat: size tracking flume channel #5857
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
Draft
hanabi1224
wants to merge
5
commits into
main
Choose a base branch
from
hm/size-tracking-flume-channel
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
dc9f610
feat: size tracking flume channel
hanabi1224 56d5a99
Merge remote-tracking branch 'origin/main' into hm/size-tracking-flum…
hanabi1224 0a58b92
Merge branch 'main' into hm/size-tracking-flume-channel
hanabi1224 34a2327
Merge remote-tracking branch 'origin/main' into hm/size-tracking-flum…
hanabi1224 4924742
Merge branch 'main' into hm/size-tracking-flume-channel
hanabi1224 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
💡 Verification agent
🧩 Analysis chain
Consider expanding
GetSize
implementation for comprehensive metricsThe current implementation only tracks heap size for
HelloResponseOutbound
events and returns 0 for all other variants. This might lead to incomplete memory usage metrics.Consider implementing proper size calculation for other message types that contain heap-allocated data (e.g.,
PubsubMessage
,ChainExchangeRequest
, etc.) to get more accurate memory usage metrics.Also applies to: 160-165
🏁 Script executed:
Length of output: 103
🏁 Script executed:
Length of output: 1424
🏁 Script executed:
Length of output: 521
Expand
GetSize
implementations to cover all heap‐backed variantsTo ensure accurate memory‐usage metrics, extend both
get_heap_size()
impls to account for every variant carrying heap data:• In
src/libp2p/service.rs
(impl forNetworkEvent
, lines 115–123):HelloResponseOutbound
arm.• In the same file (impl for
NetworkMessage
, lines 160–165):0
with amatch
that handles:Make sure that
PubsubMessage
,ChainExchangeRequest
, andNetRPCMethods
each implementGetSize
(or provide custom logic) so their heap allocations are correctly tallied.🤖 Prompt for AI Agents
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.
Right, GetSize impl is incomplete. Will fix in upcoming commits
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.
@hanabi1224, sounds good! Thanks for acknowledging the incomplete
GetSize
implementations. Looking forward to seeing the comprehensive size tracking in the upcoming commits.🧠 Learnings used