diff options
| -rw-r--r-- | runtime/doc/various.txt | 8 | ||||
| -rw-r--r-- | runtime/doc/vim_diff.txt | 7 | ||||
| -rw-r--r-- | src/nvim/memory.c | 31 | ||||
| -rw-r--r-- | src/nvim/os/shell.c | 156 | ||||
| -rw-r--r-- | test/functional/ui/output_spec.lua | 21 |
5 files changed, 141 insertions, 82 deletions
diff --git a/runtime/doc/various.txt b/runtime/doc/various.txt index 625416146e..3c1472446d 100644 --- a/runtime/doc/various.txt +++ b/runtime/doc/various.txt @@ -264,10 +264,10 @@ g8 Print the hex values of the bytes used in the After the command has been executed, the timestamp and size of the current file is checked |timestamp|. - If the command produces a lot of output the displayed - output will be "throttled" so the command can execute - quickly without waiting for the display. This only - affects the display, no data is lost. + If the command produces too much output some lines may + be skipped so the command can execute quickly. No + data is lost, this only affects the display. The last + few lines are always displayed (never skipped). Vim redraws the screen after the command is finished, because it may have printed any text. This requires a diff --git a/runtime/doc/vim_diff.txt b/runtime/doc/vim_diff.txt index 1940bc55fa..7ccdfd2bdd 100644 --- a/runtime/doc/vim_diff.txt +++ b/runtime/doc/vim_diff.txt @@ -155,10 +155,9 @@ are always available and may be used simultaneously in separate plugins. The |system()| does not support writing/reading "backgrounded" commands. |E5677| -Nvim truncates ("throttles") shell-command messages echoed by |:!|, |:grep|, -and |:make|. No data is lost, this only affects display. This makes things -faster, but may seem weird for commands like ":!cat foo". Use ":te cat foo" -instead, |:terminal| output is never throttled. +Nvim may throttle (skip) messages from shell commands (|:!|, |:grep|, |:make|) +if there is too much output. No data is lost, this only affects display and +makes things faster. |:terminal| output is never throttled. |mkdir()| behaviour changed: 1. Assuming /tmp/foo does not exist and /tmp can be written to diff --git a/src/nvim/memory.c b/src/nvim/memory.c index 3e041be4d3..071ef335cf 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -283,18 +283,16 @@ size_t memcnt(const void *data, char c, size_t len) return cnt; } -/// The xstpcpy() function shall copy the string pointed to by src (including -/// the terminating NUL character) into the array pointed to by dst. +/// Copies the string pointed to by src (including the terminating NUL +/// character) into the array pointed to by dst. /// -/// The xstpcpy() function shall return a pointer to the terminating NUL -/// character copied into the dst buffer. This is the only difference with -/// strcpy(), which returns dst. +/// @returns pointer to the terminating NUL char copied into the dst buffer. +/// This is the only difference with strcpy(), which returns dst. /// -/// WARNING: If copying takes place between objects that overlap, the behavior is -/// undefined. +/// WARNING: If copying takes place between objects that overlap, the behavior +/// is undefined. /// -/// This is the Neovim version of stpcpy(3) as defined in POSIX 2008. We -/// don't require that supported platforms implement POSIX 2008, so we +/// Nvim version of POSIX 2008 stpcpy(3). We do not require POSIX 2008, so /// implement our own version. /// /// @param dst @@ -306,16 +304,15 @@ char *xstpcpy(char *restrict dst, const char *restrict src) return (char *)memcpy(dst, src, len + 1) + len; } -/// The xstpncpy() function shall copy not more than n bytes (bytes that follow -/// a NUL character are not copied) from the array pointed to by src to the -/// array pointed to by dst. +/// Copies not more than n bytes (bytes that follow a NUL character are not +/// copied) from the array pointed to by src to the array pointed to by dst. /// -/// If a NUL character is written to the destination, the xstpncpy() function -/// shall return the address of the first such NUL character. Otherwise, it -/// shall return &dst[maxlen]. +/// If a NUL character is written to the destination, xstpncpy() returns the +/// address of the first such NUL character. Otherwise, it shall return +/// &dst[maxlen]. /// -/// WARNING: If copying takes place between objects that overlap, the behavior is -/// undefined. +/// WARNING: If copying takes place between objects that overlap, the behavior +/// is undefined. /// /// WARNING: xstpncpy will ALWAYS write maxlen bytes. If src is shorter than /// maxlen, zeroes will be written to the remaining bytes. diff --git a/src/nvim/os/shell.c b/src/nvim/os/shell.c index a300984f63..f7325b20f2 100644 --- a/src/nvim/os/shell.c +++ b/src/nvim/os/shell.c @@ -25,7 +25,9 @@ #include "nvim/charset.h" #include "nvim/strings.h" -#define DYNAMIC_BUFFER_INIT {NULL, 0, 0} +#define DYNAMIC_BUFFER_INIT { NULL, 0, 0 } +#define NS_1_SECOND 1000000000 // 1 second, in nanoseconds +#define OUT_DATA_THRESHOLD 1024 * 10U // 10KB, "a few screenfuls" of data. typedef struct { char *data; @@ -187,7 +189,8 @@ static int do_os_system(char **argv, bool silent, bool forward_output) { - out_data_throttle(NULL, 0); // Initialize throttle for this shell command. + out_data_decide_throttle(0); // Initialize throttle decider. + out_data_ring(NULL, 0); // Initialize output ring-buffer. // the output buffer DynamicBuffer buf = DYNAMIC_BUFFER_INIT; @@ -217,7 +220,7 @@ static int do_os_system(char **argv, proc->err = &err; if (!process_spawn(proc)) { loop_poll_events(&main_loop, 0); - // Failed, probably due to `sh` not being executable + // Failed, probably due to 'sh' not being executable if (!silent) { MSG_PUTS(_("\nCannot execute ")); msg_outtrans((char_u *)prog); @@ -260,6 +263,10 @@ static int do_os_system(char **argv, ui_busy_start(); ui_flush(); int status = process_wait(proc, -1, NULL); + if (!got_int && out_data_decide_throttle(0)) { + // Last chunk of output was skipped; display it now. + out_data_ring(NULL, SIZE_MAX); + } ui_busy_stop(); // prepare the out parameters if requested @@ -311,56 +318,50 @@ static void system_data_cb(Stream *stream, RBuffer *buf, size_t count, dbuf->len += nread; } -/// Tracks output received for the current executing shell command. To avoid -/// flooding the UI, output is periodically skipped and a pulsing "..." is -/// shown instead. Tracking depends on the synchronous/blocking nature of ":!". +/// Tracks output received for the current executing shell command, and displays +/// a pulsing "..." when output should be skipped. Tracking depends on the +/// synchronous/blocking nature of ":!". // /// Purpose: /// 1. CTRL-C is more responsive. #1234 #5396 -/// 2. Improves performance of :! (UI, esp. TUI, is the bottleneck here). +/// 2. Improves performance of :! (UI, esp. TUI, is the bottleneck). /// 3. Avoids OOM during long-running, spammy :!. /// -/// Note: -/// - Throttling "solves" the issue for *all* UIs, on all platforms. -/// - Unlikely that users will miss useful output: humans do not read >100KB. -/// - Caveat: Affects execute(':!foo'), but that is not a "very important" -/// case; system('foo') should be used for large outputs. -/// /// Vim does not need this hack because: /// 1. :! in terminal-Vim runs in cooked mode, so CTRL-C is caught by the /// terminal and raises SIGINT out-of-band. /// 2. :! in terminal-Vim uses a tty (Nvim uses pipes), so commands /// (e.g. `git grep`) may page themselves. /// -/// @returns true if output was skipped and pulse was displayed -static bool out_data_throttle(char *output, size_t size) +/// @param size Length of data, used with internal state to decide whether +/// output should be skipped. size=0 resets the internal state and +/// returns the previous decision. +/// +/// @returns true if output should be skipped and pulse was displayed. +/// Returns the previous decision if size=0. +static bool out_data_decide_throttle(size_t size) { -#define NS_1_SECOND 1000000000 // 1s, in ns -#define THRESHOLD 1024 * 10 // 10KB, "a few screenfuls" of data. static uint64_t started = 0; // Start time of the current throttle. static size_t received = 0; // Bytes observed since last throttle. static size_t visit = 0; // "Pulse" count of the current throttle. static size_t max_visits = 0; static char pulse_msg[] = { ' ', ' ', ' ', '\0' }; - if (output == NULL) { + if (!size) { + bool previous_decision = (visit > 0); // TODO: needs to check that last print shows more than a page started = received = visit = 0; - max_visits = 10; - return false; + max_visits = 20; + return previous_decision; } received += size; - if (received < THRESHOLD - // Display at least the first chunk of output even if it is >=THRESHOLD. + if (received < OUT_DATA_THRESHOLD + // Display at least the first chunk of output even if it is big. || (!started && received < size + 1000)) { return false; - } - - if (!visit) { + } else if (!visit) { started = os_hrtime(); - } - - if (visit >= max_visits) { + } else if (visit >= max_visits) { uint64_t since = os_hrtime() - started; if (since < NS_1_SECOND) { // Adjust max_visits based on the current relative performance. @@ -368,27 +369,74 @@ static bool out_data_throttle(char *output, size_t size) max_visits = (2 * max_visits); } else { received = visit = 0; + return false; } } - if (received && ++visit <= max_visits) { - // Pulse "..." at the bottom of the screen. - size_t tick = (visit == max_visits) - ? 3 // Force all dots "..." on last visit. - : (visit + 1) % 4; - pulse_msg[0] = (tick == 0) ? ' ' : '.'; - pulse_msg[1] = (tick == 0 || 1 == tick) ? ' ' : '.'; - pulse_msg[2] = (tick == 0 || 1 == tick || 2 == tick) ? ' ' : '.'; - if (visit == 1) { - screen_del_lines(0, 0, 1, (int)Rows, NULL); - } - int lastrow = (int)Rows - 1; - screen_puts_len((char_u *)pulse_msg, ARRAY_SIZE(pulse_msg), lastrow, 0, 0); - ui_flush(); - return true; + visit++; + // Pulse "..." at the bottom of the screen. + size_t tick = (visit == max_visits) + ? 3 // Force all dots "..." on last visit. + : (visit % 4); + pulse_msg[0] = (tick == 0) ? ' ' : '.'; + pulse_msg[1] = (tick == 0 || 1 == tick) ? ' ' : '.'; + pulse_msg[2] = (tick == 0 || 1 == tick || 2 == tick) ? ' ' : '.'; + if (visit == 1) { + screen_del_lines(0, 0, 1, (int)Rows, NULL); } + int lastrow = (int)Rows - 1; + screen_puts_len((char_u *)pulse_msg, ARRAY_SIZE(pulse_msg), lastrow, 0, 0); + ui_flush(); + return true; +} + +/// Saves output in a quasi-ringbuffer. Used to ensure the last ~page of +/// output for a shell-command is always displayed. +/// +/// Init mode: Resets the internal state. +/// output = NULL +/// size = 0 +/// Print mode: Displays the current saved data. +/// output = NULL +/// size = SIZE_MAX +/// +/// @param output Data to save, or NULL to invoke a special mode. +/// @param size Length of `output`. +static void out_data_ring(char *output, size_t size) +{ +#define MAX_CHUNK_SIZE (OUT_DATA_THRESHOLD / 2) + static char last_skipped[MAX_CHUNK_SIZE]; // Saved output. + static size_t last_skipped_len = 0; - return false; + assert(output != NULL || (size == 0 || size == SIZE_MAX)); + + if (output == NULL && size == 0) { // Init mode + last_skipped_len = 0; + return; + } + + if (output == NULL && size == SIZE_MAX) { // Print mode + out_data_append_to_screen(last_skipped, last_skipped_len, true); + return; + } + + // This is basically a ring-buffer... + if (size >= MAX_CHUNK_SIZE) { // Save mode + size_t start = size - MAX_CHUNK_SIZE; + memcpy(last_skipped, output + start, MAX_CHUNK_SIZE); + last_skipped_len = MAX_CHUNK_SIZE; + } else if (size > 0) { + // Length of the old data that can be kept. + size_t keep_len = MIN(last_skipped_len, MAX_CHUNK_SIZE - size); + size_t keep_start = last_skipped_len - keep_len; + // Shift the kept part of the old data to the start. + if (keep_start) { + memmove(last_skipped, last_skipped + keep_start, keep_len); + } + // Copy the entire new data to the remaining space. + memcpy(last_skipped + keep_len, output, size); + last_skipped_len = keep_len + size; + } } /// Continue to append data to last screen line. @@ -396,15 +444,10 @@ static bool out_data_throttle(char *output, size_t size) /// @param output Data to append to screen lines. /// @param remaining Size of data. /// @param new_line If true, next data output will be on a new line. -static void append_to_screen_end(char *output, size_t remaining, bool new_line) +static void out_data_append_to_screen(char *output, size_t remaining, + bool new_line) { - // Column of last row to start appending data to. - static colnr_T last_col = 0; - - if (out_data_throttle(output, remaining)) { - last_col = 0; - return; - } + static colnr_T last_col = 0; // Column of last row to append to. size_t off = 0; int last_row = (int)Rows - 1; @@ -457,7 +500,14 @@ static void out_data_cb(Stream *stream, RBuffer *buf, size_t count, void *data, size_t cnt; char *ptr = rbuffer_read_ptr(buf, &cnt); - append_to_screen_end(ptr, cnt, eof); + if (ptr != NULL && cnt > 0 + && out_data_decide_throttle(cnt)) { // Skip output above a threshold. + // Save the skipped output. If it is the final chunk, we display it later. + out_data_ring(ptr, cnt); + } else { + out_data_append_to_screen(ptr, cnt, eof); + } + if (cnt) { rbuffer_consumed(buf, cnt); } diff --git a/test/functional/ui/output_spec.lua b/test/functional/ui/output_spec.lua index 4aae2edfaa..d6d8f1c4e5 100644 --- a/test/functional/ui/output_spec.lua +++ b/test/functional/ui/output_spec.lua @@ -1,6 +1,5 @@ local session = require('test.functional.helpers')(after_each) local child_session = require('test.functional.terminal.helpers') -local Screen = require('test.functional.ui.screen') if session.pending_win32(pending) then return end @@ -41,10 +40,24 @@ describe("shell command :!", function() ]]) end) - it("throttles shell-command output greater than ~20KB", function() + it("throttles shell-command output greater than ~10KB", function() + screen.timeout = 20000 -- Avoid false failure on slow systems. child_session.feed_data( - ":!for i in $(seq 2 3000); do echo XXXXXXXXXX; done\n") - -- If a line with only a dot "." appears, then throttling was triggered. + ":!for i in $(seq 2 3000); do echo XXXXXXXXXX $i; done\n") + + -- If we observe any line starting with a dot, then throttling occurred. screen:expect("\n.", nil, nil, nil, true) + + -- Final chunk of output should always be displayed, never skipped. + -- (Throttling is non-deterministic, this test is merely a sanity check.) + screen:expect([[ + XXXXXXXXXX 2996 | + XXXXXXXXXX 2997 | + XXXXXXXXXX 2998 | + XXXXXXXXXX 2999 | + XXXXXXXXXX 3000 | + {10:Press ENTER or type command to continue}{1: } | + {3:-- TERMINAL --} | + ]]) end) end) |