New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] throttle shell output to maintain responsiveness #5396

Merged
merged 5 commits into from Dec 10, 2016

Conversation

Projects
None yet
4 participants
@justinmk
Member

justinmk commented Sep 28, 2016

Closes #1234

  • Commit 1 (resetting the TUI event queue if it floods) is the main safeguard against #1234. But it's quite an edge case because yes produces tons (GB in just a few minutes) of data very quickly.
  • Commit 2 (skipping :! spam periodically) is a more general "cheat" that works for all UIs and greatly improves responsiveness in the TUI even when it is "spammed" but not "flooded".

Try these shell-based ex-commands before/after this PR:

:grep -r '' *
:make
:!yes
:!grep -r '' *
:!git grep ''
:!cat foo
:!echo foo
:!while true; do date; done
:!for i in `seq 1 20000`; do echo XXXXXXXXXX $i; done
  • Output will be periodically skipped and a pulsing "..." displayed. In all cases the last few lines of the command should always be shown (these are usually what the user cares about, if anything).
  • Data is not lost, so :read still reads all data; :make and :grep still send all lines to the quickfix list; etc.

Output from :! exceeding ~10 KB is not useful to users. This is even more true for nvim because:

  • :! and system() use pipes, the nvim built-in pager doesn't work, so any output that scrolled off the screen is gone
  • :! and system() use pipes, interactive programs like :!git grep '' won't treat the output as tty, and won't page themselves (unlike Vim)
  • This PR throttles output greater than ~10 KB.
    • Significant perceived performance improvement in terminal-based Nvim.
    • :read !foo and :<range>!foo still correctly receive all data; throttling only affects UI.
    • execute('!foo') behavior did not change. (Added a test for this.)
  • Future work (?): For user-initiated :! invocations, use :terminal instead?
    • This will be hard to make "perfect", but would be a valuable feature if possible.

Note

The same "TUI flooding" problem can be triggered by doing

:g/^/p

on a very large file and then pressing G to scroll to the bottom of the messages in the pager. This PR doesn't address :g (or anything that uses the built-in pager); this PR only address shell-command output.

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from ab44eab to 1364f7c Sep 28, 2016

@marvim marvim added the WIP label Sep 28, 2016

if (*p == NL) {
lines++;
}
}

This comment has been minimized.

@oni-link

oni-link Sep 29, 2016

Contributor

Could be replaced with memcnt().

This comment has been minimized.

@justinmk

justinmk Sep 29, 2016

Member

@oni-link Thanks! Instead of counting lines, I'm thinking some upper limit of "bytes seen" for a given source should trigger the throttling.

This comment has been minimized.

@justinmk

justinmk Sep 29, 2016

Member

An idea for a more sophisticated "throttle trigger", might be the keep a count of the pending thread_events per-queue and then maybe drop them until processing catches up.

But maybe the arbitrary limit (this PR) is good enough: it's probably not useful for anyone to see more than 1000 lines blasted in the pager, in any UI (TUI or GUI). So the approach I was thinking here was to "snip" long outputs: print the last ~500 lines (as well as the first ~500 lines).

remaining = 1;
if (last_col >= Columns) {
last_col = 0;
skip_msg[1] = '\n';

This comment has been minimized.

@oni-link

oni-link Sep 29, 2016

Contributor

Where would this field be read?

// QUEUE *q = QUEUE_HEAD(&queue->headtail);
// while(true) {
// }
ILOG("thread_events=%d", nrevents);

This comment has been minimized.

@oni-link

oni-link Sep 29, 2016

Contributor

Quite noisy (when testing with yes).

This comment has been minimized.

@justinmk

justinmk Sep 29, 2016

Member

Yep, I'll add a condition or remove this. yes can generate MB of data in a couple seconds, and GB in just a few minutes.

int nrevents = queue_count(data->loop->thread_events);
// QUEUE *q = QUEUE_HEAD(&queue->headtail);
// while(true) {
// }

This comment has been minimized.

@oni-link

oni-link Sep 29, 2016

Contributor

Will this be removed?

This comment has been minimized.

@justinmk

justinmk Sep 29, 2016

Member

Yes, this PR is very much unfinished, though I will send updates tonight.

@justinmk justinmk force-pushed the justinmk:tui-throttle branch 3 times, most recently from b82bbac to f839a71 Sep 29, 2016

@justinmk justinmk referenced this pull request Oct 5, 2016

Open

:terminal improvements #5431

5 of 17 tasks complete

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 21be8cf to 7d8f027 Oct 5, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 5, 2016

Rebased, RFC. Reviews appreciated.

Not planning to write new tests for this because it would be very slow. Testing a ~20KB output should be fine, I'll add a test for that.

@justinmk justinmk changed the title from [WIP] tui: throttle (drop) output to avoid flooding to [RFC] tui: throttle (drop) output to avoid flooding Oct 5, 2016

@justinmk justinmk changed the title from [RFC] tui: throttle (drop) output to avoid flooding to [RFC] throttle shell output to to maintain responsiveness Oct 5, 2016

@justinmk justinmk changed the title from [RFC] throttle shell output to to maintain responsiveness to [RFC] throttle shell output to maintain responsiveness Oct 5, 2016

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 7d8f027 to 6daebff Oct 5, 2016

@@ -14,8 +15,7 @@
// +----------------+
// | Main loop |
// +----------------+
// ^
// |
//
// +----------------+
// +-------------->| Event loop |<------------+
// | +--+-------------+ |

This comment has been minimized.

@justinmk

justinmk Oct 5, 2016

Member

removed the connection above because... event-loop cannot have a parent. So the diagram is confusing.

@marvim marvim added RFC and removed WIP labels Oct 5, 2016

@justinmk justinmk force-pushed the justinmk:tui-throttle branch 4 times, most recently from 436fa5e to 7246072 Oct 5, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 6, 2016

Rebased and add a test.

XXXXXXXXXX |
.. |
{10:Press ENTER or type command to continue}{1: } |
{3:-- TERMINAL --} |

This comment has been minimized.

@justinmk

justinmk Oct 6, 2016

Member

This test is a bit fragile, I'm not sure only two dots will appear on all platforms, it depends on the buffer size.

If this is too fragile, I may need to add a function to screen.lua that checks for a pattern without asserting the exact screen state.

Update: Added any parameter to screen:expect() to allow checking that a partial string appears in the screen state.

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 7246072 to e15d74f Oct 6, 2016

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.
screen:expect("\n.", nil, nil, nil, true)

This comment has been minimized.

@justinmk

justinmk Oct 6, 2016

Member

Passing any=true to screen:expect checks that the string appears "somewhere" in the screen state. The expected string "\n." means that throttling was triggered for the shell output.

-- condition: Function asserting some arbitrary condition.
-- any: true: Succeed if `expected` matches ANY screen line(s).
-- false (default): `expected` must match screen exactly.
function Screen:expect(expected, attr_ids, attr_ignore, condition, any)

This comment has been minimized.

@justinmk

justinmk Oct 6, 2016

Member

Added any parameter.

return (
'Row ' .. tostring(i) .. ' did not match.\n'
..'Expected:\n |'..table.concat(msg_expected_rows, '|\n |')..'|\n'
..'Actual:\n |'..table.concat(actual_rows, '|\n |')..'|\n\n'..[[

This comment has been minimized.

@justinmk

justinmk Oct 6, 2016

Member

Modified the Expected/Actual messages to indent the contents for readability.

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from e15d74f to cac4865 Oct 6, 2016

@justinmk justinmk force-pushed the justinmk:tui-throttle branch 2 times, most recently from 096a3d6 to a6befc9 Oct 24, 2016

@justinmk justinmk force-pushed the justinmk:tui-throttle branch 3 times, most recently from 5d7daec to 1a8baa3 Dec 8, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Dec 9, 2016

1a8baa3 addresses @oni-link's test case and is generally more flexible and lower-risk. Also re-tested this against these cases.

Going to merge this after CI. The helpers.lua enhancements are needed to fix the flaky global_spec.lua test.

@justinmk justinmk changed the title from [RFC] throttle shell output to maintain responsiveness to [RDY] throttle shell output to maintain responsiveness Dec 9, 2016

@marvim marvim added RDY and removed RFC labels Dec 9, 2016

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 1a8baa3 to 4ad2d81 Dec 9, 2016

justinmk added some commits Sep 30, 2016

os/shell: Throttle :! output, pulse "..." message.
Periodically skip :! spam. This is a "cheat" that works for all UIs and greatly
improves responsiveness when :! spams MB or GB of output:
    :!yes
    :!while true; do date; done
    :!git grep ''
    :grep -r '' *

After ~10KB of data is seen from a single :! invocation, output will be skipped
for ~1s and three dots "..." will pulse in the bottom-left. Thereafter the
behavior alternates at every:
    * 10KB received
    * ~1s throttled

This also avoids out-of-memory which could happen with large :! outputs.

Note: This commit does not change the behavior of execute(':!foo').
      execute(':!foo') returns the string ':!foo^M', it captures *only* Vim
      messages, *not* shell command output. Vim behaves the same way.
      Use system('foo') for capturing shell command output.

Closes #1234

Helped-by: oni-link <knil.ino@gmail.com>
tui: "backpressure": Drop messages to avoid flooding.
Closes #1234

multiqueue:
- Implement multiqueue_size()
- Rename MultiQueueItem.parent to MultiQueueItem.parent_item, to avoid confusion
  with MultiQueue.parent.

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 9228062 to 29f1262 Dec 9, 2016

justinmk added some commits Oct 7, 2016

os/shell: do_os_system(): Always show last chunk.
This ameliorates use-cases like:
    :!cat foo.txt
    :make
where the user is interested in the last few lines of output.

Try these shell-based ex-commands before/after this commit:
    :grep -r '' *
    :make
    :!yes
    :!grep -r '' *
    :!git grep ''
    :!cat foo
    :!echo foo
    :!while true; do date; done
    :!for i in `seq 1 20000`; do echo XXXXXXXXXX $i; done

In all cases the last few lines of the command should always be shown,
regardless of where throttling was triggered.
out_data_decide_throttle(): timeout instead of hard limit.
Instead of managing max_visits, check the time every N visits. This avoids edge
cases where max_visits is large but the chunk frequency slowed down, which would
causing the "..." pulse to show for a very long time. It's more important to
show output at reasonable intervals than to avoid calling os_hrtime().

@justinmk justinmk force-pushed the justinmk:tui-throttle branch from 29f1262 to 4abe9af Dec 10, 2016

@justinmk justinmk merged commit 7c513d6 into neovim:master Dec 10, 2016

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
QuickBuild Build pr-5396 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls First build on master at 71.523%
Details

@justinmk justinmk deleted the justinmk:tui-throttle branch Dec 10, 2016

@justinmk justinmk removed the RDY label Dec 10, 2016

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas, packages.

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

@justinmk justinmk referenced this pull request May 1, 2017

Merged

NVIM v0.2.0 #6634

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017

NVIM v0.2.0
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment