Implement DecodeWithMemTracking for BoundedBTreeMap
#906
Conversation
|
CI should probably be using |
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "bounded-collections" | |||
| version = "0.2.3" | |||
| version = "0.2.4" | |||
| description = "Bounded types and their supporting traits" | |||
| readme = "README.md" | |||
| rust-version = "1.79.0" | |||
There was a problem hiding this comment.
I went through the workspace members and found the following rust versions to be the minimum:
members = [
"fixed-hash", # 1.64
"keccak-hash", # 1.79
"kvdb", # 1.64
"kvdb-memorydb", # 1.64
"kvdb-rocksdb", # 1.71.1
"kvdb-shared-tests", # 1.64
"parity-bytes", # 1.64
"rlp", # 1.64
"rlp-derive", # 1.64
"uint", # 1.64
"primitive-types", # 1.79
"bounded-collections", # 1.79
"ethereum-types", # 1.79
"ethbloom", # 1.64
]
1.64 is the minimum for the whole workspace since before this the cargo manifest requires author to be a different format.
weirdly for the individual packages I don't see the requirement for 1.81, so I wonder if there are some unused deps at the workspace level
There was a problem hiding this comment.
You can use cargo msrv just to check that it works with that.
We should also have this in CI actually.
| @@ -123,6 +123,14 @@ where | |||
| } | |||
| } | |||
|
|
|||
| impl<K, V, S> DecodeWithMemTracking for BoundedBTreeMap<K, V, S> | |||
There was a problem hiding this comment.
I thought that BoundedBtreeMap is just a wrapper over BTreeMap, but I see it has its own implementation for decode. So we should do the same as for BTreeMap in the decoding logic: https://github.com/paritytech/parity-scale-codec/blob/8d65d28e778d914988849b786c17216070ecd9a1/src/codec.rs#L1273
|
|
Ah ok that makes sense, haven't worked in this repo before. Thanks for the context |
|
WIP until paritytech/parity-scale-codec#721 is released |
| input.descend_ref()?; | ||
| input.on_before_alloc_mem(codec::mem_size_of_btree::<(K, V)>(len))?; | ||
| let inner = Result::from_iter((0..len).map(|_| Decode::decode(input)))?; | ||
| input.ascend_ref(); |
There was a problem hiding this comment.
Should be replaced by
struct PrependCompactInput { compact: Compact<u32>, read: u32, inner: I }
/// impl Input that first prepends the compact and then uses the inner
/// After that you use it
let inner = BTreeMap::decode(&mut PrependCompactInput { compact, read: 0, inner: input })?
There was a problem hiding this comment.
Nice idea, implemented as you've described it in 1a73132
Depending on how hard we want to go we could maybe avoid some allocations since Compact<u32> has a fixed upper limit on its size with something like:
struct PrependCompactInput<'a, I> {
compact_bytes: [u8; 5],
compact_len: u8,
read: u32,
inner: &'a mut I,
}
Then you just encode the compact once instead of on each read. When we construct the struct we dump the encoded compact into this array and hardcode its length, so we know only the first compact_len bytes are valid
| return self.inner.read(into); | ||
| } | ||
|
|
||
| let to_read = into.len().min(remaining_compact); |
There was a problem hiding this comment.
You also need to have loop, because into.len() is mayber bigger than remaining_compact. The read function requires that all the requested bytes are filled.
There was a problem hiding this comment.
Just got around to fixing this in 908af22 and added tests. Should be good to go now
…oding Co-authored-by: Bastian Köcher <[email protected]>
| } | ||
| } else { | ||
| // Prepended compact has been read, just read from inner. | ||
| self.inner.read(into) |
There was a problem hiding this comment.
I don't increase the PrependCompactInput read counter after the compact has been fully read as it's pointless to track any more.
Implement
DecodeWithMemTrackingforBoundedBTreeMapand release bounded collections as0.2.4.