Make unsound code opt-in.#51
Conversation
| // the latter from seeing it with `not(debug_assertions)`. | ||
| #[cfg(all(feature = "unsound", not(debug_assertions)))] | ||
| { | ||
| let page_size = 4096; |
There was a problem hiding this comment.
Hello
I was skimming rust serialization libraries and somewhat ended up here.
I'm not 100% sure, but I think that for posix systems the hardcoded page size could be replaced by something likesysconf(PAGE_SIZE); (man sysconf) then in theory be sound on compliant posix systems.
Feel free to take or toss my comment
have a nice day.
There was a problem hiding this comment.
Hello, thanks for your comment. Note that this optimization is most efficient if the page size is known statically and sysconf appears to retrieve it at runtime. AFAIK 4096 is safe because the page size won't be lower than 4096.
There was a problem hiding this comment.
Hello, thanks for your comment. Note that this optimization is most efficient if the page size is known statically and
sysconfappears to retrieve it at runtime. AFAIK 4096 is safe because the page size won't be lower than 4096.
Oh makes sense for the compile time optimisations.
Sysconf at runtime could also be used to choose the correct implementation something akin to:
if sysconf(SC_PAGESIZE) == 4096 { // or using something like a lazy static
// read_mem aligned
} else {
ptr.copy_non_overlapping();
}Downside: using sysconf would introduce a dependency on libc.
There was a problem hiding this comment.
Well a higher page size wouldn't require falling back to copy_non_overlapping. It would just slightly reduce the probability of the fallback, as the fast path could copy over 4096 byte boundaries within the larger page. The run-time overhead of this check (at least one predicted branch) might outweigh the benefits of this.
A lower page size would require something like this but, AFAIK, that's not possible.
Summary `bincode` is unmaintained. This isn't a problem on its own but if a problem comes up with it. As ruff's use of `bincode` is relatively isolated, the risk is low. Options: - defer: reevaluating what to do - use a fork: didn't see one I was confident in - fork: nothing pressing us to do this atm - `bitcode`: faster than `bincode` but blocked on SoftbearStudios/bitcode#51 but how that is being handled doesn't give me much confidence in the maintenance - `rkyv`: almost as fast as `bitcode` and highly respected If it wasn't for `rkyv` looking so promising, I would say we defer this. Test Plan Leveraging existing tests as this should be transparent
Summary `bincode` is unmaintained. This isn't a problem on its own but if a problem comes up with it. As ruff's use of `bincode` is relatively isolated, the risk is low. Options: - defer: reevaluating what to do - use a fork: didn't see one I was confident in - fork: nothing pressing us to do this atm - `bitcode`: faster than `bincode` but blocked on SoftbearStudios/bitcode#51 but how that is being handled doesn't give me much confidence in the maintenance - `rkyv`: almost as fast as `bitcode` and highly respected If it wasn't for `rkyv` looking so promising, I would say we defer this. Test Plan Leveraging existing tests as this should be transparent
2 participants
Fixes #50
Benchmarked on i7-7700k
Edit: need to benchmark again to show worst case performance difference.