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.

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

fs-storage implemented trait for conflict resolution #26

Merged
merged 46 commits into from
May 25, 2024
Merged

fs-storage implemented trait for conflict resolution #26

merged 46 commits into from
May 25, 2024

Conversation

Pushkarm029
Copy link
Collaborator

@Pushkarm029 Pushkarm029 commented Apr 9, 2024

Working on:

This PR introduces merge_from function that merges entries of local mapping and another one. Value with clashing keys are merged using the Monoid instance provided by the storage.

kirillt and others added 26 commits April 8, 2024 12:29
* Bump arklib version
* Added tokio to deal with futures
* Updating the index when links are added
* Update GitHub Actions
---------
Co-authored-by: Alvi Hysa <[email protected]>
The branch `ark-rust_hot_fix` contains a hot fix where we use the same commit specified when writing `ark-cli` but with pinning `image` crate to the correct version so that `ark-rust` can compile

This is just a hot fix. `arklib` shouldn't be even a dependency of any `ark-rust` crate. See #15 for more information

Signed-off-by: Tarek <[email protected]>
@Pushkarm029 Pushkarm029 requested a review from twitu April 9, 2024 11:09
tareknaser added a commit to tareknaser/ark-rust that referenced this pull request Apr 9, 2024
tareknaser added a commit to tareknaser/ark-rust that referenced this pull request Apr 9, 2024
kirillt pushed a commit that referenced this pull request Apr 10, 2024
Copy link

Benchmark for b6a7374

Click to view benchmark
Test Base PR %
../test-assets/lena.jpg/compute_bytes 14.0±0.73µs 13.9±0.09µs -0.71%
../test-assets/test.pdf/compute_bytes 113.4±0.69µs 114.0±0.90µs +0.53%
compute_bytes_large/compute_bytes 142.5±1.71µs 143.0±3.55µs +0.35%
compute_bytes_medium/compute_bytes 27.2±0.49µs 27.3±0.46µs +0.37%
compute_bytes_small/compute_bytes 131.2±1.59ns 131.3±2.22ns +0.08%
index_build/index_build/../test-assets/ 160.1±1.10µs 160.4±1.52µs +0.19%

Copy link
Collaborator

@twitu twitu left a comment

Choose a reason for hiding this comment

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

Looking good!


file_storage.set("key1".to_string(), "value1".to_string());
assert_eq!(file_storage.is_storage_updated().unwrap(), false);
//todo: we need to add 1ms delays to make the test pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps I was too eager in making set immediately sync the file system. It'll be more useful to allow the library user to decide whether they want to write to disk immediately after setting a value or use a different strategy like batching writes.

Once write_fs is moved out of set this test will also be valid.

fs-storage/src/monoid.rs Outdated Show resolved Hide resolved
fs-storage/src/monoid.rs Show resolved Hide resolved
Signed-off-by: pushkarm029 <[email protected]>
Copy link

Benchmark for 639d612

Click to view benchmark
Test Base PR %
../test-assets/lena.jpg/compute_bytes 13.9±0.21µs 14.0±0.11µs +0.72%
../test-assets/test.pdf/compute_bytes 115.6±1.30µs 114.7±1.46µs -0.78%
compute_bytes_large/compute_bytes 145.4±2.10µs 144.9±2.42µs -0.34%
compute_bytes_medium/compute_bytes 27.9±0.19µs 27.9±0.45µs 0.00%
compute_bytes_small/compute_bytes 131.4±1.23ns 131.4±2.40ns 0.00%
index_build/index_build/../test-assets/ 159.8±2.32µs 160.0±1.59µs +0.13%

@Pushkarm029 Pushkarm029 requested a review from twitu May 23, 2024 18:19
Signed-off-by: pushkarm029 <[email protected]>
Copy link

Benchmark for b19bd6a

Click to view benchmark
Test Base PR %
../test-assets/lena.jpg/compute_bytes 13.9±0.13µs 13.9±0.09µs 0.00%
../test-assets/test.pdf/compute_bytes 113.7±0.57µs 215.9±0.79µs +89.89%
compute_bytes_large/compute_bytes 143.3±0.63µs 337.5±2.05µs +135.52%
compute_bytes_medium/compute_bytes 27.1±0.18µs 27.8±0.48µs +2.58%
compute_bytes_small/compute_bytes 131.0±1.53ns 131.7±5.22ns +0.53%
index_build/index_build/../test-assets/ 159.8±0.67µs 160.2±2.88µs +0.25%

Signed-off-by: pushkarm029 <[email protected]>
Copy link

Benchmark for 47f2e17

Click to view benchmark
Test Base PR %
../test-assets/lena.jpg/compute_bytes 14.0±0.12µs 13.9±0.10µs -0.71%
../test-assets/test.pdf/compute_bytes 111.0±0.86µs 129.1±0.55µs +16.31%
compute_bytes_large/compute_bytes 139.8±0.72µs 170.5±1.69µs +21.96%
compute_bytes_medium/compute_bytes 27.1±0.22µs 27.9±0.39µs +2.95%
compute_bytes_small/compute_bytes 130.8±1.04ns 130.9±0.99ns +0.08%
index_build/index_build/../test-assets/ 162.0±0.58µs 161.3±2.28µs -0.43%

@Pushkarm029 Pushkarm029 requested review from kirillt and tareknaser May 24, 2024 17:01
Copy link

Benchmark for fcadd60

Click to view benchmark
Test Base PR %
blake3_resource_id_creation/compute_from_bytes:large 251.2±1.25µs 251.4±2.65µs +0.08%
blake3_resource_id_creation/compute_from_bytes:medium 15.6±0.09µs 15.6±0.08µs 0.00%
blake3_resource_id_creation/compute_from_bytes:small 1337.7±12.21ns 1325.5±3.76ns -0.91%
blake3_resource_id_creation/compute_from_path:../test-assets/lena.jpg 195.5±0.59µs 195.2±0.72µs -0.15%
blake3_resource_id_creation/compute_from_path:../test-assets/test.pdf 1719.0±2.74µs 1718.6±18.56µs -0.02%
crc32_resource_id_creation/compute_from_bytes:large 86.9±0.47µs 87.0±0.80µs +0.12%
crc32_resource_id_creation/compute_from_bytes:medium 5.4±0.02µs 5.4±0.07µs 0.00%
crc32_resource_id_creation/compute_from_bytes:small 92.4±0.62ns 92.5±1.13ns +0.11%
crc32_resource_id_creation/compute_from_path:../test-assets/lena.jpg 64.6±0.30µs 64.6±1.06µs 0.00%
crc32_resource_id_creation/compute_from_path:../test-assets/test.pdf 930.5±5.83µs 933.8±2.48µs +0.35%
index_build/index_build/../test-assets/ 1057.2±3.86µs 1078.0±3.83µs +1.97%

Signed-off-by: pushkarm029 <[email protected]>
@Pushkarm029 Pushkarm029 requested a review from tareknaser May 25, 2024 04:52
Copy link

Benchmark for e6b12bd

Click to view benchmark
Test Base PR %
blake3_resource_id_creation/compute_from_bytes:large 245.9±3.11µs 247.4±2.63µs +0.61%
blake3_resource_id_creation/compute_from_bytes:medium 15.4±0.18µs 15.5±0.15µs +0.65%
blake3_resource_id_creation/compute_from_bytes:small 1334.4±15.54ns 1316.7±21.81ns -1.33%
blake3_resource_id_creation/compute_from_path:../test-assets/lena.jpg 193.7±2.48µs 193.5±2.56µs -0.10%
blake3_resource_id_creation/compute_from_path:../test-assets/test.pdf 1702.2±14.73µs 1689.2±32.04µs -0.76%
crc32_resource_id_creation/compute_from_bytes:large 85.2±1.43µs 85.4±1.09µs +0.23%
crc32_resource_id_creation/compute_from_bytes:medium 5.3±0.07µs 5.4±0.12µs +1.89%
crc32_resource_id_creation/compute_from_bytes:small 91.2±1.19ns 91.3±1.35ns +0.11%
crc32_resource_id_creation/compute_from_path:../test-assets/lena.jpg 63.8±0.98µs 63.9±0.78µs +0.16%
crc32_resource_id_creation/compute_from_path:../test-assets/test.pdf 923.1±10.92µs 940.0±17.46µs +1.83%
index_build/index_build/../test-assets/ 1076.1±12.94µs 1050.9±16.41µs -2.34%

@kirillt
Copy link
Member

kirillt commented May 25, 2024

Last change:

  1. Returning Err in case of any error, now we don't separate case of non-existing file or other error. Ok(true) means that the storage exists and it's outdated (or our model is outdated). Developers using the crate are free to try syncing if they encounter Err.
  2. Renamed the is_outdated function and enhanced comments to highlight the symmetry between the model and storage. Any of them could be outdated, or both of them. New name is needs_syncing.

@kirillt kirillt merged commit 07cbdd0 into main May 25, 2024
7 checks passed
@kirillt
Copy link
Member

kirillt commented May 25, 2024

Thanks @Pushkarm029 and @twitu as well as @tareknaser! Great team work 👍

Copy link

Benchmark for 2058a72

Click to view benchmark
Test Base PR %
blake3_resource_id_creation/compute_from_bytes:large 249.7±3.77µs 250.5±0.91µs +0.32%
blake3_resource_id_creation/compute_from_bytes:medium 15.6±0.22µs 15.6±0.14µs 0.00%
blake3_resource_id_creation/compute_from_bytes:small 1366.8±38.03ns 1347.4±20.38ns -1.42%
blake3_resource_id_creation/compute_from_path:../test-assets/lena.jpg 195.8±1.35µs 195.8±2.57µs 0.00%
blake3_resource_id_creation/compute_from_path:../test-assets/test.pdf 1714.1±4.78µs 1732.9±29.29µs +1.10%
crc32_resource_id_creation/compute_from_bytes:large 86.7±0.40µs 86.7±0.37µs 0.00%
crc32_resource_id_creation/compute_from_bytes:medium 5.4±0.14µs 5.4±0.09µs 0.00%
crc32_resource_id_creation/compute_from_bytes:small 92.5±1.11ns 92.8±3.08ns +0.32%
crc32_resource_id_creation/compute_from_path:../test-assets/lena.jpg 64.5±0.34µs 64.8±0.75µs +0.47%
crc32_resource_id_creation/compute_from_path:../test-assets/test.pdf 934.3±13.08µs 938.4±20.86µs +0.44%
index_build/index_build/../test-assets/ 1063.7±7.48µs 1064.8±10.78µs +0.10%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants