From 27c2882b1019cd804d777425cc2424a72e59bb74 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 8 Dec 2024 22:14:12 -0800 Subject: [PATCH] progress: fix progress to not redraw every time there's new data I think the idea of the code was try do 30 updates per second even if events arrive at, say, every 20 milliseconds. If we had reset the timer every time we printed, we would otherwise reset the timer every 40 milliseconds and end up with 25 updates per second. However, a bug in the code caused it to print every update because it always set the threshold to print the next update to `now`. I tried to keep what I think was the intent of the original code while fixing the bug. --- CHANGELOG.md | 3 +++ cli/src/progress.rs | 16 ++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efd717a73c..27bd4f7989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * Fixed incompatible rendering of empty hunks in git/unified diffs. [#5049](https://github.com/martinvonz/jj/issues/5049) +* Fixed performance of progress bar rendering when fetching from Git remote. + [#5057](https://github.com/martinvonz/jj/issues/5057) + ## [0.24.0] - 2024-12-04 ### Release highlights diff --git a/cli/src/progress.rs b/cli/src/progress.rs index 55b4dc0be1..991d2ea1a5 100644 --- a/cli/src/progress.rs +++ b/cli/src/progress.rs @@ -53,6 +53,7 @@ impl Progress { if now < self.next_print { return Ok(()); } + self.next_print = now + Duration::from_secs(1) / UPDATE_HZ; if self.guard.is_none() { let guard = output.output_guard(crossterm::cursor::Show.to_string()); let guard = CleanupGuard::new(move || { @@ -61,7 +62,6 @@ impl Progress { _ = write!(output, "{}", crossterm::cursor::Hide); self.guard = Some(guard); } - self.next_print = now.min(self.next_print + Duration::from_secs(1) / UPDATE_HZ); self.buffer.clear(); write!(self.buffer, "\r").unwrap(); @@ -260,14 +260,14 @@ mod tests { // First output is after the initial delay assert_snapshot!(update(INITIAL_DELAY - Duration::from_millis(1), 0.1), @""); assert_snapshot!(update(Duration::from_millis(1), 0.10), @"[?25l\r 10% [█▊ ]"); - // TODO: No updates for the next 30 milliseconds - assert_snapshot!(update(Duration::from_millis(10), 0.11), @" 11% [██ ]"); - assert_snapshot!(update(Duration::from_millis(10), 0.12), @" 12% [██▏ ]"); - assert_snapshot!(update(Duration::from_millis(10), 0.13), @" 13% [██▍ ]"); + // No updates for the next 30 milliseconds + assert_snapshot!(update(Duration::from_millis(10), 0.11), @""); + assert_snapshot!(update(Duration::from_millis(10), 0.12), @""); + assert_snapshot!(update(Duration::from_millis(10), 0.13), @""); // We get an update now that we go over the threshold assert_snapshot!(update(Duration::from_millis(100), 0.30), @" 30% [█████▍ ]"); - // TODO: Even though we went over by quite a bit, the new threshold is relative - // to the previous output, so we don't get an update here - assert_snapshot!(update(Duration::from_millis(30), 0.40), @" 40% [███████▎ ]"); + // Even though we went over by quite a bit, the new threshold is relative to the + // previous output, so we don't get an update here + assert_snapshot!(update(Duration::from_millis(30), 0.40), @""); } }