From 53b462875ffa98d2c171726c47b68dc42edbab48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Dupr=C3=A9?= Date: Fri, 9 Feb 2024 15:33:51 +0100 Subject: [PATCH] fix members of a multibar always rendering --- src/draw_target.rs | 59 +++++++++++++++++++++++++++---- src/multi.rs | 86 +++++++++++++++++++++++++++++++++++++++++---- src/progress_bar.rs | 2 +- 3 files changed, 132 insertions(+), 15 deletions(-) diff --git a/src/draw_target.rs b/src/draw_target.rs index b99b2396..5679a49e 100644 --- a/src/draw_target.rs +++ b/src/draw_target.rs @@ -146,6 +146,18 @@ impl ProgressDrawTarget { } } + /// Check if a call to `drawable` would actually give back a `Drawable`. + pub(crate) fn will_allow_draw(&self, now: Instant) -> bool { + match &self.kind { + TargetKind::Term { rate_limiter, .. } => rate_limiter.will_allow(now), + TargetKind::Multi { state, .. } => state.read().unwrap().will_allow_draw(now), + TargetKind::Hidden => false, + TargetKind::TermLike { rate_limiter, .. } => { + rate_limiter.as_ref().map_or(true, |r| r.will_allow(now)) + } + } + } + /// Apply the given draw state (draws it). pub(crate) fn drawable(&mut self, force_draw: bool, now: Instant) -> Option> { match &mut self.kind { @@ -169,13 +181,32 @@ impl ProgressDrawTarget { } } TargetKind::Multi { idx, state, .. } => { - let state = state.write().unwrap(); - Some(Drawable::Multi { - idx: *idx, - state, - force_draw, - now, - }) + let mut state_guard = state.write().unwrap(); + + // Check if this multibar's inner draw target will rate limit + // the draw call. If so no rendering needs to be done as long + // as we mark the member for redraw. Either case no actual + // request needs to be performed on inner rate limiter otherwise + // the actual draw call won't be able to perform. + if force_draw || state_guard.will_allow_draw(now) { + // Ensures no deadlock appears by trying to redraw current bar + state_guard.unmark_delayed(*idx); + + let state = MultiState::force_redraw_delayed(state_guard, now) + .ok() + .flatten() + .unwrap_or_else(|| state.write().unwrap()); + + Some(Drawable::Multi { + idx: *idx, + state, + force_draw, + now, + }) + } else { + state_guard.mark_delayed(*idx); // needs to be drawn later + None + } } TargetKind::TermLike { inner, @@ -412,6 +443,20 @@ impl RateLimiter { } } + /// Check ahead if a call to `allow` will return `true`. + fn will_allow(&self, now: Instant) -> bool { + if now < self.prev { + return false; + } + + let elapsed = now - self.prev; + + // If `capacity` is 0 and not enough time (`self.interval` ms) has passed since + // `self.prev` to add new capacity, return `false`. The goal of this method is to + // make this decision as efficient as possible. + self.capacity != 0 || elapsed >= Duration::from_millis(self.interval as u64) + } + fn allow(&mut self, now: Instant) -> bool { if now < self.prev { return false; diff --git a/src/multi.rs b/src/multi.rs index 3a372866..c59f9ebb 100644 --- a/src/multi.rs +++ b/src/multi.rs @@ -1,6 +1,6 @@ use std::fmt::{Debug, Formatter}; use std::io; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, RwLock, RwLockWriteGuard}; use std::thread::panicking; #[cfg(not(target_arch = "wasm32"))] use std::time::Instant; @@ -9,6 +9,7 @@ use crate::draw_target::{ visual_line_count, DrawState, DrawStateWrapper, LineAdjust, ProgressDrawTarget, VisualLines, }; use crate::progress_bar::ProgressBar; +use crate::WeakProgressBar; #[cfg(target_arch = "wasm32")] use instant::Instant; @@ -159,7 +160,7 @@ impl MultiProgress { fn internalize(&self, location: InsertLocation, pb: ProgressBar) -> ProgressBar { let mut state = self.state.write().unwrap(); - let idx = state.insert(location); + let idx = state.insert(location, &pb); drop(state); pb.set_draw_target(ProgressDrawTarget::new_remote(self.state.clone(), idx)); @@ -263,6 +264,54 @@ impl MultiState { self.remove_idx(index); } + /// Mark that a given member has had its state changed without a redraw + pub(crate) fn mark_delayed(&mut self, idx: usize) { + self.members[idx].is_delayed = true; + } + + /// Mark that a given member does not need to be redrawn + pub(crate) fn unmark_delayed(&mut self, idx: usize) { + self.members[idx].is_delayed = false; + } + + /// Force the redraw of all delayed members. + /// + /// The input lock guard will be given back if and only if there was no + /// member that had to be redrawn. + pub(crate) fn force_redraw_delayed( + mut this: RwLockWriteGuard<'_, Self>, + now: Instant, + ) -> io::Result>> { + let to_redraw: Vec<_> = this + .members + .iter_mut() + .filter(|mb| mb.is_delayed) + .filter_map(|mb| { + mb.is_delayed = false; + mb.pb_weak.upgrade() + }) + .collect(); + + if to_redraw.is_empty() { + return Ok(Some(this)); + } + + // The members will need to borrow this multibar to redraw! + drop(this); + + for pb in to_redraw { + pb.state().draw(true, now)?; + } + + Ok(None) + } + + /// Check if the draw target will allow to draw. If not, the member will + /// have to be marked as delayed to redraw it later. + pub(crate) fn will_allow_draw(&self, now: Instant) -> bool { + self.draw_target.will_allow_draw(now) + } + pub(crate) fn draw( &mut self, mut force_draw: bool, @@ -399,12 +448,12 @@ impl MultiState { self.draw_target.width() } - fn insert(&mut self, location: InsertLocation) -> usize { + fn insert(&mut self, location: InsertLocation, pb: &ProgressBar) -> usize { let idx = if let Some(idx) = self.free_set.pop() { - self.members[idx] = MultiStateMember::default(); + self.members[idx] = MultiStateMember::new(pb); idx } else { - self.members.push(MultiStateMember::default()); + self.members.push(MultiStateMember::new(pb)); self.members.len() - 1 }; @@ -454,7 +503,7 @@ impl MultiState { return; } - self.members[idx] = MultiStateMember::default(); + self.members[idx] = MultiStateMember::inactive(); self.free_set.push(idx); self.ordering.retain(|&x| x != idx); @@ -470,13 +519,36 @@ impl MultiState { } } -#[derive(Default)] struct MultiStateMember { /// Draw state will be `None` for members that haven't been drawn before, or for entries that /// correspond to something in the free set. draw_state: Option, /// Whether the corresponding progress bar (more precisely, `BarState`) has been dropped. is_zombie: bool, + /// Mark the member if he has not been redrawn after its last state change + is_delayed: bool, + /// Used to trigger a redraw if this member has been delayed + pb_weak: WeakProgressBar, +} + +impl MultiStateMember { + fn new(pb: &ProgressBar) -> Self { + Self { + draw_state: None, + is_zombie: false, + is_delayed: false, + pb_weak: pb.downgrade(), + } + } + + fn inactive() -> Self { + Self { + draw_state: None, + is_zombie: false, + is_delayed: false, + pb_weak: WeakProgressBar::default(), + } + } } impl Debug for MultiStateMember { diff --git a/src/progress_bar.rs b/src/progress_bar.rs index 036ab1ef..bf738ec8 100644 --- a/src/progress_bar.rs +++ b/src/progress_bar.rs @@ -245,7 +245,7 @@ impl ProgressBar { /// [`ProgressBar::suspend()`]: ProgressBar::suspend /// [`MultiProgress`]: crate::MultiProgress pub fn println>(&self, msg: I) { - self.state().println(Instant::now(), msg.as_ref()); + let _ = self.state().println(Instant::now(), msg.as_ref()); } /// Update the `ProgressBar`'s inner [`ProgressState`]