spacejam / sled

the champagne of beta embedded databases
Apache License 2.0
7.89k stars 377 forks source link

Less unsafe, in more controlled instances #1487

Open Wicpar opened 7 months ago

Wicpar commented 7 months ago

Use Case:

Unsafe is used almost everywhere in the code. No benchmarks are there to make sure that they give a true benefit over safe alternatives.

One instance if find a bit too unsafe-happy is

unsafe { MaybeUninit::zeroed().assume_init() }

which even if it is correct now can very easily break at no additional gain in performance, it's literally the same as the safe version: compiler explorer

Proposed Change:

progressively phase out unsafe code where possible, and where impossible wrap them in primitives that can be extracted to a well tested subcrate, leaving the main crate eventually #![forbid(unsafe_code)]

Who Benefits From The Change(s)?

The whole community, from safe rust garantees, the devs for less bugs to fix, less cases to test

Alternative Approaches

live dangerously

Wicpar commented 7 months ago

in addition to the PR #1488

i see a big change that can be made in iobuf by using const generics in the config for the segment size. As they do not change during runtime they could be type defined, and thus allow for safe

#[repr (C, align (8192))]
struct AlignedBuf<const N: usize>([u8; N])

then implement deref and deref mut and eliminate all of the pointer based unsafes, and at the same time allow the compiler to optimize range checks away during slice access. The only hicup is the memset to zero as you can see here: https://godbolt.org/z/PEj6h4YoP

it can be circumvented by a MaybeUninit and in that case the generated assembly is identical to the more unsafe version.

Wicpar commented 7 months ago

turns out the modifications are impossible without removing the option to change the segment_size, rust nightly using const expressions or leaking an ugly const generic into every single struct of the library. Using an associated type in a config trait would more elegantly leak into all structs but doesn't allow for the advantages of arrays.

the most elegant solution is to simply remove the parameter, and the second best having a dyn io buffer that can be specialized with certain sizes.

I'll see if i can do that and make a benchmark to see if the difference is significant.