Update README.md #78
Update README.md #78
Conversation
Typos and adjustments that missed getting into #75
README.md
Outdated
| @@ -269,7 +269,7 @@ For Example, calling `Scale` with 4 GB of memory will result in a limit of 384 f | |||
|
|
|||
| The `FDFraction` defines how many of the file descriptors are allocated to this | |||
| scope. In the example above, when called with a file descriptor value of 1000, | |||
| this would result in a limit of 1256 file descriptors for the system scope. | |||
| this would result in a limit of ???1256 how is this calculated??? file descriptors for the system scope. | |||
There was a problem hiding this comment.
I think we should show the math on how this is calculated. I'm not remembering or quickly gleaning.
There was a problem hiding this comment.
Per later when looking at the code I think this is just 1000. I have a suggested change for it.
README.md
Outdated
| simply copy the default struct object and update the field you want to change. You can | ||
| apply changes to a `BaseLimit`, `BaseLimitIncrease`, and `LimitConfig` with | ||
| `.Apply`. | ||
|
|
||
| Example | ||
| ``` | ||
| // An example on how to tweak the default limits | ||
| // Is this correct? | ||
| tweakedDefaults := DefaultLimits | ||
| tweakedDefaults.ProtocolBaseLimit.Apply(BaseLimit{ |
There was a problem hiding this comment.
I want to confirm this is right.
I'm probably misunderstanding the go but looking at https://github.com/libp2p/go-libp2p-resource-manager/blob/master/limit.go#L102 I would have thought the limits don't apply since "l" has non-zero values.
Denote the memory units
Commenting on units and expected range of values for BaseLimitIncrease
There was a problem hiding this comment.
@MarcoPolo: good stuff moving this forward.
A few things:
- I couldn't just comment on arbitrary lines due to this GitHub limitation: Commenting on unchanged lines in a pull request github/roadmap#347 . Arg. As a result, I sometimes had to add ??? text so I could comment. I'll need you to clean this up.
- I think we're missing a "TLDR" / "Getting Started" at the top that shows how to instantiate ResourceManager with autoscaled defaults. The "Usage" section is buried down below and it doesn't discuss where "limits" comes from.
- Can we discuss from a design/implementation level and from a user perspective how the resource manager relates to the connmgr?
- When should one be used over the other?
- Limits set in one place aren't respect by the other (link to relevant backlog issue if that makes sense)
- This is implied if someone reads through the doc, but I think it's worth being explicit that the resource manager doesn't discriminate between honest peers and bad peers. It purely looks to see if the requested resource will exceed all the relevant scopes. The mechanisms for special handling for certain peers is with the allowlist limits and the connmgr if I understand right.
- I don't think we need to go into much detail, but in the top of the document where we go through all the scopes, we may want to say that "allowlist" is an additional scope that has special handling. We can link to the relevant docs for someone to learn more.
Please feel free to take over this PR, add commits, etc. I'm happy to review once changes are in or comments are addressed.
Thanks!
README.md
Outdated
| @@ -269,7 +269,7 @@ For Example, calling `Scale` with 4 GB of memory will result in a limit of 384 f | |||
|
|
|||
| The `FDFraction` defines how many of the file descriptors are allocated to this | |||
| scope. In the example above, when called with a file descriptor value of 1000, | |||
| this would result in a limit of 1256 file descriptors for the system scope. | |||
| this would result in a limit of 1000 (1000 * 1) file descriptors for the system scope. | |||
There was a problem hiding this comment.
| this would result in a limit of 1000 (1000 * 1) file descriptors for the system scope. | |
| this would result in a limit of 1256 (256 + 1000 * 1) file descriptors for the system scope. |
There was a problem hiding this comment.
@marten-seemann : are you sure? Looking at https://github.com/libp2p/go-libp2p-resource-manager/blob/master/limit_defaults.go#L332, I don't see us adding a base.
I think it's a good thing we're not adding a base because in that case we'd be using even more FDs than we allocated to libp2p. For example, it seemed odd to me reading this example that we said we had 1000 FDs for go-libp2p but then our limit was being set to 1256.
There was a problem hiding this comment.
it's 1k (and 376 for the the conns) tested with:
func TestReadmeExample(t *testing.T) {
scalingLimits := ScalingLimitConfig{
SystemBaseLimit: BaseLimit{
ConnsInbound: 64,
ConnsOutbound: 128,
Conns: 128,
StreamsInbound: 512,
StreamsOutbound: 1024,
Streams: 1024,
Memory: 128 << 20,
FD: 256,
},
SystemLimitIncrease: BaseLimitIncrease{
ConnsInbound: 32,
ConnsOutbound: 64,
Conns: 64,
StreamsInbound: 256,
StreamsOutbound: 512,
Streams: 512,
Memory: 256 << 20,
FDFraction: 1,
},
}
limitConf := scalingLimits.Scale(4<<30, 1000)
require.Equal(t, limitConf.System.Conns, 376)
require.Equal(t, limitConf.System.FD, 1000)
}
I think this is easier to understand since you can explain the behavior as: "For every GiB of memory allowed we increase the imit by LimitIncrease" Contrast this with the previous behavior of: "For every GiB of memory allowed we increase the imit by LimitIncrease, except that we first subtract 128MiB."
|
I'm changing the scale logic to be a bit simpler. The proposed change doesn't subtract 128 MiB before doing the scale logic. This means you can explain the behavior by saying: "For every GiB of memory allowed we increase the imit by LimitIncrease" Contrast this with the previous behavior of: "For every GiB of memory allowed we increase the imit by LimitIncrease, except that we first subtract 128MiB." The difference is subtle, but I think it's more intuitive. Consider also this graph of the two behaviors:
|
Agreed. |
|
r? @BigLep (I can't tag you as a reviewer of your own PR) |
|
This PR will need to be rebased onto go-libp2p, we just consolidated the resource manager: libp2p/go-libp2p#1677 |
|
Looks good to me @MarcoPolo . I assume you'll handle rebasing, merging, closing. Thanks again. |
|
Closing this PR to move it here: libp2p/go-libp2p#1684 |
Typos and adjustments that missed getting into #75