Fix: Ignore zero values when marshalling Limits.

[ajnavarro]

Ignoring zero values when marshaling zero values as it's done here https://github.com/libp2p/go-libp2p/blob/master/p2p/host/resource-manager/limit_defaults.go#L111

Outputs will change from:

"Transient": {
      "Conns": 10,
      "ConnsInbound": 0,
      "ConnsOutbound": 0,
      "FD": 0,
      "Memory": 0,
      "Streams": 0,
      "StreamsInbound": 0,
      "StreamsOutbound": 0
}

to:

"Transient": {
      "Conns": 10
}

We need to confirm what zero means for Resource Manager. My understanding after asking @marten-seemann is that zero means zero, not infinite or not set.

If zero means zero, we need to specify a way to define no limits for some of the fields inside a BaseLimit or a BaseLimitIncrease object. We can use -1 or use pointers to allow nil values.

[MarcoPolo]

We need to confirm what zero means for Resource Manager. My understanding after asking @marten-seemann is that zero means zero, not infinite or not set.

Zero means zero when inside the final struct used by go-libp2p. For example, when I start up a simple go-libp2p server and set GOLOG_LOG_LEVEL=rcmgr=debug I see the limit config that go-libp2p uses. There are only zeros where it's unused. For example, Protocols have limits for the number of streams, but 0 for connections. They aren't consulted about number of connections since it's not really related to the protocol. In this case, it's still a zero value but it's just not used when checking to see if we can accept a connection or not.

When reading from JSON or Applying, zero means not set and it should use the default/other-value.

This is pretty confusing. And I think we should have a way of defining not-set vs zero that is uniform across serialization.

If zero means zero, we need to specify a way to define no limits for some of the fields inside a BaseLimit or a BaseLimitIncrease object. We can use -1 or use pointers to allow nil values.

No limits can be MaxInt32 and that should work fine. Is there some reason to prefer a special value?

[MarcoPolo]

I think this change is pretty safe since it doesn't affect the semantics of the unmarshalled struct. What does this fix for you?

I'm thinking of adding a "IsSetFlags" bit field to BaseLimit and BaseLimit increase which will make it explicit if a field is unset and you would want a default override.

[ajnavarro]

Hi @MarcoPolo! Answering your questions:

No limits can be MaxInt32 and that should work fine. Is there some reason to prefer a special value?

This is confusing for the final user. They think that they did something wrong, or that Kubo has a bug and the config overflowed.

I think this change is pretty safe since it doesn't affect the semantics of the unmarshalled struct. What does this fix for you?

It removes some confusion for the final user. If the user changes from Kubo CLI some RM param, there is no way to avoid serializing these other values as zeroes. And now that we know that zero means two different things depending on where the struct is read, it makes even more sense to apply this change to reduce the confusion.

I'm thinking of adding a "IsSetFlags" bit field to BaseLimit and BaseLimit increase which will make it explicit if a field is unset and you would want a default override.

Why not just use the language? If we use pointers, it is more idiomatic and easy to understand for the final user modifying the configuration, and for the developer implementing libp2p RM into other apps. IMHO, if a value is set to nil, should mean no limits for that value, and the upper scope will handle it.

[MarcoPolo]

If we use pointers, it is more idiomatic and easy to understand

I originally thought that pointers would be needlessly slow for this since it would involve a pointer dereference every time you check a limit. I decided to fact check that thought by writing a quick benchmark (the benchmark is a bit contrived since I'm trying to avoid certain optimizations and avoid hitting cached L1/2 pointer values). The benchmark shows that while there is a bit of performance gain with flags, it's negligible. Especially with compiler optimizations. And it's a nicer experience to have a single value control whether something is set (nil or a number). So I'm in favor of using pointers here :)

if a value is set to nil, should mean no limits for that value, and the upper scope will handle it.

I'm not sure I agree. Then there are two ways of defining a "big enough" value and no way of defining a "please use a default" value. I think nil should mean "this is unset, I don't care what this is, please use a default value", and a big number is how you define a big enough value.

If you want this to appear simpler in the config, would it be better to write "Infinity" in the config and parse "Infinity" as a big enough value? Maybe we can make another type that would marshal as "Infinity" when MaxInt is used.

[BigLep]

Oof - so are we doing anything extra to make this even clearer (beyond what was done in this PR)?

An issue I'm seeing here is that at least in the Kubo usecase of computing default limits and then applying user-supplied overrides is that there isn't a way for a user to set a limit to zero. If they do, it gets overridden by the computed default.

Can we handle this better?

[BigLep]

Here's a PR in PR to at least make clear that we don't currently support 0-value user-supplied overrides: ipfs/kubo#9563

[ajnavarro]

I'm not sure I agree. Then there are two ways of defining a "big enough" value and no way of defining a "please use a default" value. I think nil should mean "this is unset, I don't care what this is, please use a default value", and a big number is how you define a big enough value.

Fair enough :)

If you want this to appear simpler in the config, would it be better to write "Infinity" in the config and parse "Infinity" as a big enough value? Maybe we can make another type that would marshal as "Infinity" when MaxInt is used.

Sounds good.

[lidel]

Continued in ipfs/kubo#9564

[MarcoPolo]

The changes around the config are in #2000