Skip to content
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

Less unsafe, in more controlled instances #1487

Open
Wicpar opened this issue Nov 11, 2023 · 2 comments
Open

Less unsafe, in more controlled instances #1487

Wicpar opened this issue Nov 11, 2023 · 2 comments
Labels

Comments

@Wicpar
Copy link

Wicpar commented Nov 11, 2023

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
Copy link
Author

Wicpar commented Nov 12, 2023

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
Copy link
Author

Wicpar commented Nov 12, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant