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

Download benchmark results: set concurrent range gets to 8 #146

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

assafvayner
Copy link
Contributor

From the results of the benchmarks, we should bump up the number of ranges downloaded at a time to a maximum of 8. With no change to the number of threads used.


details:

  • the benchmark is not comprehensive over all data types. Only file tested was a 5 GB safetensors file with 75 terms and largely no dedupe. The environment used was a t3.2xl ec2 instance; fast link to s3.
  • the benchmark showed that the current chunk cache is a significant bottleneck (causing download times to nearly triple ~15s -> ~40s). Cache changes will be addressed in later pull request/s.
  • Changing the number of system threads used had largely no/minor impact on the download time.
  • Changing the number of maximum buffered terms did have an impact.
    • with caching disabled 16 was marginally better than 8
    • with caching enabled 8 was marginally better than 16
    • in any case 4 (current) was too low
    • attempting to download all terms at once has a marginal benefit with caching disabled, while yielding worse performance with the current caching implementation likely stemming from lock contention.

@assafvayner assafvayner merged commit d5ced3d into main Jan 22, 2025
2 checks passed
@assafvayner assafvayner deleted the assaf/benchmark-results-bump-concurrency branch January 22, 2025 20:09
@assafvayner assafvayner restored the assaf/benchmark-results-bump-concurrency branch January 28, 2025 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants