-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
rcmgr: consider removing memory tracking, except for muxers #1708
Comments
Going one step further, one could argue that this is purely a yamux (and mplex?) problem, and that reasonable stream multiplexers will have a connection-level flow control window. For QUIC, we use 15 MB per connection: go-libp2p/p2p/transport/quic/transport.go Line 45 in 86aad88
At reasonable numbers of connections (a few 100), this shouldn't cause any problems except on the very weakest of nodes. |
thats a step backwards and premature optimizatio i think. |
We need to go the other way and actually build a resource manager that can be used by applications (ideally one that doesn't live in libp2p, but that is shared by go-ipfs, go-libp2p, and anything else). Libp2p itself needs blocking allocation and oversubscription so we can actually utilize system resources.
We can create 200 connections in a single dht query (we need to fix that, but that's another issue). Bursting up to 1k connections is normal, and we should support thousands of connections. We can't just say "well, each connection takes 15MB, so 1k connections requires 15GiB of memory". That's absurd and would make libp2p unusable. |
I don't necessarily disagree with this. It would be nice to have a component that does this.
We already don't. Once we introduce autoscaling limits (#48), the connection limit for the weakest node (assumed to have no more than 1 GB of RAM) will be 128, scaling up to much higher values as available system memory increases. |
prologue caveat: I don't think removing memory tracking is a high pri issue. It's fine as is, we can also tweak this later and hopefully with more experience (e.g. do folks end up using it and loving it? keep; after 1 year does nobody use it? probably prune). I generally agree with Marten here. Especially the part where it doesn't make sense that libp2p tries to manage resources of the whole application. I, as an application developer, don't want to jump through hoops to have my application use libp2p. However, libp2p should make sure that it uses a reasonable amount of memory itself (I think both marten & steven agree with this). Libp2p owns streams and owns connections. It should make sure the memory usage of those owned resources doesn't balloon. 15GiB of memory for 1k connections is crazy town. I Agree with Steven that 1k burst connections is normal, here's my node that is doing nothing and hit 1k connections. I haven't thought too much on what the correct solution is here, but it's probably a mix of letting the user limit their number of connections and globally scale resource usage down amongst all connections to stay within limits (maybe even be smart and if the signal latency is low, you can reduce your window size). What makes more sense to me is to rely on existing tools and patterns for managing resources. However there doesn't seem to be a great story around this in Go. See: golang/go#14162, vitessio/vitess#8897. But maybe golang/go#48409 would help here.
Good point. |
Quick clarification, we don't allocate the max window buffer at the start. The 1k connections won't use 15GiB of memory under normal circumstances. This would only happen if all 1k connections were sending us a lot of data with high latency, which is probably the exact case we want to protect against. |
Having looked through traces / rcmgr metrics, the only significant (tracked) consumer of memory in our stack are the stream multiplexers. The only memory allocations outside of the stream muxers are allocations of a few kB to parse protobufs.
One might argue that this only shows that more application protocols need to adopt the resource manager and in order to track every memory allocation. I'm not convinced by that argument:
We should therefore consider dropping the memory tracking from all scopes, expect:
Thoughts, @MarcoPolo @vyzo @Stebalien?
The text was updated successfully, but these errors were encountered: