Make blocks_required() branchless. #85
Conversation
src/lib.rs
Outdated
| } | ||
| /// to store those bits. (Using the classic ceiling division formula: ⌈n / b⌉ = ⌊(n + b - 1) / b⌋) | ||
| const fn blocks_required<T>(num_bits: usize) -> usize { | ||
| (num_bits + bits_per_block::<T>() - 1) / bits_per_block::<T>() |
There was a problem hiding this comment.
A problem with this (more on 32 than 64 bit) is that it can overflow, even with a valid (and on 32 bit, very plausible) size. Even worse, in release mode, it won't cause a panic -- something weird will happen, but I'm not quite sure what in this context!
There was a problem hiding this comment.
Ok, i see.
We could write it as num_bits / bits_per_block::<T>() + (remainder != 0 as usize) instead, and still be branchless (unless as adds some under the hood).
And yes, bool as usize is idiomatic, safe, rust code.
There was a problem hiding this comment.
I'm allergic to as, but usize::from probably works and would be fine with me!
There was a problem hiding this comment.
I'm not sure if replacing an if branch with a, potentially non-inlined, method call is any improvement:
fn blocks_required<T>(num_bits: usize) -> usize {
num_bits / bits_per_block::<T>() + usize::from(num_bits % bits_per_block::<T>() != 0)
}
Just checked with godbolt: with "-C opt-level=3" both as and usize::from() compiles down to the same one-single-OP code.
There was a problem hiding this comment.
It definitely looks to me like the from impl just calls as in an #[inline(always)] function.
https://doc.rust-lang.org/src/core/convert/num.rs.html#76-79
There was a problem hiding this comment.
My problem with as is that it has surprising use edge cases, which are double dangerous if types are later changed. It works well until it doesn't!
There was a problem hiding this comment.
Yeah, as can be dicey. Specially on machines where usize != u64, but once wrapped inside std::from() it's the problem of the Rust maintainers.
For this particular conversion: bool -> usize, the op is safe even if usize is anu8.
|
We're having some problems with github at our end, so this might take a little while to merge in. Don't worry, it will! |
|
Please can you force push an update to this branch? Hopefully that will kick our CI back into life. |
Made bits_per_block(), bytes_per_block() and block_offset() const fn.
3 participants
bits_per_block()andbytes_per_block()now compiles intousizeliterals: the ultimate inline.