Add back pressure to pty #536
Conversation
There was a problem hiding this comment.
@kxt This looks great ❤️! There is a significant improvement in performance in release build and I didn't notice any lags in the debug build. Although, the render/cat-ing time has increased (in debug), but I think that's what we should be expecting with the bounded queue.
One thing I noticed is that a couple of tests are still failing intermittently. Maybe because of the changes to the FakeInputOutput. Instead of adding sleep to specific tests, does increasing
Line 25 in 0c0355d
to let's say 200ms or more help? Or do the tests still fail?
Nevertheless, I've asked @imsnif to take a look at the changes to FakeInputOutput more closely.
All other changes LGTM. And we can go ahead and merge once the tests pass consistently.
Pty reads a command's output and feeds it to Screen using an unbounded queue. However, if the command produces output faster than what `Screen` can render, `Pty` still pushes it on the queue, causing it to grow indefinitely, resulting in high memory usage and latency. This patch fixes this by using a bounded queue between Pty and Screen, so if Screen can't keep up with Pty, the queue will fill up, exerting back pressure on Pty, making it read the command's output only as fast as Screen renders it. The unbounded queue is kept between Screen and producers other than Pty to avoid a deadlock such as this scenario: * pty thread filling up screen queue as soon as screen thread pops something from it * wasm thread is processing a Render instruction, blocking on the screen queue * screen thread is trying to render a plugin pane. It attempts to send a Render insturction to the blocked wasm thread running the plugin. This patch also adds a generous amount of sleeps to the integration tests as having backpressure changes the timing of how instructions are processed. Fixes zellij-org#525.
|
Well, those With the The indeterminacy of the threaded architecture makes testing quite tricky. Regarding the performance That being said, this patch does have some actual performance penalty, indirectly caused by the bounded queue: previously, the queue was not blocking, so it was ok to call it in an async block. Now it being bounded, it can block, hence the use of |
|
@kunalmohan can you provide a failing test output? |
---- tests::integration::compatibility::top_and_quit stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: src/tests/integration/snapshots/zellij__tests__integration__compatibility__top_and_quit.snap
Snapshot: top_and_quit
Source: src/tests/integration/compatibility.rs:625
-old snapshot
+new results
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
snapshot_before_quit
────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
0 │-Tasks: 158 total, 1 running, 157 sleeping, 0 stopped, 0 zombie
1 │-%Cpu(s): 24.2 us, 1.6 sy, 0.0 ni, 74.2 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st
2 │-MiB Mem : 15715.1 total, 7001.4 free, 2163.7 used, 6549.9 buff/cache
3 │-MiB Swap: 16384.0 total, 16384.0 free, 0.0 used. 12809.8 avail Mem
0 │+█
1 │+
2 │+
3 │+
4 │+
5 │+
6 │+
7 │+
8 │+
9 │+
10 │+
11 │+
12 │+
13 │+
14 │+
15 │+
16 │+
17 │+
18 │+
19 │+
20 │+
21 │+
22 │+
23 │+
24 │+
25 │+
26 │+
27 │+
28 │+
29 │+
30 │+
31 │+
32 │+
33 │+
34 │+
35 │+
36 │+
37 │+
38 │+
39 │+
40 │+
41 │+
42 │+
43 │+
44 │+
45 │+
46 │+
47 │+
48 │+
49 │+
50 │+
51 │+
52 │+
53 │+
4 54 │
5 │- PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
6 │- 15838 aram 20 0 7275240 56912 18964 S 73.3 0.4 0:17.79 zellij
7 │- 12653 aram 20 0 39032 22164 14372 S 20.0 0.1 0:00.63 urxvt
8 │- 1477 aram 20 0 3178660 301992 203236 S 6.7 1.9 3:49.82 Web Content
9 │- 1 root 20 0 175360 11532 8596 S 0.0 0.1 0:05.90 systemd
10 │- 2 root 20 0 0 0 0 S 0.0 0.0 0:00.01 kthreadd
11 │- 3 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 rcu_gp
12 │- 4 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 rcu_par_gp
13 │- 6 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kworker/0:0H-kblockd
14 │- 8 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 mm_percpu_wq
15 │- 9 root 20 0 0 0 0 S 0.0 0.0 0:01.24 ksoftirqd/0
16 │- 10 root -2 0 0 0 0 S 0.0 0.0 0:00.00 rcuc/0
17 │- 11 root -2 0 0 0 0 I 0.0 0.0 0:06.12 rcu_preempt
18 │- 12 root -2 0 0 0 0 S 0.0 0.0 0:00.00 rcub/0
19 │- 13 root rt 0 0 0 0 S 0.0 0.0 0:00.01 migration/0
20 │- 14 root -51 0 0 0 0 S 0.0 0.0 0:00.00 idle_inject/0
21 │- 16 root 20 0 0 0 0 S 0.0 0.0 0:00.00 cpuhp/0
22 │- 17 root 20 0 0 0 0 S 0.0 0.0 0:00.00 cpuhp/1
23 │- 18 root -51 0 0 0 0 S 0.0 0.0 0:00.00 idle_inject/1
24 │- 19 root rt 0 0 0 0 S 0.0 0.0 0:00.35 migration/1
25 │- 20 root -2 0 0 0 0 S 0.0 0.0 0:00.00 rcuc/1
26 │- 21 root 20 0 0 0 0 S 0.0 0.0 0:00.49 ksoftirqd/1
27 │- 23 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kworker/1:0H-kblockd
28 │- 24 root 20 0 0 0 0 S 0.0 0.0 0:00.00 cpuhp/2
29 │- 25 root -51 0 0 0 0 S 0.0 0.0 0:00.00 idle_inject/2
30 │- 26 root rt 0 0 0 0 S 0.0 0.0 0:00.39 migration/2
31 │- 27 root -2 0 0 0 0 S 0.0 0.0 0:00.00 rcuc/2
32 │- 28 root 20 0 0 0 0 S 0.0 0.0 0:00.99 ksoftirqd/2
33 │- 30 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kworker/2:0H-kblockd
34 │- 31 root 20 0 0 0 0 S 0.0 0.0 0:00.00 cpuhp/3
35 │- 32 root -51 0 0 0 0 S 0.0 0.0 0:00.00 idle_inject/3
36 │- 33 root rt 0 0 0 0 S 0.0 0.0 0:00.43 migration/3
37 │- 34 root -2 0 0 0 0 S 0.0 0.0 0:00.00 rcuc/3
38 │- 35 root 20 0 0 0 0 S 0.0 0.0 0:00.90 ksoftirqd/3
39 │- 37 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kworker/3:0H-kblockd
40 │- 38 root 20 0 0 0 0 S 0.0 0.0 0:00.00 kdevtmpfs
41 │- 39 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 netns
42 │- 40 root 20 0 0 0 0 S 0.0 0.0 0:00.00 rcu_tasks_kthre
43 │- 41 root 20 0 0 0 0 S 0.0 0.0 0:00.00 kauditd
44 │- 44 root 20 0 0 0 0 S 0.0 0.0 0:00.00 khungtaskd
45 │- 45 root 20 0 0 0 0 S 0.0 0.0 0:00.00 oom_reaper
46 │- 46 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 writeback
47 │- 47 root 20 0 0 0 0 S 0.0 0.0 0:00.00 kcompactd0
48 │- 48 root 25 5 0 0 0 S 0.0 0.0 0:00.00 ksmd
49 │- 49 root 39 19 0 0 0 S 0.0 0.0 0:00.00 khugepaged
50 │- 137 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kintegrityd
51 │- 138 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 kblockd
52 │- 139 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 blkcg_punt_bio
53 │- 140 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 ata_sff
54 │- 141 root 0 -20 0 0 0 I 0.0 0.0 0:00.00 edac-poller
55 │-⋊> ~/c/zellij on fix-top ⨯ █ 13:00:53
55 │+
────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review` |
|
@kunalmohan - for me the tests pass consistently. Do they fail very often for you? Any chance this also happens for you in main? Also, I don't see any changes to the Other than that, after trying this out: WOW, this feels great! I can't wait to see this merged. |
|
The test seems to be failing on the first run for me. They pass on the second run. And this isn't the case with main branch. And yes the changes to |
|
@kunalmohan - does the test pass if you add another sleep to it before the end? |
|
@imsnif yes, the test passes in that case. |
|
Let's add that sleep then for now, because I imagine you will not be the only one. |
This PR fixes #525 by adding back pressure if
Ptyis producing renderable output faster thanScreencan render it . This is achieved by changing the unbounded queue to a bounded one between them.