console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.47k stars 244 forks source link

fix incorrect remainder calculation in RateLimiter #565

Closed taoky closed 1 year ago

taoky commented 1 year ago

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:

https://github.com/console-rs/indicatif/assets/2109893/681e46ee-7a48-4c4f-87cd-18c088d68c4a

After this fix:

https://github.com/console-rs/indicatif/assets/2109893/8dfbb070-ccdf-4e35-9de2-154d137f6e0e

taoky commented 1 year ago

Well, and this may explain what is recorded in https://github.com/console-rs/indicatif/issues/452#issuecomment-1219670708.