Support custom crc parameters #11
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Enables anyone to BYOP (bring your own parameters). See: awesomized#5
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for custom CRC parameters in the library and its C-compatible shared library. Key changes include:
- Adding new functions (new_with_params, checksum_with_params, checksum_file_with_params) in Rust to support custom CRC parameters.
- Extending FFI bindings in Rust and updating the C header (libcrc_fast.h) to expose the new API.
- Adding test coverage for the new functionality.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/lib.rs | New functions for custom CRC parameters added, along with corresponding tests. |
| src/ffi.rs | FFI functions added to support the new custom parameter API. |
| libcrc_fast.h | Updated header file exposing the new API for custom parameters. |
Comments suppressed due to low confidence (1)
src/lib.rs:618
- The variable 'digest' is declared but never used in this test. Consider removing it to avoid confusion.
let mut digest = Digest::new(config.get_algorithm());
jg15
reviewed
Jun 11, 2025
- Crc32Custom and Crc64Custom CrcAlgorithms - checksum_combine_with_custom_params() function - get_custom_params() function - test coverage for the above
Enables easier integration for consumers who would like to use custom CRC parameters, such as polynomials.
- Adds the Crc32Custom and Crc64Custom CrcFastAlgorithms - Adds the crc_fast_checksum_combine_with_custom_params() function - Adds the crc_fast_get_custom_params() function
jg15
reviewed
Jun 12, 2025
jg15
reviewed
Jun 12, 2025
Replaces get_custom_params() with CrcParams::new(). awesomized#11 (comment)
jg15
approved these changes
Jun 13, 2025
- [x] 1. Create cache module with core data structures - Create `src/cache.rs` module file - Define `CrcParamsCacheKey` struct with `width`, `poly`, and `reflected` fields - Implement `Debug`, `Clone`, `PartialEq`, `Eq`, and `Hash` traits for the cache key - Add module declaration to `src/lib.rs`
- [x] 2. Implement thread-safe cache storage
- Define global cache using `std::sync::OnceLock<RwLock<HashMap<CrcParamsCacheKey, [u64; 23]>>>`
- Implement `get_cache()` function to initialize and return cache reference
- Add necessary imports for `std::collections::HashMap`, `std::sync::{OnceLock, RwLock}`
- [x] 3. Implement cache lookup and storage functions - Create `get_or_generate_keys(width: u8, poly: u64, reflected: bool) -> [u64; 23]` function - Implement cache hit path with read lock and HashMap lookup - Implement cache miss path with key generation followed by write lock and storage - Add error handling for lock poisoning with fallback to direct key generation
- [x] 4. Add cache management utilities - Implement `clear_cache()` function for testing and memory management - Add proper error handling for all cache operations - Ensure all cache operations are best-effort with graceful degradation
- [x] 5. Integrate cache into CrcParams::new() - Modify `CrcParams::new()` in `src/structs.rs` to use `cache::get_or_generate_keys()` - Replace direct call to `generate::keys()` with cache lookup - Ensure all existing functionality remains unchanged - Verify that the function signature and behavior are identical
- Add tests to `src/cache.rs` - Write tests for cache key creation, equality, and hashing - Test cache hit scenarios (same parameters return cached keys) - Test cache miss scenarios (new parameters generate and cache keys) - Test that cached keys are identical to directly generated keys
- [x] 7. Add thread safety tests - Write concurrent access tests using `std::thread` - Test multiple threads reading from cache simultaneously - Test read-write contention scenarios - Verify cache consistency under concurrent access - Test lock poisoning recovery behavior
- Add tests to verify `CrcParams::new()` behavior is unchanged - Test that all existing CRC parameter combinations work correctly - Verify that cached and uncached results are identical - Test multiple `CrcParams` instances with same parameters use cached keys
- [x] 9. Add comprehensive error handling tests - Test cache behavior when locks are poisoned - Test memory allocation failure scenarios - Verify fallback to direct key generation works correctly - Test cache operations under memory pressure
- [x] 10. Update existing tests to work with caching - Run all existing tests to ensure no regressions - Update any tests that might be affected by caching behavior - Ensure test isolation by clearing cache between tests if needed - Verify all CRC algorithm tests still pass
- [x] 11. Add documentation and finalize implementation - Add inline documentation for all new public and internal functions - Update module-level documentation - Add usage examples in code comments - Ensure all code follows existing project style and conventions
- [x] 1. Phase 1: Add CrcKeysStorage enum and helper methods - Add CrcKeysStorage enum with KeysFold256 and KeysFutureTest variants - Implement get_key() and key_count() methods on CrcKeysStorage - Add const constructor methods from_keys_fold_256() and from_keys_fold_future_test() - Add safe accessor methods to CrcParams that delegate to existing keys field - Write comprehensive unit tests for CrcKeysStorage functionality
- [x] 2. Phase 2: Update architecture code to use safe accessors
- [x] 2.1 Update SIMD folding code in src/arch/ to use params.get_key()
- Replace direct keys[index] access with params.get_key(index) in algorithm.rs
- Update VPCLMULQDQ code to use safe key access methods
- Update aarch64 and x86 architecture-specific code
- _Requirements: 3.1, 5.2_
- [x] 2.2 Update CRC32 algorithm code to use safe accessors
- Modify src/crc32/algorithm.rs to use params.get_key() instead of keys[index]
- Update fusion code in src/crc32/fusion/ if it accesses keys directly
- _Requirements: 3.1, 5.2_
- [x] 2.3 Update CRC64 algorithm code to use safe accessors
- Modify src/crc64/algorithm.rs to use params.get_key() instead of keys[index]
- Update any other CRC64-specific code that accesses keys directly
- _Requirements: 3.1, 5.2_
- [x] 2.4 Run performance benchmarks to verify zero overhead
- Benchmark key access performance before and after changes
- Verify compiler optimizations eliminate any performance regression
- Document that performance remains identical to direct array access
- _Requirements: 2.2, 4.4_
- [x] 3. Phase 3: Switch CrcParams to use CrcKeysStorage
- [x] 3.1 Update CrcParams struct definition
- Change keys field from [u64; 23] to CrcKeysStorage
- Update CrcParams accessor methods to delegate to CrcKeysStorage
- Remove temporary delegation methods added in Phase 1
- _Requirements: 5.3_
- [x] 3.2 Update all CRC32 const definitions
- Update src/crc32/consts.rs to use CrcKeysStorage::from_keys_fold_256()
- Modify all CRC32_* const definitions to use new key storage format
- Ensure all existing key arrays are properly wrapped
- _Requirements: 1.2, 2.1_
- [x] 3.3 Update all CRC64 const definitions
- Update src/crc64/consts.rs to use CrcKeysStorage::from_keys_fold_256()
- Modify all CRC64_* const definitions to use new key storage format
- Ensure all existing key arrays are properly wrapped
- _Requirements: 1.2, 2.1_
- [x] 3.4 Update get-custom-params binary output
- Modify src/bin/get-custom-params.rs to output CrcKeysStorage format
- Update output template to use CrcKeysStorage::from_keys_fold_256()
- Test that generated const definitions compile and work correctly
- _Requirements: 6.1, 6.2, 6.3_
- [x] 3.5 Update cache system for new CrcParams structure
- Modify src/cache.rs to work with CrcKeysStorage-based CrcParams
- Update CrcParams::new() method to use new key storage format
- Ensure cache functionality remains intact after structural changes
- _Requirements: 2.3, 5.3_
- [x] 4. Create comprehensive test suite for future-proof functionality
- [x] 4.1 Add unit tests for bounds checking behavior
- Test that get_key() returns 0 for out-of-bounds indices
- Test that get_key_checked() returns None for invalid indices
- Verify key_count() returns correct values for different storage variants
- _Requirements: 3.2_
- [x] 4.2 Add integration tests for third-party compatibility
- Create mock third-party const definitions using new format
- Test that existing key access patterns continue to work
- Verify backwards compatibility throughout migration phases
- _Requirements: 1.1, 2.3_
- [x] 4.3 Add performance regression tests
- Benchmark CRC calculation performance before and after changes
- Verify that key access performance matches direct array access
- Test memory usage impact of enum-based storage
- _Requirements: 2.2, 4.4_
- [x] 4.4 Add future expansion simulation tests
- Create test CrcParams using KeysFutureTest variant with 25 keys
- Test that code gracefully handles different key array sizes
- Verify that expansion to larger key arrays works as designed
- _Requirements: 1.1, 4.2_
- [x] 5. Validate migration and run full test suite - Run cargo test to ensure all existing tests pass - Run cargo clippy to ensure code quality standards - Run cargo fmt to ensure consistent formatting - Verify that all CRC calculations produce identical results - Test that third-party usage patterns remain functional
- [x] 6. Implement FFI future-proofing for C/C++ compatibility
- [x] 6.1 Update CrcFastParams struct to use pointer-based keys
- Change keys field from [u64; 23] to const uint64_t *keys pointer
- Add key_count field to track number of available keys
- Update From<CrcFastParams> and From<CrcParams> conversion implementations
- _Requirements: 7.2, 8.1_
- [x] 6.2 Implement stable key pointer management
- Add create_stable_key_pointer() helper function for CrcKeysStorage
- Ensure key pointers remain valid for the lifetime of CrcFastParams
- Handle memory management safely between Rust and C boundaries
- _Requirements: 8.2, 8.3_
- [x] 6.3 Update FFI functions to use new CrcFastParams structure
- Update existing FFI functions to use new pointer-based CrcFastParams
- Ensure all FFI functions handle variable key counts correctly
- Test conversion between CrcKeysStorage variants and C pointer access
- _Requirements: 7.1, 7.3_
- [x] 6.4 Update C header file and add comprehensive FFI tests
- Update CrcFastParams struct definition in libcrc_fast.h to use pointer
- Create FFI tests for direct pointer access with different key counts
- Test future expansion scenarios with different key counts (23, 25, etc.)
- Verify memory safety and pointer stability across FFI boundary
- _Requirements: 7.1, 8.1_
More consistent with similar functions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Problem
While the list of hardcoded supported CRC variants is comprehensive, covering all the known CRC-32 and CRC-64 variants, it may be useful to support arbitrary custom parameters for future or unknown variants.
The Solution
Add support for custom CRC parameters.
Changes
Planned version bump
MINORLinks