rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

build(deps): Update lru to 0.8.0

Open thomaseizinger opened this issue 3 years ago • 5 comments

Description

This is a replacement for https://github.com/libp2p/rust-libp2p/pull/2889. With lru version 0.8.0, we can no longer pass a capacity of 0 to it which is why we write our own wrapper that can disable the cache internally.

Links to any relevant issues

  • https://github.com/libp2p/rust-libp2p/pull/2889

Open Questions

Change checklist

  • [x] I have performed a self-review of my own code
  • [x] I have made corresponding changes to the documentation
  • ~[ ] I have added tests that prove my fix is effective or that my feature works~
  • [x] A changelog entry has been made in the appropriate crates

thomaseizinger avatar Sep 16 '22 01:09 thomaseizinger

How would one disable the caching logic entirely with the NonZeroUsize? I could imagine a use-case where someone does not want libp2p-identify to be involved in the address selection, but does want another NetworkBehaviour to contribute to the list of addresses.

Haven't thought of that. Can the caching actually be harmful? We try all sorts of addresses for a peer when we dial them, a few more or less don't seem to be a problem?

We could make the cache optional or wrap it in our own type so that it can still act as a no-op in case it is disabled.

Regarding a default value of 10, what is the rational here? I would say most of our default values are optimized for laptops and servers. With a maximum identify payload of 4096 byte I would be fine with caching >= 100 peers, which would still keep the upper bound of the cache below a mega byte. What do you think?

No rationale other than just picking a number but you are right, it will probably be a lot more useful if we up the cache size. Can also add a comment that explains that even with 100 peers, this should be well under a megabytes of memory usage.

thomaseizinger avatar Sep 22 '22 09:09 thomaseizinger

Would you like it to be possible for the cache to be disabled?

thomaseizinger avatar Sep 22 '22 09:09 thomaseizinger

Would you like it to be possible for the cache to be disabled?

I would be in favor of it. Though more as an intuition and the expectation that there are some crazy implementations of Networkbehaviour out there.

mxinden avatar Sep 22 '22 16:09 mxinden

@mxinden I ended up not doing to the change to the default cache size in here so that this is a clean dependency update without a functional change. This allows us to consider the cache size change individually.

thomaseizinger avatar Sep 23 '22 03:09 thomaseizinger

I opened an issue here: https://github.com/libp2p/rust-libp2p/issues/2933

thomaseizinger avatar Sep 23 '22 03:09 thomaseizinger