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

Performance expectations #47

Closed
AdamSimpson opened this issue Apr 27, 2023 · 8 comments · Fixed by #51
Closed

Performance expectations #47

AdamSimpson opened this issue Apr 27, 2023 · 8 comments · Fixed by #51
Assignees
Labels
enhancement New feature or request semver-minor New feature or other non-breaking addition

Comments

@AdamSimpson
Copy link

AdamSimpson commented Apr 27, 2023

cacache version: v11.3.0
rustc: 1.69.0

I'd like to use cacache in a performance sensitive area of my application, but I have found there to be more overhead than hoped. For instance on my M2 MacBook Air(2,800 MBps read SSD) it takes ~90ms to read a 30MB file from cache(sync or async), while reading the file directly from disk takes ~15ms. I see similar performance within my application as well as within microbenchmarks, so I don't believe this is simply bad benchmarking.

I'm not familiar with the caching strategy, so maybe this is expected? Possibly a system(MacOS) specific issue? Bad benchmarking?

@zkat
Copy link
Owner

zkat commented Apr 27, 2023

Cacache will be strictly slower than direct file access because it builds on top of it and involves a bit of overhead. There "official" benchmarks I use to compare various scenarios tend to show smaller gaps than what you're reporting, though.

The first thing that comes to mind is that there will definitely be a big gap between sync and async access.

@AdamSimpson
Copy link
Author

I added a few timers to read() and it looks like the integrety check is very time consuming. I don't have any feeling for if this is a reasonable amount of time for the check, but it was unexpected.

FWIW I also tested on a linux box and see similar perf.

content_path time: 1.458µs

read time: 3.023791ms

check time: 86.757209ms

@zkat
Copy link
Owner

zkat commented Apr 28, 2023

I guess that's not entirely unexpected but it definitely feels like more than it should be. Then again, sha256 can be expensive?

@AdamSimpson
Copy link
Author

I ran some benchmarks with the sha2 crate and on my system it looks to be the cost of business. I also tested xxhash, for some use cases that might be of interest :)

sha256_30mb_file        time:   [87.594 ms 87.714 ms 87.840 ms]                             
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild

xxhash_30mb_file        time:   [1.9958 ms 1.9993 ms 2.0033 ms]                              
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

@zkat
Copy link
Owner

zkat commented Apr 30, 2023

It might be interesting to make xxhash or a similar high-perf non-crypto algo available behind a feature flag. All my use cases for cacache necessitate cryptographic algos, though, so it's unlikely this is something I would do myself. Happy to take a PR, though!

@zkat
Copy link
Owner

zkat commented May 20, 2023

After some thinking, I've decided to add xxhash support to cacache, as I've found a specific use case where I don't need the cryptographic guarantees of sha2. Assigning this to myself!

@zkat zkat self-assigned this May 20, 2023
@zkat zkat added enhancement New feature or request semver-minor New feature or other non-breaking addition labels May 20, 2023
zkat added a commit that referenced this issue May 21, 2023
zkat added a commit that referenced this issue May 21, 2023
@zkat zkat closed this as completed in #51 May 21, 2023
zkat added a commit that referenced this issue May 21, 2023
@zkat
Copy link
Owner

zkat commented May 21, 2023

@AdamSimpson I've added xxhash support and released it as part of v11.6.0. Wanna give it a whirl and tell me what you think? :)

@AdamSimpson
Copy link
Author

So sorry for missing this, but this is perfect. Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor New feature or other non-breaking addition
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants