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: add resource manager for separate DHT libp2p host #54

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Dec 20, 2023

Seems like we need a separate resource manager for the DHT host if it's separate. For now I just reused the limits, but that's obviously not right since that would put the default memory usage at 0.85 * 2 of total memory and 0.5 *2 of total FDs.

Some notes:

  • We should be able to significantly throttle inbound streams and connections
  • We could separate various flags (connmgr, memory, fd) flags for the new host if we wanted, but either way we need some sensible default percentages to allocate
  • For some reason running before Add peering support #35 does not result in resource manager errors from the accelerated DHT client (perhaps related to smart dialing?), but this change seems needed anyway

Thoughts?

@hsanjuan
Copy link
Contributor

Have we established that running 2 hosts is better? I keep forgetting that there are 2. We should settle on one or 2.

@aschmahmann aschmahmann force-pushed the feat/separate-dht-host-resource-mgr branch from d1d1827 to 9ac2e2b Compare December 22, 2023 20:09
@aschmahmann
Copy link
Contributor Author

Have we established that running 2 hosts is better? I keep forgetting that there are 2. We should settle on one or 2.

I think so, but would like @ns4plabs or @acejam to confirm when they have a chance.

@@ -15,6 +16,39 @@ import (

var infiniteResourceLimits = rcmgr.InfiniteLimits.ToPartialLimitConfig().System

func makeResourceMgrs(maxMemory uint64, maxFD int, connMgrHighWater int, separateDHT bool) (bitswapHost, dhtHost network.ResourceManager, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes here might be overkill and is pretty guesstimated, just wanted something other than disabling the resource manager for people to test with.

Recommendations appreciated 🙏

@BigLep
Copy link
Contributor

BigLep commented Jan 5, 2024

2024-01-05 conversation:

  1. In the ipfs.io rainbow deployment, are we seeing these errors? (@ns4plabs said no but we should double check here)
  2. test whether shared DHT has better performance (looking at CPU/memory utilization, TTFB) on the ipfs.io gateway staging

Wny this matters:
If we are seeing errors, then routing resolution will be worse.

If we don't see any errors here, then the priority of this goes down.

Note that we'll only see the errors closer to node startup.

Next step: Get @aschmahmann access to logs and a record of what commit was deployed when.

@aschmahmann aschmahmann force-pushed the feat/separate-dht-host-resource-mgr branch 4 times, most recently from c512e2e to 32d7e58 Compare February 13, 2024 15:57
@aschmahmann aschmahmann force-pushed the feat/separate-dht-host-resource-mgr branch from 32d7e58 to 5eae981 Compare February 13, 2024 15:57
@aschmahmann aschmahmann marked this pull request as ready for review February 13, 2024 15:57
@hacdias hacdias merged commit 361f35b into main Feb 13, 2024
10 checks passed
@hacdias hacdias deleted the feat/separate-dht-host-resource-mgr branch February 13, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants