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 Gossip excessive memory consumption by SeenCache #300

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

Nashatyrev
Copy link
Collaborator

@Nashatyrev Nashatyrev commented Aug 16, 2023

The SeenCache aims to effectively filter duplicate messages for a longer period (2 minutes by default) than mCache stores (5 seconds by default). The SeenCache implementations shouldn't retain references to received messages' content since it could be significant memory consumption in case of heavy gossip traffic:

image

The secondary purpose of the SeenCache is to cache messageId (which calculation could be resource consuming) of seen messages based of a fast message id (see SeenCache.getSeenMessageCached() method and FastIdSeenCache implementation)

To address the memory issue the SeenCache implementations should rely on message ids (either slow or fast or both) and don't keep the PubsubMessage references.

To preserve the secondary purpose the FastIdSeenCache.getSeenMessageCached() wraps the PubsubMessage with a wrapper instance which is supplied with the cached messageId when available.

Unfortunately there are no reliable means to create unit tests for memory leak/excessive usage cases

@Nashatyrev
Copy link
Collaborator Author

Basically the concept of messageId encapsulation to PubsubMessage should be revisited and probably refactored. Its current implementation is really confusing
I would remove the notion of messageId from the PubsubMessage as it basically should not be exposed to the client code.
The client code should just be able to supply the function of deriving messageId from a PubsubMessage instance during Gossip construction

@StefanBratanov
Copy link
Collaborator

LGTM

Copy link

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@Nashatyrev Nashatyrev merged commit 0ba9a90 into libp2p:develop Aug 17, 2023
2 checks passed
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.

3 participants