Progress bar flickers #493

New issue

Closed

opened 2024-08-30 00:19:22 +00:00 by jade · 3 comments

jade commented

2024-08-30 00:19:22 +00:00

Owner

Note that there is a fix-ish PR here by roberth, which prevents redrawing the progress bar if there is no change in the rendered text: https://github.com/NixOS/nix/pull/11380

Whether this is the correct fix, or whether this is better solved by sending fewer progress updates to begin with, is another question entirely. Or perhaps both. Either way, there is room for improvement in this subsystem.

But the flickering is, in general, because we are redrawing the progress bar very often, and often unnecessarily so.

Note that there is a fix-ish PR here by roberth, which prevents redrawing the progress bar if there is no change in the rendered text: https://github.com/NixOS/nix/pull/11380 Whether this is the correct fix, or whether this is better solved by sending fewer progress updates *to begin with*, is another question entirely. Or perhaps both. Either way, there is room for improvement in this subsystem. But the flickering is, in general, because we are redrawing the progress bar very often, and often unnecessarily so.

jade added the

bug

E/help wanted

labels 2024-08-30 00:19:23 +00:00

lix-bot commented

2024-08-30 02:39:35 +00:00

Member

This issue was mentioned on Gerrit on the following CLs:

comment in cl/1862 ("libstore: use notifications for stats counters")

This issue was mentioned on Gerrit on the following CLs: * comment in [cl/1862](https://gerrit.lix.systems/c/lix/+/1862) ("libstore: use notifications for stats counters")

just1602 commented

2024-08-30 18:49:07 +00:00

I have this patch that does the same thing as the 11380 nixos PR, but I have the following question:

nixos ProgressBar still doesn't support multiline and multiline-with-logs, so they still only have one statement that generate output. I'm wondering if using a string and concatenating all the previous writeLogsToStderr calls to the string before returning it is the right way to do that
I'm wondering what would be the best way to test this, at least manually, since I still have the impression that it's flaky sometime, but I don't know if it's due to the fact that my terminal (alacritty) is slow / has bad latency or my desktop is powerful enough to make it look flaky anyways because it takes very little time to compile stuff.

If you prefer, I can open a CL on gerrit, but I wasn't sure if it was worth it since I cannot really prove that it's better with this patch.

diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc
index cdb15d8c7..2afd3479b 100644
--- a/src/libmain/progress-bar.cc
+++ b/src/libmain/progress-bar.cc
@@ -6,6 +6,7 @@
 #include "strings.hh"
 
 #include <map>
+#include <optional>
 #include <thread>
 #include <sstream>
 #include <iostream>
@@ -329,9 +330,22 @@ void ProgressBar::eraseProgressDisplay(State & state)
     }
 }
 
-std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s)
+std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s) {
+
+    std::optional<std::string> newOutput;
+    auto nextWakeup = draw(state, s, newOutput);
+    if(newOutput && *newOutput != lastOutput) {
+        writeLogsToStderr(*newOutput);
+        lastOutput = std::move(*newOutput);
+    }
+
+    return nextWakeup;
+}
+
+std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s, std::optional<std::string> & output)
 {
     auto nextWakeup = A_LONG_TIME;
+    std::string out = "";
 
     state.haveUpdate = false;
     if (state.paused > 0) return nextWakeup;
@@ -347,7 +361,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
     state.lastLines = 0;
 
     if (s != std::nullopt)
-        writeLogsToStderr(filterANSIEscapes(s.value(), !isTTY) + ANSI_NORMAL "\n");
+        out += filterANSIEscapes(s.value(), !isTTY) + ANSI_NORMAL "\n";
 
     std::string line;
     std::string status = getStatus(state);
@@ -357,12 +371,12 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
         line += "]";
     }
     if (printMultiline && !line.empty()) {
-        writeLogsToStderr(filterANSIEscapes(line, false, width) + ANSI_NORMAL "\n");
+        out += filterANSIEscapes(line, false, width) + ANSI_NORMAL "\n";
         state.lastLines++;
     }
 
-     auto height = windowSize.first > 0 ? windowSize.first : 25;
-     auto moreActivities = 0;
+    auto height = windowSize.first > 0 ? windowSize.first : 25;
+    auto moreActivities = 0;
     auto now = std::chrono::steady_clock::now();
 
     std::string activity_line;
@@ -399,7 +413,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
 
             if (printMultiline) {
                 if (state.lastLines < (height -1)) {
-                    writeLogsToStderr(filterANSIEscapes(activity_line, false, width) + ANSI_NORMAL "\n");
+                    out += filterANSIEscapes(activity_line, false, width) + ANSI_NORMAL "\n";
                     state.lastLines++;
                 } else moreActivities++;
             }
@@ -407,7 +421,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
     }
 
     if (printMultiline && moreActivities)
-        writeLogsToStderr(fmt("And %d more...", moreActivities));
+        out += fmt("And %d more...", moreActivities);
 
     if (!printMultiline) {
         if (!line.empty()) {
@@ -415,10 +429,16 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s
         }
         line += activity_line;
         if (!line.empty()) {
-            writeLogsToStderr(filterANSIEscapes(line, false, width) + ANSI_NORMAL);
+            out += filterANSIEscapes(line, false, width) + ANSI_NORMAL;
         }
     }
 
+    if(out.empty()) {
+        output = std::nullopt;
+    } else {
+        output = std::move(out);
+    }
+
     return nextWakeup;
 }
 
diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh
index 8343beff1..1d1714859 100644
--- a/src/libmain/progress-bar.hh
+++ b/src/libmain/progress-bar.hh
@@ -62,6 +62,9 @@ struct ProgressBar : public Logger
     bool printMultiline = false;
     bool isTTY;
 
+    // Use to avoid useless redraw of the ProgressBar
+    std::string lastOutput;
+
     ProgressBar(bool isTTY);
 
     ~ProgressBar();
@@ -111,6 +114,12 @@ struct ProgressBar : public Logger
 
 private:
     void eraseProgressDisplay(State & state);
+
+    std::chrono::milliseconds draw(
+        State & state,
+        const std::optional<std::string_view> & s,
+        std::optional<std::string> & output
+    );
 };
 
 Logger * makeProgressBar();

I have this patch that does the same thing as the 11380 nixos PR, but I have the following question: 1. nixos `ProgressBar` still doesn't support `multiline` and `multiline-with-logs`, so they still only have one statement that generate output. I'm wondering if using a string and concatenating all the previous `writeLogsToStderr` calls to the string before returning it is the right way to do that 2. I'm wondering what would be the best way to test this, at least manually, since I still have the impression that it's flaky sometime, but I don't know if it's due to the fact that my terminal (alacritty) is slow / has bad latency or my desktop is powerful enough to make it look flaky anyways because it takes very little time to compile stuff. If you prefer, I can open a CL on gerrit, but I wasn't sure if it was worth it since I cannot really prove that it's better with this patch. ```diff diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index cdb15d8c7..2afd3479b 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -6,6 +6,7 @@ #include "strings.hh" #include <map> +#include <optional> #include <thread> #include <sstream> #include <iostream> @@ -329,9 +330,22 @@ void ProgressBar::eraseProgressDisplay(State & state) } } -std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s) +std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s) { + + std::optional<std::string> newOutput; + auto nextWakeup = draw(state, s, newOutput); + if(newOutput && *newOutput != lastOutput) { + writeLogsToStderr(*newOutput); + lastOutput = std::move(*newOutput); + } + + return nextWakeup; +} + +std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<std::string_view> & s, std::optional<std::string> & output) { auto nextWakeup = A_LONG_TIME; + std::string out = ""; state.haveUpdate = false; if (state.paused > 0) return nextWakeup; @@ -347,7 +361,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s state.lastLines = 0; if (s != std::nullopt) - writeLogsToStderr(filterANSIEscapes(s.value(), !isTTY) + ANSI_NORMAL "\n"); + out += filterANSIEscapes(s.value(), !isTTY) + ANSI_NORMAL "\n"; std::string line; std::string status = getStatus(state); @@ -357,12 +371,12 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s line += "]"; } if (printMultiline && !line.empty()) { - writeLogsToStderr(filterANSIEscapes(line, false, width) + ANSI_NORMAL "\n"); + out += filterANSIEscapes(line, false, width) + ANSI_NORMAL "\n"; state.lastLines++; } - auto height = windowSize.first > 0 ? windowSize.first : 25; - auto moreActivities = 0; + auto height = windowSize.first > 0 ? windowSize.first : 25; + auto moreActivities = 0; auto now = std::chrono::steady_clock::now(); std::string activity_line; @@ -399,7 +413,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s if (printMultiline) { if (state.lastLines < (height -1)) { - writeLogsToStderr(filterANSIEscapes(activity_line, false, width) + ANSI_NORMAL "\n"); + out += filterANSIEscapes(activity_line, false, width) + ANSI_NORMAL "\n"; state.lastLines++; } else moreActivities++; } @@ -407,7 +421,7 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s } if (printMultiline && moreActivities) - writeLogsToStderr(fmt("And %d more...", moreActivities)); + out += fmt("And %d more...", moreActivities); if (!printMultiline) { if (!line.empty()) { @@ -415,10 +429,16 @@ std::chrono::milliseconds ProgressBar::draw(State & state, const std::optional<s } line += activity_line; if (!line.empty()) { - writeLogsToStderr(filterANSIEscapes(line, false, width) + ANSI_NORMAL); + out += filterANSIEscapes(line, false, width) + ANSI_NORMAL; } } + if(out.empty()) { + output = std::nullopt; + } else { + output = std::move(out); + } + return nextWakeup; } diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index 8343beff1..1d1714859 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -62,6 +62,9 @@ struct ProgressBar : public Logger bool printMultiline = false; bool isTTY; + // Use to avoid useless redraw of the ProgressBar + std::string lastOutput; + ProgressBar(bool isTTY); ~ProgressBar(); @@ -111,6 +114,12 @@ struct ProgressBar : public Logger private: void eraseProgressDisplay(State & state); + + std::chrono::milliseconds draw( + State & state, + const std::optional<std::string_view> & s, + std::optional<std::string> & output + ); }; Logger * makeProgressBar(); ```

jade commented

2024-11-12 22:51:54 +00:00

Author

Owner

Fixed by: https://gerrit.lix.systems/c/lix/+/2174, I think

jade closed this issue

2024-11-12 22:51:54 +00:00