Optimized .count() for the iterators.

[eadf]

Added some modest optimizations for .count() of all the iterators

Results

With a benchmark of 100_000 random bits, on my M1 Mac:

• Vob<usize>::iter().count() took 31_472 ps, now it takes 314 ps (100 times faster)
• Vob<usize>::iter_set_bits(..).count() took 178_860 ns, now it takes 198 ns (900 times faster)
• Vob::iter_storage().count() has been updated too (now it just does math instead of counting).

Changes

Added a private count_set_bits() method that counts set bits in O(number of words)
Fixed the deprecated rand::Rng methods in the benchmark code
Added tests and benchmarks for the new iter*.count() functionality

Issues that needs resolve:

✓ ### move count_set_bits() (delay to next semver update)
We could move count_set_bits() into the iterator if IterSetBits and IterUnsetBits were merged into one BitsIterator. This could be done without any runtime penalties, and we'd skip some duplicated boilerplate:

pub struct BitsIterator<'a, T: 'a, const BIT_STATE: bool> {...}

We could do this is stages, for now we could simply copy&paste like this (to reduce risk of new bugs):

fn next(&mut self) -> Option<usize> {
    if BIT_STATE {
      // old IterSetBits::next() code
    } else {
      // old IterUnsetBits::next() code 
    }
}

Remove benchmark

Once the gain is confirmed, the new count_bits() benchmark can be removed because it simply tests range.count(), and that's like benchmarking std::Range.

Possibly add a new helper method

for n % bits_per_block::<T>(), just like block_offset() does n / bits_per_block::<T>()

✓ ### Tidy up the .filter(|_|true)
I use iterator.filter(|_| true).count() to trigger the old .count() behavior in the tests, maybe there are better ways to do that.

✓ ### count_ones_usize()
count_ones_usize() could safely use unwrap_unchecked() (or it could even safely use as)

(link #86)

[ltratt]

Could you update the benchmarks so that they all use the same units please? My pea brain misreads comparisons when the units change!

[ltratt]

In vob we use off not word. So this would be start_off (etc).

[ltratt]

Vob calls this b.

[ltratt]

I'd prefer to call the count_ones std lib method directly rather than use this wrapper. The try_from is mildly ugly in source code, but it'll be optimised away, so it's not worth worrying about.

[eadf]

but it'll be optimised away, so it's not worth worrying about.

I was sceptical about that, so i tested it out. It's actually true (on non-embedded platforms anyways)

[ltratt]

I like the direction of this! My comments are mostly shallow and making sure this code feels in keeping with the rest of Vob's source, but the general idea clearly works as-is.

We could move count_set_bits() into the iterator if IterSetBits and IterUnsetBits were merged into one BitsIterator

This would be a semver breakage. It might be worth doing after the next release, but it would be nice to get the optimisation out to people first before we think about breaking the API.

I use iterator.filter(|_| true).count() to trigger the old .count() behavior in the tests, maybe there are better ways to do that.

I don't have a better idea!

count_ones_usize() could safely use unwrap_unchecked() (or it could even safely use as)

rustc will almost certainly optimise the unwrap away entirely on the platforms we know and care about (certainly, in all similar cases I've seen, and checked, it has done so, though obviously one can not 100% guarantee this), so the try_from is good, because if someone ever tries this on a platform with different integer constraints it will break in the right way.

[eadf]

This would be a semver breakage

Yes, you are right about that. But it is unlikely that anybody stores an IterSetBits as just that type, instead of a impl Iterator. But i agree, let's wait with that change.

rustc will almost certainly optimise the unwrap away entirely

Not only that, the try_from() is removed too. At least on 64bits machines.
I didn't know that, as you said - we learn every day.

[ltratt]

But it is unlikely that anybody stores an IterSetBits as just that type, instead of a impl Iterator

I quite often store iterator types, so I know one user who'll be affected ;)

[ltratt]

This can go at the top of the file alongside the other imports.

[ltratt]

One minor comment then I think we're ready to merge.

[eadf]

One minor comment then I think we're ready to merge.

And the other issues? i.e remove count_bits() bench and helper method for n % bits_per_block::<T>()?

[ltratt]

I think we're ready. Please undraft?

[eadf]

Ok, I'll remove the benches i mentioned earlier and won't not add a modulus helper method.

[ltratt]

Thanks!