Snapshot chunks packed by size #5318
Conversation
arkpar
added
A0-pleasereview 🤓
util/rlp/src/stream.rs
Outdated
| }, | ||
| } | ||
|
|
||
| // return chainable self | ||
| self | ||
| } | ||
|
|
||
|
|
||
| /// Declare appending the list of unknown size, chainable. | ||
| pub fn begin_unbound_list(&mut self) -> &mut RlpStream { |
ethcore/src/snapshot/account.rs
Outdated
| } | ||
|
|
||
| if let Some(pair) = leftover.take() { | ||
| account_stream.append_raw_checked(&pair, 1, target_chunk_size); |
There was a problem hiding this comment.
can you put an assert here?
| let len = self.buffer.len() - list.position; | ||
| self.encoder().insert_list_payload(len, list.position); | ||
| self.note_appended(1); | ||
| } |
There was a problem hiding this comment.
Maybe not in this pr, but what do you think about moving this logic to a new structure - UnboundedRlpStream? imo, it would reduce complexity
There was a problem hiding this comment.
It would only make sense if we forced each list to be a separate stream. What would it mean to call begin_list(X) on an UnboundedRlpStream or begin_list_unbounded() on a regular Stream?
There was a problem hiding this comment.
There is a use case when both bounded and unbounded lists are added to the same stream. I'd redesign RlpStream to accept tuples instead of fixed size begin_list()/end_list() and add a variant of append() that accepts an iterator. But that's out of the scope of this PR
ethcore/src/snapshot/account.rs
Outdated
| } | ||
|
|
||
| loop { | ||
| match db_iter.next() { |
There was a problem hiding this comment.
one of those loops could be moved to a new structure -> RlpChunkedStream which returns vec of chunks
There was a problem hiding this comment.
Chunking implemented here is too snapshot specific. Itertools::chunks can be used in simpler cases.
debris
added
A6-mustntgrumble 💦
arkpar
added
A0-pleasereview 🤓
debris
added
A8-looksgood 🦄
ethcore/src/snapshot/account.rs
Outdated
|
|
||
| if let Some(pair) = leftover.take() { | ||
| let leftover_appended = account_stream.append_raw_checked(&pair, 1, target_chunk_size); | ||
| assert!(leftover_appended); |
gavofyork
added
A5-grumble 🔥
|
|
|
Assert here indicates that this function is used incorrectly ( |
|
feel free to keep the assert, but just ensure you prove why it can never happen :) |
|
Since this is a public function and the |
arkpar
added
A0-pleasereview 🤓
|
if a public function can be called with invalid data such that it'll end up returning meaningless results (or worse having arbitrary side-effects), then it should be detecting such invalid input and failing fast with a |
|
That is now the behavior. Also, I'm not sure it really counts as a "public" function since it's only accessible from the module where the target chunk size constant is declared (and this is what the assertion depends on). I think either way is fine, either as an |
rphmeier
added
A8-looksgood 🦄
No description provided.