-
Notifications
You must be signed in to change notification settings - Fork 47
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
Changed multiaddr backing from Arc<Vec<u8>> to EcoVec<u8> #89
base: master
Are you sure you want to change the base?
Conversation
Thanks @imbrem for investing into rust-multiaddr performance characteristics. Can you please provide a benchmark that proves that this pull request improves performance? |
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. |
I can not think of a place where we manipulate an IPv4 address in a hot-path. |
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. |
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. |
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>
implementsWrite
, but if typst/ecow#23 lands, it can be removed.