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

Speed up consensus::[d]encode significantly #264

Merged
merged 6 commits into from Jun 5, 2019

Conversation

Projects
None yet
5 participants
@TheBlueMatt
Copy link
Member

commented May 22, 2019

This makes the (obvious) improvement of swapping out repeated single-byte reads for a read_slice method. For my network crawler (https://github.com/TheBlueMatt/dnsseed-rust/) this took CPU usage down from pegging 4 ARM cores for not-that-many connections to something more manageable. There's tons of functions left to migrate, but this is a good start. Note that, long-term, I'd really, really like to limit the generics that support Encodable/Decodable. Notably, its really easy to accidentally call [T]'s Decode when you meant to call [T; 32]'s Decode (they do different things...).

Also slipped in adding SendHeaders support, cause I need it and don't feel like separating it into its own PR.

@dpc

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Yes. I could use some improvements here for Bitcoin Indexer! :)

@dpc

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

You could add couple of benches or something. Basically just export a random block from a node, and decode it (and then encode it back maybe).

@TheBlueMatt

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@dpc

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Does it make any difference when decoding a whole block stored in Vec<u8>?

@@ -553,21 +583,25 @@ impl<D: Decoder, T: Decodable<D>> Decodable<D> for Vec<T> {
}
}

impl<S: Encoder, T: Encodable<S>> Encodable<S> for Box<[T]> {

This comment has been minimized.

Copy link
@stevenroose

stevenroose May 23, 2019

Collaborator

So this is effectively a breaking change that doesn't allow encoding (boxed) slices of encodables anymore? Are you sure this doesn't affect certain users?

This comment has been minimized.

Copy link
@apoelstra

apoelstra May 23, 2019

Member

This is the least interesting of all the generic impls that are being dropped :)

This comment has been minimized.

Copy link
@stevenroose

stevenroose May 23, 2019

Collaborator

The argument that it's meant for consensus is quite a convincing one. We shouldn't support any types more than the ones ever used by consensus. And especially having very broad generic types (like [T]) prevents some users to implement such constructions using their own types.

@elichai

This comment has been minimized.

Copy link

commented May 23, 2019

@TheBlueMatt just want to make sure that your stats are done after compiling with --release,
because in my experience generics and complex rust types can be slow on debug and almost as fast as simple types in release

@TheBlueMatt

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@dpc I presume so, though didn't test.
@stevenroose indeed, its a breaking change, but I don't know that we really even want to allow anyone to "consensus::encode" their own types, its for consensus, that's why we have the trait, not for arbitrary types.
@elichai indeed, even with lto = true and codegen-units = 1.

@TheBlueMatt

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

Now also removes a bunch of unused Encodable/Decodable impls, in part to special-case Vec (cause then we can't have a Vec impl). This is the opposite direction from #262, but I'm really not a fan of the idea of that anyway.

@apoelstra

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Ok, after a long IRC discussion I'm on board with this. Can you add some commits though which

  • Renames Encoder to WriteExt and Decoder to ReadExt so it's obvious to users that these traits extend Write and Read respectively
  • Seals ReadExt, WriteExt, Encodable, Decodable by making them depend on a new non-exported empty Sealed trait.
  • Renames VarInt::encoded_length to VarInt::len and makes it return a usize

This will be mildly annoying for rust-elements, which will need to make its own copies of these traits, but it won't need to re-implement endianness/varint/bitcoin_hashes/vec<vec>/etc/etc/etc because it can just pass through to the rust-bitcoin versions of the crate.

@apoelstra
Copy link
Member

left a comment

ACK except for whatever's going on with Travis.

I'll make a followup PR with my suggestions.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-05-faster-enc-dec branch from cc86d14 to 8a4b24d May 25, 2019

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-05-faster-enc-dec branch from 8a4b24d to 2b6058e May 30, 2019

@apoelstra apoelstra merged commit 08c756d into rust-bitcoin:master Jun 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.