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.

Sign up for GitHub

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

Jump to bottom

switch impls based on crate features for u8 as an example #101

Merged
merged 13 commits into from
Apr 23, 2023
Merged

switch impls based on crate features for u8 as an example #101

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
06c5344
switch impls based on crate features for u8 as an example
KillingSpark Feb 27, 2023
c7f7f73
switch impls based on crate features for u16
KillingSpark Apr 8, 2023
e584dd5
switch impls based on crate features for u32
KillingSpark Apr 8, 2023
9d24c86
switch impls based on crate features for u64
KillingSpark Apr 8, 2023
2b77d4d
switch impls based on crate features for u128
KillingSpark Apr 8, 2023
91f1e99
if no feature is selected use the bytewise implementation
KillingSpark Apr 9, 2023
9200156
add tests for the sizes of the default implementation under the vario…
KillingSpark Apr 12, 2023
943ef4d
add script that runs the tests under all feature combinations
KillingSpark Apr 12, 2023
8bc5434
make size tests fail on compilation
KillingSpark Apr 17, 2023
d337ce5
rename the features to reflect their effects better and avoid the wor…
KillingSpark Apr 17, 2023
692ff85
cargo fmt
KillingSpark Apr 17, 2023
e6a2916
rename features to xxx-mem-limit
KillingSpark Apr 23, 2023
68699a3
cargo fmt...
KillingSpark Apr 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ keywords = ["crc", "crc16", "crc32", "crc64", "hash"]
categories = ["algorithms", "no-std"]
edition = "2018"

[features]
notable-defaults = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] On naming, wdyt about "prefer-low-memory" and "prefer-fast" instead for "notable-defaults" and "slice16-defaults" respectively? They might be easier to understand for some users since it maps closer to intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect the people that care about any of this to also care about the specifics i.e. how much memory is "low memory" exactly at which point they also need to read about the different implementations. To then name the features after these implementations seems logical to me

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I still think the feature names can be a bit clearer though. Especially with "notable" as it can be interpreted as a single word. Maybe "prefer-no-table", "prefer-bytewise", "prefer-slice-by-16"? I think it's worth avoiding "default" as it's an overloaded term.

Also, would be good to add a comment on top of each feature (similar to https://github.com/serde-rs/serde/blob/master/serde/Cargo.toml#L39)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have another think about the naming

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came up with

  • no-table-memory-restrictions
  • bytewise-memory-restrictions
  • slice16-memory-restrictions

I think this captures the aspect of the additional restrictions one is imposing by activating the features, hopefully also conveying that those are additive.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good idea! I've also seen "limit" used in this context. Wdyt about using "mem-limit" or "memory-limit" as the suffix? Has the advantage that it's shorter. Sorry for being so picky on the naming :)

Copy link
Contributor Author

@KillingSpark KillingSpark Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being so picky on the naming :)

No worries, naming is important and one of the hardest things to get right :)

"Limit" seems like a fine choice!

bytewise-defaults = []
akhilles marked this conversation as resolved.
Show resolved Hide resolved
slice16-defaults = []

[dependencies]
crc-catalog = "2.1.0"

Expand Down
61 changes: 58 additions & 3 deletions src/crc128.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use crc_catalog::Algorithm;

use crate::util::crc128;
use crc_catalog::Algorithm;

mod bytewise;
mod default;
Expand Down Expand Up @@ -170,9 +169,65 @@ const fn update_slice16(

#[cfg(test)]
mod test {
use crate::{Bytewise, Crc, NoTable, Slice16};
use crate::{Bytewise, Crc, Implementation, NoTable, Slice16};
use crc_catalog::{Algorithm, CRC_82_DARC};

#[test]
fn default_table_size() {
let table_size = core::mem::size_of::<<u128 as Implementation>::Table>();
akhilles marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(256 * 16, table_size);
#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 16, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(256 * 16 * 16, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 16, table_size);
}

/// Test this opitimized version against the well known implementation to ensure correctness
#[test]
fn correctness() {
Expand Down
87 changes: 82 additions & 5 deletions src/crc128/default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,62 @@
use crate::table::crc128_table;
use crate::{Algorithm, Crc, Digest};
use crate::crc128::{finalize, init};
use crate::{Algorithm, Crc, Digest, Implementation};

use super::{finalize, init, update_bytewise};
#[cfg(feature = "notable-defaults")]
impl Implementation for u128 {
type Width = u128;
type Table = ();
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
impl Implementation for u128 {
type Width = u128;
type Table = [u128; 256];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
impl Implementation for u128 {
type Width = u128;
type Table = [[u128; 256]; 16];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
impl Implementation for u128 {
type Width = u128;
type Table = [u128; 256];
}

impl Crc<u128> {
pub const fn new(algorithm: &'static Algorithm<u128>) -> Self {
akhilles marked this conversation as resolved.
Show resolved Hide resolved
let table = crc128_table(algorithm.width, algorithm.poly, algorithm.refin);
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
let table =
crate::table::crc128_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
let table = crate::table::crc128_table(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(feature = "notable-defaults")]
#[allow(clippy::let_unit_value)]
let table = ();

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
let table = crate::table::crc128_table(algorithm.width, algorithm.poly, algorithm.refin);

Self { algorithm, table }
}

Expand All @@ -16,7 +67,33 @@ impl Crc<u128> {
}

const fn update(&self, crc: u128, bytes: &[u8]) -> u128 {
update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
{
super::update_slice16(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(feature = "notable-defaults")]
{
super::update_nolookup(crc, self.algorithm, bytes)
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}
}

pub const fn digest(&self) -> Digest<u128> {
Expand Down
58 changes: 57 additions & 1 deletion src/crc16.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,65 @@ const fn update_slice16(

#[cfg(test)]
mod test {
use crate::{Bytewise, Crc, NoTable, Slice16};
use crate::{Bytewise, Crc, Implementation, NoTable, Slice16};
use crc_catalog::{Algorithm, CRC_16_IBM_SDLC};

#[test]
fn default_table_size() {
let table_size = core::mem::size_of::<<u16 as Implementation>::Table>();
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(0, table_size);
#[cfg(all(
feature = "notable-defaults",
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(0, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
feature = "slice16-defaults"
))]
assert_eq!(256 * 2, table_size);
#[cfg(all(
not(feature = "notable-defaults"),
feature = "bytewise-defaults",
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 2, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
assert_eq!(256 * 16 * 2, table_size);

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
assert_eq!(256 * 2, table_size);
}

/// Test this opitimized version against the well known implementation to ensure correctness
#[test]
fn correctness() {
Expand Down
86 changes: 81 additions & 5 deletions src/crc16/default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,62 @@
use crate::crc16::{finalize, init};
use crate::table::crc16_table_slice_16;
use crate::{Algorithm, Crc, Digest};
use crate::{Algorithm, Crc, Digest, Implementation};

use super::update_slice16;
#[cfg(feature = "notable-defaults")]
impl Implementation for u16 {
type Width = u16;
type Table = ();
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
impl Implementation for u16 {
type Width = u16;
type Table = [u16; 256];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
impl Implementation for u16 {
type Width = u16;
type Table = [[u16; 256]; 16];
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
impl Implementation for u16 {
type Width = u16;
type Table = [u16; 256];
}

impl Crc<u16> {
pub const fn new(algorithm: &'static Algorithm<u16>) -> Self {
let table = crc16_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
let table =
crate::table::crc16_table_slice_16(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
let table = crate::table::crc16_table(algorithm.width, algorithm.poly, algorithm.refin);

#[cfg(feature = "notable-defaults")]
#[allow(clippy::let_unit_value)]
let table = ();

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
let table = crate::table::crc16_table(algorithm.width, algorithm.poly, algorithm.refin);

Self { algorithm, table }
}

Expand All @@ -17,7 +67,33 @@ impl Crc<u16> {
}

const fn update(&self, crc: u16, bytes: &[u8]) -> u16 {
update_slice16(crc, self.algorithm.refin, &self.table, bytes)
#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
feature = "slice16-defaults"
))]
{
super::update_slice16(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(all(not(feature = "notable-defaults"), feature = "bytewise-defaults"))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}

#[cfg(feature = "notable-defaults")]
{
super::update_nolookup(crc, self.algorithm, bytes)
}

#[cfg(all(
not(feature = "notable-defaults"),
not(feature = "bytewise-defaults"),
not(feature = "slice16-defaults")
))]
{
super::update_bytewise(crc, self.algorithm.refin, &self.table, bytes)
}
}

pub const fn digest(&self) -> Digest<u16> {
Expand Down
Loading