Improved encoding and decoding speed of Vec<u8> #619
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## trunk #619 +/- ##
==========================================
- Coverage 54.22% 54.01% -0.21%
==========================================
Files 50 51 +1
Lines 4406 4447 +41
==========================================
+ Hits 2389 2402 +13
- Misses 2017 2045 +28
... and 8 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Pre-computing the size drops the execution speed down by another 50% |
|
I'm willing to call this good if this is okay with you @JojiiOfficial Execution speeds in the order of 51.781 µs and 364.96 µs are essentially instant. We can probably optimize this further, but we're hunting very specific LLVM optimizations at this point. In a real world scenario we would almost certainly not notice a difference between the two. I'd suggest we say that this is fast enough for now and if we find a real world example where bincode 2 is significantly slower than bincode 1 we can add more benchmarks. |
| @@ -262,10 +278,20 @@ where | |||
|
|
|||
| impl<T> Decode for Vec<T> | |||
| where | |||
| T: Decode, | |||
| T: Decode + 'static, | |||
There was a problem hiding this comment.
Doesn't this bound rule out zero-copy deserialization of types like Vec<&'de str>?
There was a problem hiding this comment.
To answer my own question: No, as this case goes though impl BorrowDecode for Vec<T>.
|
Encoding still is 6 times slower than in v1. A real world example could be a lazy loaded data structure (eg. an index) that keeps its data encoded and if an item is accessed, it gets decoded. If you're doing this a million of times lets say you're building the index, v2 will take 6 times longer than v1 (As you do a lot of encoding when building). An index built with V2 will take 6 minutes where the same data structure will only take 1 minute if it was using bincode v1. If v1 takes 1h, v2 will take 6 hours and so on... Not saying this fix isn't ok just want to point out that there are real world scenarios where this will probably hurt. In my opinion opening a tracking issue and merging this PR should be fine for now but having a de/serializing library for binary data that is implemented as fast as possible with as much optimizations as possible would be really nice. Maybe there are even more possible optimizations that would make it even faster than v1 🤔 |
|
If you have a benchmark with data that lasts a minute to encode/decode that would be very interesting to dive into |
|
Change the parameter for |
|
The following code improves the performance even more for Strings and [u8] arrays: impl<T> Encode for [T]
where
T: Encode + 'static,
{
fn encode<E: Encoder>(&self, encoder: &mut E) -> Result<(), EncodeError> {
super::encode_slice_len(encoder, self.len())?;
if core::any::TypeId::of::<T>() == core::any::TypeId::of::<u8>() {
let t: &[u8] = unsafe { transmute(self) };
encoder.writer().write(t)?;
return Ok(());
}
for item in self {
item.encode(encoder)?;
}
Ok(())
}
}
|
|
Added a couple of |
|
Indeed, very nice! |
Added a SizeWriter because someone finally has a benchmark to show it's faster
VictorKoenders
force-pushed
the
vko/improve_string_decode
branch
from
0a0bb51
to
e613898
Compare
| @@ -295,10 +295,17 @@ impl Encode for char { | |||
|
|
|||
| impl<T> Encode for [T] | |||
| where | |||
| T: Encode, | |||
| T: Encode + 'static, | |||
There was a problem hiding this comment.
This change now gives me errors when trying to derive Encode for types with Vecs that contain references.
For example:
struct MyStruct<'a> {
some_field: Vec<&'a str>
}Argument requires that 'a must outlive 'static
Running against the benchmarks in #618
decoding seems to be on-par with bincode 1 now. Encoding is down to being a factor 10 slower.