Skip to content

Commit

Permalink
Fix incorrect seconds_per_step calculation
Browse files Browse the repository at this point in the history
seconds_per_step looks at a ring buffer with samples taken each
tick. Each sample is

    <duration of the tick> / <number of progress steps>

This results in an incorrect seconds_per_step, when this is
calculated as a raw average of the buffer values. This is because
we have no reason to assume any tick is a more representative sample
than any other tick, but this will treat ticks with very few steps
as *more* representative than steps with many ticks, because the
value of the denominator is lost.

The result is a very poor ETA / bitrate estimation when step
production follows a burst-wait pattern, since ticks with few or
no steps are to be expected.

A better estimate could be achieved by averaging over the step-rate
of each tick, but this would still not be idea since it would treat
short ticks as equally representative as long ticks in the average.

Instead, we add a second ring buffer for the duration of each tick,
and use this to calculate a *true* weighted average when
determining seconds_per_step.
  • Loading branch information
afontenot committed Apr 24, 2023
1 parent 4987fc5 commit c62e3a8
Showing 1 changed file with 12 additions and 11 deletions.
23 changes: 12 additions & 11 deletions src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,8 @@ impl TabExpandedString {
/// Ring buffer with constant capacity. Used by `ProgressBar`s to display `{eta}`,
/// `{eta_precise}`, and `{*_per_sec}`.
pub(crate) struct Estimator {
steps: [f64; 16],
steps: [u64; 16],
step_durations: [f64; 16],
pos: u8,
full: bool,
prev: (u64, Instant),
Expand All @@ -379,7 +380,8 @@ pub(crate) struct Estimator {
impl Estimator {
fn new(now: Instant) -> Self {
Self {
steps: [0.0; 16],
steps: [0; 16],
step_durations: [0.0; 16],
pos: 0,
full: false,
prev: (0, now),
Expand All @@ -388,7 +390,9 @@ impl Estimator {

fn record(&mut self, new: u64, now: Instant) {
let delta = new.saturating_sub(self.prev.0);
if delta == 0 || now < self.prev.1 {
let elapsed = duration_to_secs(now - self.prev.1);

if delta == 0 || elapsed < 0.0 {
// Reset on backwards seek to prevent breakage from seeking to the end for length determination
// See https://github.com/console-rs/indicatif/issues/480
if new < self.prev.0 {
Expand All @@ -397,14 +401,9 @@ impl Estimator {
return;
}

let elapsed = now - self.prev.1;
let divisor = delta as f64;
let mut batch = 0.0;
if divisor != 0.0 {
batch = duration_to_secs(elapsed) / divisor;
};
self.steps[self.pos as usize] = delta;
self.step_durations[self.pos as usize] = elapsed;

self.steps[self.pos as usize] = batch;
self.pos = (self.pos + 1) % 16;
if !self.full && self.pos == 0 {
self.full = true;
Expand All @@ -422,7 +421,9 @@ impl Estimator {
/// Average time per step in seconds, using rolling buffer of last 15 steps
fn seconds_per_step(&self) -> f64 {
let len = self.len();
self.steps[0..len].iter().sum::<f64>() / len as f64
let total_steps = self.steps[0..len].iter().sum::<u64>();
let total_duration = self.step_durations[0..len].iter().sum::<f64>();
total_duration / total_steps as f64
}

fn len(&self) -> usize {
Expand Down

0 comments on commit c62e3a8

Please sign in to comment.