Generic Resource Identifiers with ResourceIdTrait
#32
Conversation
Benchmark for 84bcdf7Click to view benchmark
|
| Deserialize, | ||
| Serialize, | ||
| )] | ||
| pub struct ResourceId {} |
There was a problem hiding this comment.
Is it stub for future implementation? We have "cryptographic-hash" enabled by default, so we should provide the default implementation.
There was a problem hiding this comment.
The ResourceId struct doesn't hold any data because there is no need to. Instead, methods like from_path and from_bytes return hash values directly.
The ResourceIdTrait simply enforces some restrictions on types implementing it, to allow us to use it from other crates without worrying about the underlying hash function.
| Deserialize, | ||
| Serialize, | ||
| )] | ||
| pub struct ResourceId {} |
There was a problem hiding this comment.
It was like this:
pub struct ResourceId {
pub data_size: u64,
pub hash: u32,
}
By the way, could it be useful to have definition like this shared across implementations?
pub struct ResourceId<H> {
pub data_size: u64,
pub hash: H,
}
Then we would have ResourceId type everywhere in the client code without the need for aliases.
There was a problem hiding this comment.
We can use a generic type instead of an associated type
pub trait ResourceIdTrait<T>
where
T: Debug
+ Display
+ FromStr
+ Clone
+ PartialEq
+ Eq
+ Ord
+ PartialOrd
+ Hash
+ Serialize,
{
/// Computes the resource identifier from the given file path
fn from_path<P: AsRef<Path>>(file_path: P) -> Result<T>;
/// Computes the resource identifier from the given bytes
fn from_bytes(data: &[u8]) -> Result<T>;
}and in data-resource/src/crc32.rs, we can implement it
impl ResourceIdTrait<u32> for ResourceId {
fn from_path<P: AsRef<Path>>(file_path: P) -> Result<u32> {I'm not entirely sure how that change would affect our situation.
Btw, do you think we should keep data_size as a field of ResourceId?
There was a problem hiding this comment.
Another point to keep in mind:
With pub trait ResourceIdTrait<T> where T: Debug, we'd need to manually annotate types when importing ResourceId in other crates.
This isn't ideal as we want to switch between "non-cryptographic-hash" and "cryptographic-hash" implementations without manual code changes if possible.
Benchmark for 9ee46bdClick to view benchmark
|
Benchmark for 88315efClick to view benchmark
|
|
We should test build of |
tareknaser
force-pushed
the
resourceid_trait
branch
from
e6ad074 to
609b055
Compare
Benchmark for da50af1Click to view benchmark
|
|
I've added two more tasks to the CI workflow. These tasks check if Once we reach the second milestone as discussed in #29 (comment), we can simplify things by using these commands to check compilation features: cargo clippy --workspacecargo clippy --workspace --features non-cryptographic-hash --no-default-featuresand cargo test --workspacecargo test --workspace --features non-cryptographic-hash --no-default-featuresThese commands will handle linting and running tests for all relevant crates with different features. |
|
So, after we finish #42 we should have approximately this layout:
Compilation features |
But wouldn't that be an issue if
What sets
We could write benchmarks directly into the crate housing the implementations of
I think it would be useful for the crate containing multiple implementations of Here's the reasoning:
I agree. |
I mean something like this:
The difference is that
Agree, I've also came to this idea that we should move the benchmarks into
The |
- Introduce `ResourceIdTrait`:
- Hash type restrictions
- Calculate hash values from paths or bytes.
- Add compilation features for `data-resource`:
- "non-cryptographic-hash" and "cryptographic-hash" (default) features.
- Implement `blake3::ResourceId` and `crc32::ResourceId`:
- Include `blake3::ResourceId` under the "cryptographic-hash" feature.
- Include `crc32::ResourceId` under the "non-cryptographic-hash" feature.
- Define benchmarks for hash creation:
- Create benchmarks for hash generation from paths and bytes.
- Update other crates in `ark-rust` workspace:
- Transition them to use the "non-cryptographic-hash" feature for now, because previously they were using crc32fast hash values directly.
Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
The `data-resource` crate now defines two compilation features: "cryptographic-hash" and "non-cryptographic-hash". Each feature corresponds to a specific hash function. This allows for more granular control over dependencies, as only the necessary dependencies for the selected hash function are exposed. Signed-off-by: Tarek <[email protected]>
…urces Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
tareknaser
force-pushed
the
resourceid_trait
branch
from
609b055 to
b159dfb
Compare
|
Moved to #42 |
Description
This PR ports changes from ARK-Builders/arklib#90 into
ark-rust.It represents the first milestone of #29
Changes
ResourceIdTrait:data-resource:blake3::ResourceIdandcrc32::ResourceId:blake3::ResourceIdunder the "cryptographic-hash" featurecrc32::ResourceIdunder the "non-cryptographic-hash" featureark-rustworkspace: