Feature/config limit #439
Feature/config limit #439
Conversation
|
TODO(self):
|
VictorKoenders
force-pushed
the
feature/config-limit
branch
from
ba72eaa
to
d8cd6e9
Compare
Codecov Report
@@ Coverage Diff @@
## trunk #439 +/- ##
==========================================
+ Coverage 62.19% 62.87% +0.68%
==========================================
Files 46 46
Lines 3383 3456 +73
==========================================
+ Hits 2104 2173 +69
- Misses 1279 1283 +4
Continue to review full report at Codecov.
|
| super::impl_core::collect_into_array(&mut (0..N).map(|_| T::decode(&mut decoder))); | ||
| let result = super::impl_core::collect_into_array(&mut (0..N).map(|_| { | ||
| // See the documentation on `unclaim_bytes_read` as to why we're doing this here | ||
| decoder.unclaim_bytes_read(core::mem::size_of::<T>()); |
There was a problem hiding this comment.
rather than unclaiming in a loop, we could just unclaim right after claiming. That should reduce loop maths.
There was a problem hiding this comment.
The reason I unclaim per item is because of nested containers.
Consider the situation where there is data type Vec<Vec<u8>>, where each Vec is 100 entries. This means the entire container would allocate 10kb of memory.
If our limit is set to 2500, the current impl would:
- Allocate the outer
Vec<Vec<_>>(100 * 24), count = 2400 - Deallocate 1 inner
Vec<u8>(24 bytes), count = 2376 - Allocate the inner
Vec<u8>(100 bytes), count = 2476 - Deallocate 1 inner
Vec<u8>(24 bytes), count = 2452 - Allocate the inner
Vec<u8>(100 bytes), count = 2552
This would fail at the 3rd allocation
Whereas if we unclaim the entire vec immediately, we would get:
- Allocate the outer
Vec<Vec<_>>(100 * 24), count = 2400 - Deallocate this, count = 0
- 25x:
- Allocate the inner
Vec<u8>(100 bytes), count = 100, 200, etc
- Allocate the inner
This would fail at the 25th allocation
| @@ -52,8 +52,13 @@ where | |||
| { | |||
| fn decode<D: Decoder>(mut decoder: D) -> Result<Self, DecodeError> { | |||
| let len = crate::de::decode_slice_len(&mut decoder)?; | |||
| decoder.claim_container_read::<T>(len)?; | |||
There was a problem hiding this comment.
why is this used sometimes and other times (like the array impl) done manually? Also, same comment here about immediately unclaiming
There was a problem hiding this comment.
This is specifically for containers (e.g. Vec, HashMap, etc). An array [T; N] is in my eyes not a container
VictorKoenders
force-pushed
the
feature/config-limit
branch
from
d8cd6e9
to
bc30d0a
Compare
…inlining to the decoder
…ize_of<[T; N]>())`
Adds a limit to configuration and keeps track of how many bytes are going to be read, returning a
DecodeError::LimitExceededif this exceeds the amount of bytes read.The reason this is set up like this is because we use a lot of
Container::with_capacitywhen decoding containers. The limit exists primarily to not allocate too much value, for example when data is garbage, we don't want to accidentally allocate 10GB of memory when the data is invalid.I've added an
unclaimsystem as well. As per theDecodercomments: