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
Limit progress update to fix 100% cpu, larger buffer and show speed per chunk #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
I am testing your changes and the CPU is cooling down :-)
I just add two comments - if you can update the code now it can be great, else I will update this one later.
Nice update! 👍
use response::CheckResponseStatus; | ||
use std::cmp::min; | ||
use std::io::Read; | ||
use std::thread; | ||
use std::time::{Instant, Duration}; | ||
|
||
const DOWNLOAD_BUFFER_BYTES: usize = 1024*64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add a single comment to explain why you multiply a buffer size (1024) by 64, please :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A larger buffer means less context switches between the kernel and the snatch process.
The multiplication is just to it's clear that it is 64KiB.
2048byte buffer means 5120 loops per second on 10MiB/s
64KiB is 81 loops in the best case.
There are also Disk seek times on HDD and page sizes on SSDs to consider.
let mut sum_bytes = 0; | ||
|
||
let progress_update_interval = Duration::from_millis(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can extract the 500 value in a constant (like REFRESH_DURATION) - it would be nice if we need to update this one later.
Thanks a lot 👍 |
Oh god, u32 is wrong :( it must be u64. |
No problem - I fixed the issue ;-) |
Thanks :) |
I was trying to make a constant for the interval duration, but it does not seem possible to create a
Duration
in a static/const becauseDuration
fields are private and calls are not permitted in const/static expressions.CPU usage is way down with the 500ms progress update interval.