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

store Multiaddrs as strings and don't use interfaces #58

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

  1. This saves one level of indirection, one distinct allocation, and a few bytes.
  2. This allows Multiaddrs to be used as map keys (we can use this to make our Peerstore more efficient).

This commit isn't as efficient as it could be. We could improve it slightly by using unsafe casts to temporarily treat strings as bytes. However I'm not sure this will improve performance much and, in general, this change will lead to fewer allocations overall.

TL;DR: We have lots of these, make them small and efficient.

1. This saves one level of indirection, one distinct allocation, and a few bytes.
2. This allows Multiaddrs to be used as map keys (we can use this to make our
Peerstore more efficient).

This commit isn't as efficient as it could be. We could improve it slightly by
using unsafe casts to temporarily treat strings as bytes. However I'm not sure
this will improve performance much and, in general, this change will lead to
fewer allocations overall.
// String returns the string representation of this Multiaddr
// (may panic if internal state is corrupted)
func (m Multiaddr) String() string {
s, err := bytesToString(m.Bytes())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're going down this path, we might has well have bstrToString

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd just have to call Bytes() inside it as we can't extract varints from strings.

@Stebalien
Copy link
Member Author

So, the really annoying (probably deal breaker) thing here is that Multiaddr would no longer have a nil value (just an empty Multiaddr{} value). This breaks lots of code.

@whyrusleeping
Copy link
Member

I'm not actually sure how this is supposed to save us a lot of memory.

@whyrusleeping
Copy link
Member

I mean, i get map keys, but we're going to have to do a lot more allocations as a result of this (every call to .String() now allocates a full byte slice, as well as the string its generating)

@Stebalien
Copy link
Member Author

We shouldn't have to do that many more allocations:

  1. We already copied when calling Bytes().
  2. That call to String() makes multiple allocations (it builds the string). One more won't hurt. Also, unless I'm mistaken, we don't use String() in performance critical code. If it does become an issue, we can do some unsafe magic to cast the bstr to a byte slice temporarily.

My main concern here was saving space so we didn't have to store each multiaddr twice (once as a map key, once as a map value). However, after profiling again, I'm pretty sure that's not the main issue. The issue appears to be that maps are wasteful (to avoid collisions) and we shouldn't be storing lots of little maps. I'm working on a patch to store multiaddrs in sorted slices instead of maps.

So yeah, I'm going to close this PR. I'd still like to avoid having a pointer to a pointer but we can do that by defining type Multiaddr []byte instead (and we get the added bonus that nil still works so we don't break everything).

@Stebalien Stebalien closed this Oct 20, 2017
@Stebalien
Copy link
Member Author

Note: we should be able to store records for >10,000 peers in ~6MiB (assuming 5 addresses per peer) so there is definitely room for improvement here before we have to fall back on writing stuff to disk.

Stebalien added a commit that referenced this pull request May 20, 2020
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