search

multiformats / rust-multiaddr

multiaddr implementation in rust
https://crates.io/crates/multiaddr
Other
88 stars 47 forks source link

Changed multiaddr backing from Arc<Vec<u8>> to EcoVec<u8> #89

Open imbrem opened 1 year ago

imbrem commented 1 year ago

While this increases the size of a multiaddr on the stack or in a container (from 1 word to 2 words), it decreases the overall memory usage of the multiaddr (from 2 allocations to 1 allocation) and avoids a double dereference when manipulating multiaddrs. Hence, I believe that this should be a win for most applications.

This PR currently includes a hack so that EcoVec<u8> implements Write, but if https://github.com/typst/ecow/pull/23 lands, it can be removed.

mxinden commented 1 year ago

Thanks @imbrem for investing into rust-multiaddr performance characteristics.

Can you please provide a benchmark that proves that this pull request improves performance?

imbrem commented 1 year ago

I added some basic benchmarks in the branch https://github.com/imbrem/rust-multiaddr/tree/multiaddr-benches; these indicate a 25% performance improvement in creating and cloning ipv4 addresses, and about the same performance for ipv6 addresses. The biggest performance improvements should be in manipulating ipv4 addresses, but making a benchmark for this is more complicated; I should be able to find some time to do this in a few days.

mxinden commented 1 year ago

The biggest performance improvements should be in manipulating ipv4 addresses

I can not think of a place where we manipulate an IPv4 address in a hot-path.

imbrem commented 1 year ago

If you want I can try to benchmark memory usage, which should be significantly improved as we eliminate the Arc allocation holding the Vec. Another option is to modify EcoVec to have a longer buffer so it can hold an ipv6 address inline or more; this would leave memory the same as the original (since the larger EcoVec would have a larger storage footprint) but now more multiaddrs would share in the performance improvement. Issue is I’d probably have to fork EcoVec in the latter case since it adds a lot to the complexity.

mxinden commented 1 year ago

If you want I can try to benchmark memory usage, which should be significantly improved as we eliminate the Arc allocation holding the Vec.

You could test with some downstream application using rust-libp2p, e.g. https://github.com/mxinden/kademlia-exporter/. Maybe the change here shows up in heaptrack of a CPU flamegraph.