Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

imbrem
Copy link

@imbrem imbrem commented Apr 26, 2023

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 typst/ecow#23 lands, it can be removed.

@mxinden
Copy link
Member

mxinden commented Apr 27, 2023

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

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

@imbrem
Copy link
Author

imbrem commented Apr 27, 2023

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
Copy link
Member

mxinden commented May 9, 2023

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
Copy link
Author

imbrem commented May 9, 2023

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
Copy link
Member

mxinden commented May 16, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants