Use inline storage for small hashes

[rklaehn]

This also gets rid of the Bytes dependency and replaces it with an Arc<[u8]>.

Implementation for #46

Note that the size of this new multihash is 40, where as the size of the old Bytes based multihash is 32, plus the stuff on the heap. I think this is a pretty good tradeoff!

[dignifiedquire]

I like this approach, though I still wonder if just using smallvec and letting the user wrap in an arc if needed wouldn’t make for a nicer separation of concerns,

[dignifiedquire]

why 39?

[rklaehn]

So the total size is 40, a multiple of 8. 1 byte is needed for the enum discriminator.

[rklaehn]

39 fits all 256 bit hashes with some room to spare. 512 byte hashes won't work though, and I think once you go to such large hashes, you are better off with an Arc<[u8]>.

But the size of the inline buffer can of course be adjusted when even larger hashes become common. The whole mechanism is completely opaque.

[rklaehn]

@dignifiedquire smallvec has an overhead of one usize for storing the size https://github.com/servo/rust-smallvec/blob/master/lib.rs#L374 , which is not necessary because the hash already knows how big it is. It also has the overhead of the discriminator, unless you use the "union" feature. https://github.com/servo/rust-smallvec/blob/master/lib.rs#L292, which is disabled by default: https://docs.rs/smallvec/1.2.0/smallvec/#union-feature . Due to the alignment rules, this discriminator costs you 8 bytes on 64 bit arch. So in total an overhead of 16 bytes.

But most of all, it is a dependency containing unsafe code that can cause problems for others and that is not needed for a few LOC.

[burdges]

There is an old smallbox crate but comes with a silly inefficiency that's worse than smallvec here.

I'd think fix SmallBox<T, Space> with an inner union that discriminates based on size_of::<T>() < size_of::<Space>(), except..

I presume smallbox really exists to make SmallBox<dyn Trait, Space> work. Ideally, you'd make two vtables for dyn Trait that distinguished between smaller and larger than Space, but this would require deep rustc integration.

It appears possible to do this manually for individual traits, like we really could make a no_std no_alloc compatible SmallError that only takes 16 bytes and lets std::io traits work without std or alloc, so long as error types stay smaller than usize: rust-lang/rfcs#2820 (comment)

It's possible some #[dyn_smallbox] procmaco could abstract this over traits with the current rustc, but nobody did one.

Apologies for the derail, but I'm agreeing with you that smallbox optimizations become messy and custom, especially whenever you want dyn Trait to work.

I'm also however pointing out that alloc being optional comes with some benefits, although perhaps not for multihash.

[rklaehn]

@burdges interesting, but I think in this case just doing the branching manually is best. It is quite simple code.

[rklaehn]

Thought about this some more.

• Given that the average 256 bit hash is 32 bytes, 34 bytes with metadata, there is enough room to store the length separately in 40 bytes to avoid this awkward recomputation of the length from the content. Will update the PR. Then the storage is just an efficient encoding of an arbitrary Vec, giving better separation of concerns.
• The way Bytes was used here, there is really no benefit in Bytes over Arc<[u8]>.

[vmx]

Nice!

I've run this code against Vec and Bytes based on the use case the libp2p has: https://gist.github.com/vmx/0dc0d981b522e2834084559faa0dcb56

On my machine this code code is always faster or similar (never slower).

[rklaehn]

@vmx thanks for the benchmark. Criterion is pretty nice.

For small hashes that fit, the improvement is quite good. I think we should give multiaddr the same treatment once this is merged...

[vmx]

@twittner Could you please have a look at this PR? You were in favour for using Bytes. Would this change for for your use case in libp2p?

[twittner]

Looks like a nice improvement to me.

[vmx]

@twittner Did all your concerns get addressed?