Repository navigation
perf(cmux-tui): a key to an idle input port is written on its own turn, with no thread hand-off (#35413 A/B) - #35872
Conversation
…ocket when its request returns (#35413 A/B; red) The s17fa A/B on build 5cd1899b (STATUS 12857, 12866): with the input lane on, the daemon part of a key was +0.025 / +0.17 / +0.53 ms at p50 / p95 / p99 over the lane off, all of it in daemon.send.written -> host.pty.write. The request thread puts the key on the terminal's input port and returns; the port's writer thread then wakes to write the host socket, and under the L6 CPU load the wake is the extra time. With the lane off the request thread writes the socket itself. The test holds the port's writer before it writes anything, sends a key, and reads the host socket: the key must already be there.
… own turn, with no thread hand-off (#35413 A/B) Red at dd67a78. The s17fa A/B on build 5cd1899b (STATUS 12857, 12866): with the S1 input lane on, the daemon part of a key was +0.025 / +0.17 / +0.53 ms at p50 / p95 / p99 over the lane off, all of it between daemon.send.written and the host's PTY write (OFF 0.016-0.019 / 0.14-0.17 / 0.38-0.48 ms, ON 0.038-0.046 / 0.27-0.32 / 0.67-0.99 ms, in every round, also in s17d). The request thread put each key on the terminal's input port; the port's writer thread then woke to write the host socket, and under the L6 CPU load that wake was the extra time. With the lane off the request thread wrote the socket itself. Now a key (no waiter, at most 1 KB) to an idle port (nothing queued or in flight, no reconnect wait) is written on the request's own turn, under the port lock, when HostInputRoute::try_send_now can write it at once: try_lock on the connection's writer and a non-blocking send (MSG_DONTWAIT, and MSG_NOSIGNAL off macOS) that a Unix stream socket takes whole or not at all for a frame this small (a partial write would shut the connection down, as a failed write does). A busy writer, a full socket, an error or a frame ahead leaves the key to the port as before, so a key still never waits for another writer or a host that stopped reading (S1.6). The key counts as one of the port's frames, taken and finished at once, so the order barrier and the viewer-size queue see the same state as before. Tests: the port's writer tests that hold a key at a paused writer opt out of the inline write (queue_every_write_for_test), so they still exercise the writer's order, reconnect and QoS.
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41930990041 at a7fac9e failed: cmux-next crash ends the app. |
…ing and keep the order (#35872 review) Review of ef166da (SHIP), should-fixes and notes: - The red test moves to surface/tests/input_port_inline.rs (hosted.rs was at its 1500-line cap and the new test had taken the doc of the next one); the order test gets its doc back, saying an idle port now writes inline. - Fallback tests, each deterministic: - another writer holds the connection: the key returns at once, goes to the port, and reaches the host once when the writer is free; - the host socket is full (filled with whole frames, the host reads nothing): the key returns at once and reaches the host after every frame written before it; - a key written inline, a script write the port's writer holds, a key queued behind it, then a key after the port is idle: the host reads them in that order, once each. - try_send_now recovers a poisoned connection writer, as send_host_frame does, instead of sending every later key through the port. - MSG_NOSIGNAL only on Linux and Android, the gate pty_custody uses; Apple targets rely on std's SO_NOSIGPIPE and the ignored SIGPIPE, as every other write on this socket. - InputPort::enqueue's doc says when the key is written inline.
…version (clippy unused_qualifications)
… requests (#35872; red on macOS) The full-socket test (b1e8db9) timed out on macOS (step 8f40081f419d7d78b217103e, 120 s) and passed on Linux. Its fill loop sent whole frames with MSG_DONTWAIT until EAGAIN; on macOS such a send on a blocking stream socket waits for room instead (XNU honours only the socket's own non-blocking mode there), so the loop blocked. The inline write's send in try_send_now is the same call: on macOS a key whose inline send meets a full host socket waits until the host reads, holding its request, the connection's writer and the port lock, which is the wait S1.6 removed. The test types 1000 keys into a host that reads nothing for 10 s: every request must return, the slowest under 2 s; then the host reads and must get every key once, in order. On macOS without a fix the inline send blocks after a few hundred keys (the socket's room runs below one frame); on Linux it passes (MSG_DONTWAIT is honoured).
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
…efore it sends (#35872 review: macOS) Red at 7293b1b (keys_to_a_host_that_stopped_reading_never_block_their_requests, macOS). Re-review NO-SHIP at ecc1116, must-fixes 1 and 2: - macOS: XNU's sosendcheck returns EWOULDBLOCK only for a non-blocking socket (or a kernel-only flag); with MSG_DONTWAIT on a blocking socket it sleeps in sbwait until there is room. The daemon's host socket is blocking (its reader shares it) with a 2 s send timeout, so a key whose inline send met a full socket waited up to 2 s holding its request, the connection's writer and the port lock (other keys, barrier callers, viewer sizes and route changes waited behind it). try_send_now now polls for POLLOUT with a zero timeout under the writer guard and sends only when it is set: on XNU POLLOUT for a connected local socket means at least the 2048-byte send low-water mark is free, more than any inline frame (a const assert pins INLINE_FRAME_MAX + HEADER_LEN < 2048), and the guard keeps every other daemon writer off the socket until the send. On Linux MSG_DONTWAIT is honoured as before; POLLOUT there only sends keys to the port earlier, once a quarter of the send buffer is used. - The full-socket test is replaced (7293b1b) by the stalled-host test: its fill loop's MSG_DONTWAIT sends hung on macOS. - should-fix 3: the order test waits for an idle port (no frame queued or in flight, new idle_for_test) before "3" and holds the writer again, so a "3" that went to the writer instead of inline is missing from the read. - note 4: the busy-writer test sends its key from a helper thread with a 1 s bound, so a key that waited for the held writer fails with a message instead of deadlocking the test.
…when the socket is full (#35872 review) Re-review SHIP at 4cc7e1b, should-fix 2: the comment "a zero timeout: poll does not wait" is false on XNU when the socket has no room. There poll_nocancel sets a deadline of now without KEVENT_FLAG_IMMEDIATE, so kqueue_scan blocks on a timer that is already due (set 10 us ahead), and the thread needs one wake. That happens only on the no-room path, holding the request's turn, the connection writer and the port lock; it never waits for room or for the host. XNU's poll also makes and frees a kqueue per call. has_room_now's doc now says this, and the module doc names the room check instead of "a non-blocking send". Comments only; no code changes.
|
Review (ae25c6d154f94c01a-inline-reviewer): SHIP at 89075c0 Independent review subagent. It read the code, the XNU and Linux kernel sources and the fleet job logs; it ran no build.
|
…fix/p0-typing-s1-default-on
…line key write #35872) into fix/p0-typing-s1-8-delete-old-path S1.7 landed on feat as one squash, so feat holds the S1.7 branch's content but not its commits, and the merge base git picks (0f4febb) is older than every S1 change. With that base, each line S1.8 set back to its pre-S1 form looks unchanged on this side, and the merge silently put the S1.7 version back (for example old_path_mux in server/tests/input_lane.rs). So this merge is made against 6bfdf88, the S1.7 head this branch already merged (git merge-tree --merge-base): this side brings only S1.8's own changes, feat brings everything after that S1.7 head. Checked after the merge: no deleted name is back (the switch, the kept terminal, the route generations, the publishing and lock-free flags), and git diff against feat touches the same 34 S1.8 files plus the two below. Resolved: - rds_switch.rs: deleted (feat's copy is the S1.7 branch's). - DaemonDiagnosticKnobs.swift: feat moved the lane knob to inputLaneDaemonName and passed it to the Chief owner (chiefOwnerDaemonNames). Both lane parts are deleted; chiefOwnerDaemonNames keeps the R-LIFECYCLE keepers switch only. DaemonLauncher.chiefEnvironment's doc no longer names the lane switch, and theInputLaneSwitchReachesTheChiefOwner (it tested that knob) is deleted. - surface/tests/hosted.rs: feat made hosted_typing_surface and wait_until pub(super) for input_port_inline.rs and opts the held-writer tests out of the inline write; kept with S1.8's hosted_typing_surface(name) (no lane argument). - surface/tests/input_port_inline.rs (new on feat): its four hosted_typing_surface calls drop the lane argument. - surface/input_port.rs and terminal_host_runtime/shared/attachment.rs: the inline write and its room check stay exactly as on feat (attachment.rs has no diff against feat; input_port.rs differs only in S1.8's doc lines).
…nput.rs (hosted.rs past its 1500-line cap after the feat merge) The feat merge (#35872's inline-write opt-outs) took surface/tests/hosted.rs to 1507 lines, past the 1500-line cap of a Rust test file (scripts/cmux-next/check-no-godfiles.sh). The test S1.8 added there, input_to_a_hosted_terminal_with_no_input_route_fails_and_does_not_panic, is an input delivery test, so it moves unchanged to surface/tests/input.rs, the input delivery tests. It uses hosted.rs's hosted_typing_surface and frames_until_quiet, now pub(super) (hosted_typing_surface already was, for input_port_inline.rs). hosted.rs is 1459 lines.
…e per connection and sends without a copy (#35413 s17ga) (#35988) * test(cmux-tui): a connection's keys share one kernel queue for their room check (#35413 s17ga; red on macOS) The s17ga typing A/B for #35413 (STATUS 13245, 13254) kept the S1 input lane off: daemon part p50 was +0.016 ms with ON, against an OFF round-to-round spread of 0.009 ms, with both ON rounds above both OFF rounds. The whole difference was the key's write call (daemon.send.write -> written): 0.019 and 0.020 ms at p50 with ON, 0.005 ms with OFF. The inline key write (#35872) checks for room with poll(POLLOUT, 0) before its send. On XNU each poll() opens a kernel event queue, registers a knote on the socket under the socket's lock, and frees both again (sys_generic.c poll_nocancel; the #35872 reviewer's note 3). The test sends 100 keys to one idle connection and counts the kernel event queues that their room checks opened, with a test-only per-thread counter at the room check. It fails on this code: 100 keys open 100 queues, one per key. macOS only (Linux's poll opens no queue). * fix(cmux-tui): the inline key write checks room with one kernel queue per connection and sends without a copy (#35413 s17ga) The test is the commit before this one (red on macOS: 100 keys to one connection opened 100 kernel event queues, one per key's room check). s17ga (STATUS 13245, 13254, 13302) kept the S1 input lane off. On one boundary for both arms, ON was worse at p50 beyond the OFF spread. The whole difference was the write call on the key's own request thread: 0.019-0.020 ms with the inline write, against 0.005 ms for the old path's direct write. The inline write did three things the old path did not: 1. poll(POLLOUT, 0) before the send. On XNU each poll() opens a kernel event queue, registers a knote under the socket lock and frees both. 2. Two heap copies of the key (Frame::new(payload.to_vec()), encode_frame). 3. One send of a frame it had just built. Now (terminal_host_runtime/shared/attachment/room_check.rs): - macOS: each host connection keeps one kernel event queue, with the socket's EVFILT_WRITE registered once at its first key. Each key reads it with kevent64(KEVENT_FLAG_IMMEDIATE): one call, no allocation, no sleep. The filter's data is the socket's free room; the key goes inline only when it holds the whole frame. EV_EOF or EV_ERROR is no room, so the key goes to the port and its reconnect path. A failed kqueue() falls back to poll(). The connection's attachment owns the queue and shares it with every route clone; it closes with the attachment, and a reconnect makes a new one. - Linux: no check. MSG_DONTWAIT is honoured, and a Unix stream socket takes a frame this small whole or not at all. - Every Unix target: the frame header is built on the stack (terminal_host_protocol::input_frame_header, pinned to encode_frame's output by the_inline_frame_header_is_the_encoders_header). The header and the key go out in one sendmsg of two iovecs, with no heap copy of the key. The order rules, the size limit and the fallbacks to the port are the same. The room check still runs under the connection's writer guard, so nothing takes the room before the send. * test(cmux-tui): after a full host socket drains, the next key goes inline again; the header test covers Paste (#35988 review) Review of #35988 (SHIP at 0fffc79), should-fix 1: no test sent a key after the socket was full and the host then read everything. On macOS that is the one state the per-connection room-check queue keeps between keys: if its write filter did not report room again, every later key on the connection would go to the port and nothing would say so. keys_to_a_host_that_stopped_reading_never_block_their_requests now ends by waiting for an idle port, holding the port's writer and sending one more key: it must reach the host inline. The order test uses the same pattern. Note 3: the_inline_frame_header_is_the_encoders_header also covers MessageKind::Paste, which reaches the inline write too.
Summary
With the S1 input lane on, each key the person types went through one extra thread hand-off: the request thread put the key on the terminal's input port and returned, and the port's own writer thread then woke up to write it to the terminal host's socket. Under the L6 typing load that wake-up made the daemon's part of every key slower than with the lane off: +0.025 / +0.17 / +0.53 ms at p50 / p95 / p99 over 1760 keys per arm in the s17fa A/B for #35413 (build 5cd1899b; STATUS 12857, 12866). All of the difference was in one segment, from the key's handoff (
daemon.send.written) to the host's PTY write: OFF 0.016-0.019 / 0.14-0.17 / 0.38-0.48 ms, ON 0.038-0.046 / 0.27-0.28 / 0.67-0.77 ms, in every round (STATUS 12890). With the lane off, the request thread writes the socket itself.Now a person's key (no waiter, at most 1 KB) to an idle input port (nothing queued or in flight, no reconnect wait) is written on the request's own turn, under the port lock, when
HostInputRoute::try_send_nowcan write it at once:try_lockon the connection's writer. A writer that another thread holds sends the key to the port.poll(POLLOUT)with a zero timeout on the host socket. No room sends the key to the port. This check is needed on macOS: there asendwithMSG_DONTWAITon a blocking stream socket still sleeps until the socket has room (XNU honours only the socket's own non-blocking mode), and this socket must stay blocking because its reader shares it. Without the check, a key to a host that stopped reading waited up to the socket's 2 s send timeout, holding its request, the connection's writer and the port lock. On XNU, POLLOUT means at least the 2048-byte send low-water mark is free; a const assert keepsINLINE_FRAME_MAXplus the frame header under that. The writer guard keeps every other daemon writer off the socket until the send, so the room cannot shrink in between. On Linux, POLLOUT only sends keys to the port earlier (once a quarter of the send buffer is used).sendwithMSG_DONTWAIT(plusMSG_NOSIGNALon Linux and Android). With room for the whole frame, a Unix stream socket takes it whole. If a partial write ever happened, the connection is shut down, as a failed write already does, and the port's writer sends the key again on the reconnected host.A busy writer, a full socket, an error or a frame ahead leaves the key to the port as before, so S1.6's rule holds: a key never waits for another writer or for a host that stopped reading.
Order: the check and the write are under the port lock and only when the port is idle, so no frame the port took can be passed, and frames taken after the key queue behind it. The key counts as one of the port's frames, taken and finished at once, so the order barrier and the viewer-size queue (
PortOrder) see the same state as before.Waits under the port lock:
try_lockand a send with room never wait, and the zero-timeoutpollnever waits for room or for the host. On Linux it returns at once. On macOS it returns at once when there is room; with no room, XNU's poll sleeps on its timer, which is already due (about 10 us, then one wake), before it reports none, and the key goes to the port. XNU's poll also makes and frees a kqueue on each call, a few us per key. The s17g A/B runs on a Mac (cmux21): it measures the poll on every key, and the no-room sleep only if a host falls behind during the run. If the no-room sleep shows in the slowest keys, a per-connection kqueue checked withkevent64(..., KEVENT_FLAG_IMMEDIATE)removes it.Testing
Focused command:
cmux-ci run --class isolated --script scripts/ci/cmux-tui-rust-check.sh --os linux|macos --arg test --arg FILTER --ref SHA. The head 89075c0 changes only comments after 4cc7e1b (the poll doc above), so the runs are at 4cc7e1b unless named. Red first:a_key_to_an_idle_port_is_on_the_host_socket_when_its_request_returns(holds the port's writer before it writes anything, sends a key, reads the host socket): fails at dd67a78 (tests only, on feat 0d30cf0) on Linux, step 05a6c8fc60135ee8ffa23f55 ("the key was not on the host socket when its request returned"); passes at 4cc7e1b.keys_to_a_host_that_stopped_reading_never_block_their_requests(types 1000 keys into a host that reads nothing for 10 s; every request must return, the slowest under 2 s; then the host must get every key once, in order): fails on macOS at 7293b1b (the test, before the room check), step f20ed39d22fb64ecea06cbc3 ("a key's request waited for a host that stopped reading: 1000 keys did not return in 10 s"); passes at 4cc7e1b. Without the fix, with the default 8 KB local stream buffer, the inline send sleeps at about the 228th key until the host reads. The first version of this test (b1e8db9) filled the socket withMSG_DONTWAITsends and hung for 120 s on macOS (step 8f40081f419d7d78b217103e), which is how the macOS behaviour came out in review.input_port_inlineat 4cc7e1b: 4 of 4 pass on Linux (step a4cd0e1678f9675112ee9794) and on macOS (step 7d282d8ea46f8260b4b4df5c). The other two tests: a key while another writer holds the connection goes to the port and does not wait for it (sent from a helper thread with a 1 s bound); keys keep their order between the inline write and the port's writer (the test waits for an idle port and holds the writer again before the last key).surface::tests::hostedat 4cc7e1b: macOS 40 of 40 pass (step 12f6bc3358744ae7c91bb478, with the macOS-only writer QoS test; the idle-port test moved toinput_port_inline). At ef166da: Linux 40 of 40 (step 619f8f3828391b33024b79ae), macOS 41 of 41 (step 4e1bd96e190692787b21cec2). The port's writer tests that hold a key at a paused writer opt out of the inline write (queue_every_write_for_test), so they still exercise the writer's order, reconnect and QoS.server::tests::rds_input_lane11,server::tests::input_lane9, one app projection test).peer_integration,peer_request_deadlines,session_pool; acpmux does not depend on cmux-tui-core) and the 3server::agent_transfer::teststhat fail alone on feat. 3 failed in the full run and passed alone: an acpmux auto-start test, thelink_ensurestop test (a link that exited but was not yet reaped under load; it failed the same way once in a full run of fix(cmux-tui): terminal output never waits for a durable publication (#35413 echo stall) #35765, which has none of this change) and the knownlayout_ops_conserve_tabsload flake. Every input-port and input-lane test passed:surface::tests::hosted39 (its QoS test is macOS-only),input_port_inline4,server::tests::rds_input_lane11,server::tests::input_lane7. At ef166da (step 47991676362511aa57c988a3): 17051 tests, 9 failed; 6 also fail alone and are base failures (3 acpmux, which does not depend on cmux-tui-core, and the 3server::agent_transfer::teststhat fail alone on feat), 3 failed in the full run and passed alone (the knowna_persons_requests_answer_while_busy_sessions_saturate_the_registryload flake, an agent-session-share room test and an acpmux token test).Observed / Not run
Performance
Changelog
none (the S1 input lane is off by default until #35413; this changes only the lane-on path)
Proof
No capture: daemon-only change with no UI. The typing A/B with movies on the fixed build is reported on #35413.