rlp serialization refactor #4873

debris · 2017-03-11T19:05:32Z

for now, it's only serialization refactor. it's still WIP, but wanted to know, if you like interface changes I made:

changes:

removed BytesEncodable trait
removed Encodable trait
serialization do not use ToBytes
Encodable is no longer implemented to T where T: ToBytes
RlpStream does not implement Stream trait. All functions where moved to public interface. I find additional Stream import to be really annoying
used byteorder for serialization
new api method append_list which should be used to append list of encodable items
new api method append_internal which should be used for wrapper types to encoding item without incrementing list elements counter
serialization of lists preallocates 1 additional byte for payload. This allows us avoid some unnecessary shifts for short lists

benchmarks:

old

test bench_stream_1000_empty_lists   ... bench:       9,839 ns/iter (+/- 1,207)
test bench_stream_nested_empty_lists ... bench:         223 ns/iter (+/- 23)
test bench_stream_u256_value         ... bench:         453 ns/iter (+/- 52)
test bench_stream_u64_value          ... bench:         119 ns/iter (+/- 16)

new (update)

test bench_stream_1000_empty_lists   ... bench:       9,143 ns/iter (+/- 1,209)
test bench_stream_nested_empty_lists ... bench:         181 ns/iter (+/- 22)
test bench_stream_u256_value         ... bench:         344 ns/iter (+/- 53)
test bench_stream_u64_value          ... bench:          90 ns/iter (+/- 10)

so encoding integers is almost 25% faster

rphmeier · 2017-03-11T19:31:18Z

util/rlp/src/stream.rs

+		stream
+	}
+
+	pub fn append<'a, E>(&'a mut self, value: &E) -> &'a mut Self where E: RlpEncodable {


One nice change we could make would be to impl on [u8] instead of &'a [u8] (likewise for str), and then do E: RlpEncodable + ?Sized so we don't have to double-reference slices (which currently leads to lots of &&*my_vec in the codebase)

It will be included in separate pr, this one is big enough ;)

keorn · 2017-03-13T10:46:21Z

ethcore/src/engines/tendermint/message.rs

@@ -162,7 +162,8 @@ impl Decodable for Step {

 impl Encodable for Step {
 	fn rlp_append(&self, s: &mut RlpStream) {
-		RlpEncodable::rlp_append(&self.number(), s);
+		//s.append(&self.number());


Old comment.

keorn · 2017-03-13T10:46:31Z

ethcore/src/engines/tendermint/message.rs

@@ -278,6 +279,8 @@ mod tests {
 			::rlp::encode(&H520::default()).to_vec(),
 			Vec::new()
 		];
+
+		println!("seal: {:?}", seal);


unnecessary print

rphmeier · 2017-03-14T10:22:41Z

util/rlp/src/stream.rs

+	finished_list: bool,
+}
+
+impl Default  for RlpStream {


double space after Default

arkpar · 2017-03-14T17:03:34Z

ethcore/light/src/net/status.rs

@@ -16,7 +16,7 @@

 //! Peer status and capabilities.

-use rlp::{DecoderError, RlpDecodable, RlpEncodable, RlpStream, Stream, UntrustedRlp, View};
+use rlp::{DecoderError, RlpDecodable, Encodable, RlpStream, UntrustedRlp, View};


Why was RlpEncodable renamed, but not the rest?

RlpDecodable was renamed accordingly in the next pr. The rest of data structures names remained unchanged. I didn't want to introduce to many changes in a single pr

arkpar · 2017-03-14T17:31:22Z

util/rlp/src/impls.rs

+	}
+}
+
+impl<T> Encodable for Option<T> where T: Encodable {


This is dangerous and should be removed IMO. Why does Option has to be a list?

It's not a question to me. It's already like that on master,

https://github.com/ethcore/parity/blob/8a67a0a80aae506233f65cc41962fe483fc10c15/util/rlp/src/rlpstream.rs#L340-L350

I know. Might be a good time to get rid of it.

I will file an issue and fix in the next pr

how else would you encode an Option in RLP?

Manually. My point is that changing e.g. a struct member type T to Option<T> silently changes encoding of that member to a list. I recall fixing a bug caused by this.

arkpar · 2017-03-14T17:52:05Z

lgtm

NikVolf · 2017-03-14T18:38:20Z

util/rlp/src/impls.rs

+			fn rlp_append(&self, s: &mut RlpStream) {
+				let leading_empty_bytes = self.leading_zeros() as usize / 8;
+				let mut buffer = [0u8; $size];
+				(&mut buffer as &mut [u8]).$func::<BigEndian>(*self).expect("buffer.len() == sizeof(*self); qed");


can be just
BigEndian::$func(&mut buffer, *self);

NikVolf · 2017-03-14T18:39:07Z

util/rlp/src/stream.rs

+		let leading_empty_bytes = size.leading_zeros() as usize / 8;
+		let size_bytes = 4 - leading_empty_bytes as u8;
+		let mut buffer = [0u8; 4];
+		(&mut buffer as &mut [u8]).write_u32::<BigEndian>(size).expect("buffer.len() == sizeof(value); qed");


can be just
BigEndian::write_u32(&mut buffer, size)

gavofyork · 2017-03-14T21:34:02Z

reduction of code size and number of traits is good.

addition of _list everywhere less so.

NikVolf · 2017-03-14T22:07:00Z

lgtm

gavofyork · 2017-03-15T12:44:24Z

is it the case that the only underlying reason for the append/append_list API divergence is the need to distinguish between Vec<u8> as a single piece of data and as a list of numbers?

debris · 2017-03-15T12:47:21Z

yes

gavofyork · 2017-03-15T13:01:03Z

i seem to remember there was something we could do with traits, no? Have a Listable trait which is good for all types except u8?

debris · 2017-03-17T22:31:57Z

i seem to remember there was something we could do with traits, no? Have a Listable trait which is good for all types except u8?

Nothing less complicated than what we had before (with at least 3 different traits) comes to my mind. And I think the solution from this pr is less complicated and error prone

gavofyork · 2017-03-19T07:42:00Z

well the fact i now have two different functions to call in order to do the same conceptual thing, depending on some arbitrary distinction between datatypes, adds to the complexity.

that said, i won't block it if y'all prefer this way of doing it.

debris · 2017-03-20T10:23:19Z

thanks, @arkpar @NikVolf @rphmeier @tomusdrw @keorn ?

debris added 2 commits March 11, 2017 16:37

fixed naming of rlp modules

f96c025

RlpStream cleanup

0e16e2c

debris added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Mar 11, 2017

debris requested a review from rphmeier March 11, 2017 19:05

rphmeier reviewed Mar 11, 2017

View reviewed changes

debris added 5 commits March 12, 2017 10:44

appending short rlp lists (0...55 bytes) is 25% faster

4eabe95

RlpStream does not use bytes module, nor trait Stream

bd74566

removed unused code from rlp module

84b96fe

compiling ethcore-util with new rlp serialization

d8c15ea

compiling parity with new rlp serialization

c77bf65

debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 12, 2017

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

e459599

debris requested a review from arkpar March 12, 2017 13:15

fixed compiling ethcore-light with new rlp serialization

79f74bb

debris changed the title ~~rlp refactor~~ rlp serialization refactor Mar 12, 2017

debris added 2 commits March 12, 2017 16:31

fixed compiling ethsync with new rlp serialization

25a95e4

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

78079c8

keorn reviewed Mar 13, 2017

View reviewed changes

debris added 2 commits March 13, 2017 14:45

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

cd817c9

removed redundant comment, print

7325e48

rphmeier reviewed Mar 14, 2017

View reviewed changes

debris added 2 commits March 14, 2017 11:52

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

d2b2644

removed redundant double-space

5da1539

debris mentioned this pull request Mar 14, 2017

second (and last) part of rlp refactor #4901

Merged

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

b5ba53c

arkpar reviewed Mar 14, 2017

View reviewed changes

arkpar requested a review from gavofyork March 14, 2017 17:41

debris mentioned this pull request Mar 14, 2017

Remove Encodable for Option<T> #4910

Closed

NikVolf reviewed Mar 14, 2017

View reviewed changes

NikVolf added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 14, 2017

debris added 2 commits March 14, 2017 22:59

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

d3710c7

replace usage of WriteBytesExt with ByteOrder

ce4776a

debris added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Mar 14, 2017

Merge branch 'master' of github.com:ethcore/parity into rlp_cleanup

091ccfa

gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 19, 2017

NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Mar 20, 2017

gavofyork merged commit a555686 into master Mar 20, 2017

gavofyork deleted the rlp_cleanup branch March 20, 2017 18:14

debris restored the rlp_cleanup branch July 31, 2017 14:15

debris deleted the rlp_cleanup branch July 31, 2017 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rlp serialization refactor #4873

rlp serialization refactor #4873

debris commented Mar 11, 2017 •

edited

rphmeier Mar 11, 2017

debris Mar 12, 2017

keorn Mar 13, 2017

keorn Mar 13, 2017

rphmeier Mar 14, 2017

arkpar Mar 14, 2017

debris Mar 14, 2017

arkpar Mar 14, 2017

debris Mar 14, 2017

arkpar Mar 14, 2017

debris Mar 14, 2017

gavofyork Mar 14, 2017

arkpar Mar 15, 2017

arkpar commented Mar 14, 2017

NikVolf Mar 14, 2017

NikVolf Mar 14, 2017 •

edited

gavofyork commented Mar 14, 2017

NikVolf commented Mar 14, 2017

gavofyork commented Mar 15, 2017

debris commented Mar 15, 2017

gavofyork commented Mar 15, 2017

debris commented Mar 17, 2017

gavofyork commented Mar 19, 2017

debris commented Mar 20, 2017

rlp serialization refactor #4873

rlp serialization refactor #4873

Conversation

debris commented Mar 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Mar 14, 2017

Choose a reason for hiding this comment

NikVolf Mar 14, 2017 • edited

Choose a reason for hiding this comment

gavofyork commented Mar 14, 2017

NikVolf commented Mar 14, 2017

gavofyork commented Mar 15, 2017

debris commented Mar 15, 2017

gavofyork commented Mar 15, 2017

debris commented Mar 17, 2017

gavofyork commented Mar 19, 2017

debris commented Mar 20, 2017

debris commented Mar 11, 2017 •

edited

NikVolf Mar 14, 2017 •

edited