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

fix incorrect remainder calculation in RateLimiter #565

Merged
merged 1 commit into from Jul 27, 2023

Conversation

taoky
Copy link
Contributor

@taoky taoky commented Jul 27, 2023

In RateLimiter, the remainder is calculated like this:

let (new, remainder) = (
    elapsed.as_millis() / self.interval as u128,
    elapsed.as_nanos() % self.interval as u128 * 1_000_000,
);

It looks like remainder is treated as milliseconds (% takes place prior to *). However, this seems off as it is shown later:

self.prev = now
    .checked_sub(Duration::from_nanos(remainder as u64))
    .unwrap();

remainder is actually used as nanoseconds.

By removing the multiply adjusting the order of calculation, it seems that the rate limiter is much more stable. See this MRE:

use std::time::Duration;

use indicatif::{ProgressDrawTarget, ProgressStyle};

fn main() {
    let pb = indicatif::ProgressBar::with_draw_target(Some(u64::MAX), ProgressDrawTarget::stdout_with_hz(1));
    pb.set_message(format!("Downloading"));
    pb.set_style(
        ProgressStyle::default_bar()
            .template("{msg}\n[{elapsed_precise}] {bytes}/{total_bytes}")
            .unwrap()
            .progress_chars("#>-"),
    );
    loop {
        pb.inc(1);
        std::thread::sleep(Duration::from_micros(1000));
    }
}

Before this fix:

Kooha-2023-07-27-16-45-46.mp4

After this fix:

Kooha-2023-07-27-16-46-19.mp4

@taoky
Copy link
Contributor Author

taoky commented Jul 27, 2023

Well, and this may explain what is recorded in #452 (comment).

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Wow, great catch, thanks!

@djc djc merged commit 91ba34e into console-rs:main Jul 27, 2023
9 checks passed
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.

None yet

2 participants