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

use the resource manager #1275

Merged
merged 8 commits into from
Jan 18, 2022
Merged

use the resource manager #1275

merged 8 commits into from
Jan 18, 2022

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Dec 30, 2021

Integrates the ResourceManager in go-libp2p; #1285 and #1289 have been squashed in.

TBD:

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

We need to do the circuit servers as well, at the stream level -- SetService and reserve the buffers.

p2p/protocol/circuitv2/client/transport.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann mentioned this pull request Jan 1, 2022
69 tasks
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

will open a pr on top for adding rcmgr support in the internal services.

p2p/net/mock/mock_conn.go Outdated Show resolved Hide resolved
p2p/net/mock/mock_peernet.go Outdated Show resolved Hide resolved
p2p/net/mock/mock_stream.go Outdated Show resolved Hide resolved
limits.go Show resolved Hide resolved
Comment on lines 70 to 80
// get a scope for memory reservations at service level
err := h.Network().ResourceManager().ViewService(ServiceName,
func(s network.ServiceScope) error {
var err error
r.scope, err = s.BeginTransaction()
return err
})
if err != nil {
return nil, err
}

Copy link
Member

Choose a reason for hiding this comment

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

This block looks like it'll become boilerplate. Is there a missing API here?

Copy link
Contributor

Choose a reason for hiding this comment

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

not really, most services will work at the stream level.

Copy link
Contributor

Choose a reason for hiding this comment

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

on the same time, it would be very easy to define a utility method to get a txn scope for a service, and it might eliminate some boilerplate.

Where should we put it though?

p2p/protocol/circuitv1/relay/relay.go Show resolved Hide resolved
p2p/protocol/circuitv1/relay/relay.go Outdated Show resolved Hide resolved
p2p/protocol/circuitv1/relay/relay.go Show resolved Hide resolved
p2p/protocol/ping/ping.go Show resolved Hide resolved
defaults.go Outdated
Comment on lines 90 to 91
// Default memory limit: 1/8th of total memory, minimum 128MB, maximum 1GB
limiter := rcmgr.NewStaticLimiter(0.125, 128<<20, 1<<30)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default libp2p node would use at max 1GB of memory? That seems a little low no? Although perhaps if this is just for things like connections and streams we don't have to worry too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the max for network buffers, which is what we are accounting.
I don't think that's too low, but we can certainly raise it.

limits.go Outdated
if _, ok := limiter.ServiceLimits[identify.ServiceName]; !ok {
limiter.ServiceLimits[identify.ServiceName] = limiter.DefaultServiceLimits.
WithMemoryLimit(1, 4<<20, 64<<20). // max 64MB service memory
WithStreamLimit(128, 128) // max 128 streams
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's identify going to do here if it needs to be run, but can't (e.g. I want to make more than 128 connections at once and each of those needs to run identify on start).

Copy link
Contributor

@vyzo vyzo Jan 14, 2022

Choose a reason for hiding this comment

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

we have stopped closing the connection if it fails (in master) and we'll also add some code for retrying a few times with exponential backoff.

limits.go Outdated Show resolved Hide resolved
limits.go Outdated
limiter.ServiceLimits[circuitv1.ServiceName] = limiter.DefaultServiceLimits.
WithMemoryLimit(1, 4<<20, 64<<20). // max 64MB service memory
WithStreamLimit(1024, 1024) // max 1024 streams
limiter.ServicePeerLimits[circuitv1.ServiceName] = peerSvcLimit(128)
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Is this for the relayv1 clients? I'd expect servers to have pretty high limits.
  2. Is this a little weird given that if I'm behind a relay it's possible almost all my connections will need to be relayed. Should the connmgr care about this and want to GC so we can make/receive more relayed connections?

Given that we're moving away from relayv1 this might not be so bad, but want to make sure we know + document any gotchas a user might run into

Copy link
Contributor

Choose a reason for hiding this comment

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

no, this is for the relayv1 server.

limits.go Outdated
limiter.ServiceLimits[circuitv2.ServiceName] = limiter.DefaultServiceLimits.
WithMemoryLimit(1, 4<<20, 64<<20). // max 64MB service memory
WithStreamLimit(1024, 1024) // max 1024 streams
limiter.ServicePeerLimits[circuitv2.ServiceName] = peerSvcLimit(128)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the client or server for relayv2? Should they be treated separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the server, the client we don't limit other than the default protocol limits.

Copy link
Contributor

Choose a reason for hiding this comment

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

and yeah, they are separate and should be treated separately as one is for our own functionality while the other is a service.

p2p/protocol/circuitv2/relay/relay.go Show resolved Hide resolved
p2p/protocol/ping/ping.go Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

ship it!

@marten-seemann marten-seemann merged commit 15d7dfb into master Jan 18, 2022
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.

4 participants