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: memory based connection limits #4281

Merged
merged 41 commits into from
Aug 8, 2023

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Aug 1, 2023

Description

Implements memory-based connection limits where the user can specify an absolute or a relative limit of the process' memory usage in relation to the available system memory.

Related: #4252.

TODO list:

  • Refactor connection_limits:Behaviour to take a generic ConnectionLimits type
  • Rename ConnectionLimits to StaticConnectionLimits
  • Implement MemoryBasedConnectionLimits
  • Disable the feature on unsupported platforms
  • Code docs and examples
  • Tests

Notes & open questions

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

@mxinden
Copy link
Member

mxinden commented Aug 1, 2023

Thank you for testing this out right-away @hanabi1224. Let me know once you would like a review.

@hanabi1224
Copy link
Contributor Author

@mxinden It will be great if you can take a quick look and let me know if it's on the right path. The code is done, while docs and tests are still missing

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

I'd like to discuss whether we should extend the existing libp2p-connection-limits here or create a new crate: libp2p-memory-connection-limits.

I think there a several arguments for a new crate:

  • It means that we have more design freedom in regards to the API and can still release it as a patch-release because it is just additions.
  • There isn't actually any shared code between those two. For example, the memory limits don't care about the connection kind at all. Tthe "put together what changes together" mantra also suggests that this should be separate.
  • A dedicated NetworkBehaviour will be able to make use of the poll function to register a timer for refreshing the memory stats. Alternatively, we could simply refresh them on every connection attempt (how expensive is gathering those stats?)

misc/connection-limits/src/connection_limits/memory.rs Outdated Show resolved Hide resolved
misc/connection-limits/src/connection_limits/memory.rs Outdated Show resolved Hide resolved
@hanabi1224
Copy link
Contributor Author

@thomaseizinger Thanks for your feedback, I can make it a new crate.

mxinden and others added 9 commits August 3, 2023 20:02
Explicitly set `libp2p-kad` `Kademlia::set_mode` to `Mode::Server` to reduce complexity of example, i.e. not having to explicitly set external addresses.

Pull-Request: libp2p#4197.
`OutboundQueryCompleted` hasn't been fully replaced by `OutboundQueryProgressed` in kad doc.

Pull-Request: libp2p#4257.
`libp2p-relay` crate specifies the `libp2p-swarm/async-std` feature, which causes an `async-std` dependency to always be introduced.

Pull-Request: libp2p#4283.
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! Some suggestions :)

misc/memory-connection-limits/CHANGELOG.md Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/connection_limits.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/connection_limits.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
@hanabi1224
Copy link
Contributor Author

Thanks! Some suggestions :)

@thomaseizinger Thanks! I've just added 2 unit tests for max_bytes and max_percentage scenarios, please let me know if that's sufficient.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Great progress! I've added some more comments :)

misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/tests/util.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/tests/util.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/tests/max_percentage.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/tests/max_bytes.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Great work! Two more comments :)

misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
misc/memory-connection-limits/src/lib.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

I pushed some follow-up commits, hope you don't mind :)

@hanabi1224 hanabi1224 marked this pull request as ready for review August 7, 2023 12:55
thomaseizinger
thomaseizinger previously approved these changes Aug 7, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

This is ready from my PoV but I've also pushed some commits to this so I'll defer to @mxinden for a final review :)

@thomaseizinger
Copy link
Contributor

One more thing: We should add this to the libp2p meta crate and re-export it from there. Plus linking to it from the main changelog would be great! :)

@hanabi1224
Copy link
Contributor Author

One more thing: We should add this to the libp2p meta crate and re-export it from there. Plus linking to it from the main changelog would be great! :)

Done!

libp2p/CHANGELOG.md Outdated Show resolved Hide resolved
mxinden
mxinden previously approved these changes Aug 7, 2023
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @hanabi1224 and @thomaseizinger!

misc/memory-connection-limits/tests/util.rs Show resolved Hide resolved
@mergify mergify bot dismissed stale reviews from thomaseizinger and mxinden August 8, 2023 12:10

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@thomaseizinger
Copy link
Contributor

@hanabi1224 No need to manually update the PR once it is queued (i.e. send-it label has been applied). See https://github.com/libp2p/rust-libp2p/blob/master/CONTRIBUTING.md#merging-of-prs-is-automated.

@mergify mergify bot merged commit b5d9932 into libp2p:master Aug 8, 2023
@hanabi1224 hanabi1224 deleted the memory-based-connection-limits branch August 9, 2023 08:40
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
Implements memory-based connection limits where the user can specify an absolute or a relative limit of the process' memory usage in relation to the available system memory.

Related: #4252.

Pull-Request: #4281.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants