-
-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
array de bin leak #80
Conversation
according to these godbolt examples it should optimize ok, but may be slower in debug builds |
I think additional code complexity is not worth it. This leak looks quite niche too be too concerned about. I would rather make a good looking fix or just leave it as is for now. |
Your idea, but IMO looks a bit better. I am not sure if rust-lang/rust#89379 will be stabilized super soon, so, maybe, considering the same problem for json/ron, we need to make our own poor-man try_from_fn or something. |
I spent a little while checking and yours performs way better on memory usage (speed optimized about the same) at every opt level. |
Added your version, just with a couple more comments impl<T, const N: usize> DeBin for [T; N]
where
T: DeBin,
{
fn de_bin(o: &mut usize, d: &[u8]) -> Result<Self, DeBinErr> {
use core::mem::MaybeUninit;
// waiting for uninit_array(or for array::try_from_fn) stabilization
// https://github.com/rust-lang/rust/issues/96097
// https://github.com/rust-lang/rust/issues/89379
let mut to: [MaybeUninit<T>; N] =
unsafe { MaybeUninit::<[MaybeUninit<T>; N]>::uninit().assume_init() };
for index in 0..N {
to[index] = match DeBin::de_bin(o, d) {
Ok(v) => MaybeUninit::new(v),
Err(e) => {
// drop all the MaybeUninit values which we've already
// successfully deserialized so we don't leak memory.
// See https://github.com/not-fl3/nanoserde/issues/79
for (_, to_drop) in (0..index).zip(to) {
unsafe { to_drop.assume_init() };
}
return Err(e);
}
}
}
// waiting for array_assume_init or core::array::map optimizations
// https://github.com/rust-lang/rust/issues/61956
Ok(unsafe { (&*(&to as *const _ as *const MaybeUninit<_>)).assume_init_read() })
}
} that cast at the end is so hard to read, but seems like all the easy-to-read options perform very badly |
@not-fl3 did you have any remaining concerns over this implementation? |
Let's merge it! |
Ah, sure, I will do that. I wonder if I could get a better test 🤔 |
4ae5509
to
d4ca3ac
Compare
…d objects and erroring out
…cts when an error is encountered
ac3c0a9
to
174f660
Compare
@not-fl3 added a commit with tests for json/ron/bin showing that drop code was not being run previously, and is being run after the update. Can check out the first commit to see the failing tests, and the second commit that fixes it |
If you're happy with the DeJson/DeRon impls I think it's ready |
LGTM! |
pr to close #79. The primitives for fallible creation of arrays and transmutation of
MaybeUninit
arrays are both unstable still, so doing it manually here. There are now only two unsafe lines, one toassume_init
each item in case of success, and one toassume_init
each item which has been successfully deserialized (so it can be dropped) before returning an error.The
core::array::map
documentation has some concerning looking warnings about poor performance, so maybe would be better to go back to a transmute for that instead of applyingassume_init
in the map like I've done here.