search

mrhooray / crc-rs

Rust implementation of CRC(16, 32, 64) with support of various standards
Apache License 2.0
187 stars 49 forks source link

Introduce an implementation that calculates the crc without using a lookup table #78

Closed KillingSpark closed 1 year ago

KillingSpark commented 1 year ago

This PR introduces an implementation for the Crc width 32, that calculates the crc without using a lookup table.

It also adds an explicit choice for the Bytewise implementation, which is the current default behaviour for Crc<u32>

Additionally it reorganizes the modules a little more neatly for the crc32 instead of having crc32, crc32_fast, ... it's now:

src
├── crc32
    ├── bytewise.rs
    ├── default.rs
    ├── nolookup.rs
    └── slice16.rs

This will allow easily and (invisibly to users) changing the default implementation.

This should also be used as the template for providing Nolookup<> and Bytewise<> for the other widths.

Benches

crc32/nolookup          time:   [137.87 µs 137.98 µs 138.10 µs]
                        thrpt:  [113.14 MiB/s 113.24 MiB/s 113.33 MiB/s]

crc32/bytewise          time:   [29.316 µs 29.330 µs 29.345 µs]
                        thrpt:  [532.46 MiB/s 532.74 MiB/s 532.98 MiB/s]

crc32/slice16           time:   [4.5155 µs 4.5248 µs 4.5324 µs]
                        thrpt:  [3.3666 GiB/s 3.3723 GiB/s 3.3792 GiB/s]+
akhilles commented 1 year ago

Thanks for submitting the PR! I'm a bit swamped this week, but I'll try to review this over the next couple days.

KillingSpark commented 1 year ago

Don't worry if it takes longer. I know how it can be sometimes :)

akhilles commented 1 year ago

Overall, the changes LGTM. Just have some minor nitpicks.

KillingSpark commented 1 year ago

Apart from the crc32.rs -> mod.rs rename this LGTM.

Introduce an implementation that calculates the crc32 without using a lookup table would probably be a good squashed commit message

KillingSpark commented 1 year ago

I'll make some more PRs for the other widths. Do you prefer the same separation for slice16/notable impls or should I do one PR per width that adds both slice16/notable at once?

akhilles commented 1 year ago

I'll make some more PRs for the other widths. Do you prefer the same separation for slice16/notable impls or should I do one PR per width that adds both slice16/notable at once?

Up to you, whatever's easier.