Make loops cancellable: poll interrupts on back edges, and add MP:WITH-TIMEOUT - #1843
Open
dg1sbg wants to merge 6 commits into
Open
Make loops cancellable: poll interrupts on back edges, and add MP:WITH-TIMEOUT#1843dg1sbg wants to merge 6 commits into
dg1sbg wants to merge 6 commits into
Conversation
MP:PROCESS-KILL cannot stop a loop whose body compiles to pure opcodes. The VM polls queued interrupts only on function entry, in bytecode_call, so a loop that calls nothing never reaches a safepoint and no interrupt is ever delivered to it. (LOOP), (LOOP UNTIL *FLAG*), (LOOP FOR I FROM 0), (LOOP (SETQ X (1+ X))) and (DO ((I 0 (1+ I))) (NIL)) all survive PROCESS-KILL and run until the image exits. A loop containing any call, and a thread blocked in SLEEP, were always killable; this is specifically the call-free case. That defect blocks any deadline or timeout facility built on cancellation, since the one workload a timeout most needs to interrupt -- a spinning thread -- is the one that cannot be interrupted. Note that the return value never indicated this. MP:PROCESS-KILL and MP:PROCESS-CANCEL are both (MP:INTERRUPT process 'MP:CANCELLATION-INTERRUPT) and the C++ entry point is void, so they answer NIL whether or not the target dies. MP:PROCESS-ACTIVE-P is the only way to tell. This polls on backward branches, which is where a loop must pass regardless of its body. All eight jump sites route through one helper rather than repeating the test; a forward branch pays only a predictable rel < 0 comparison. Two details matter for correctness and cost: The handler can cons or unwind, so vm._pc and vm._stackPointer are synced before polling -- the GC scans the bytecode stack up to _stackPointer. The fast path tests the pending-interrupt state through the ThreadLocalState pointer the VM already caches per frame, not through my_thread. On Darwin every my_thread access is a _tlv_get_addr thunk, which per loop iteration would cost far more than the poll itself. It also cannot use interrupt_queue_validp(): the queue keeps a dummy head node so it is never empty, and that predicate is true whenever the queue exists. The real test is that the head's cdr is non-nil. Cost, measured against the same source revision without this patch, alternating runs, on a loop whose entire body is one opcode: about 1-2%, and in one of three rounds the patched build was faster. Regression suite 2014 -> 2017, the three new tests, no other change.
The bytecode VM now polls interrupts on backward branches, but native code reached a safepoint only at function entry, in the XEP before the tail call. A natively compiled loop whose body calls nothing therefore still could not be cancelled: PROCESS-KILL on (COMPILE NIL '(LAMBDA () (LOOP))) had no effect, with the function confirmed to be a SIMPLE-CORE-FUN rather than bytecode. BIR has no loop or dominance analysis, but its iblocks are laid out in forward flow order, so a branch to an already-laid-out iblock is a back edge -- the same test the VM makes with a negative jump offset. TRANSLATE-TERMINATOR's :AROUND method is the one place covering JUMP, IFI and CASE. Polling every back edge is not affordable. cc_safepoint is an out-of-line call declared PRIMITIVE-UNWINDS, so LLVM must treat it as possibly-throwing and cannot hoist loop-invariant work or keep values in registers across it; on a loop whose body is a single increment that costs about 3x. It cannot be declared non-throwing, since the handler really can unwind, and cleavir/primitives.lisp warns against exactly that change. So this samples. A counter in the function's alloca block is incremented at each back edge and the call is made once every 256, leaving cancellation latency at most 255 iterations -- submicrosecond -- for an add, a mask and a predictable branch per iteration. Measured over nine alternating runs against the same source revision without this patch: about 8% on that worst case, down from 3x. Loops doing more per iteration pay proportionally less, and loops containing a call were always cancellable. The residual 8% is the inline counter rather than the call, so raising the sample interval will not help much. Two options not taken: read a global pending-interrupt flag instead of counting, which requires an atomic load so LLVM cannot hoist it out of the loop; or instrument only call-free loops, which would make almost all real code pay nothing. The new test is vacuous where no native compiler exists; it asserts SIMPLE-CORE-FUN so it cannot pass by silently testing bytecode. Regression suite 2017 -> 2018, the one new test.
Clasp had no way to bound the running time of a form. The only timeout primitive in MP was CONDITION-VARIABLE-TIMEDWAIT, which bounds one wait rather than an arbitrary body, so every timeout-dependent test had to be written without one. WITH-TIMEOUT runs BODY and signals TIMEOUT in the calling process if it has not finished within SECONDS. The deadline is delivered as an interrupt, so it lands at the next safepoint and inherits exactly the coverage interrupts have. That is why this could not usefully exist until loops polled on back edges: the workload a timeout most needs to interrupt is a spinning thread, and that was the one workload no interrupt could reach. The watchdog waits on a condition variable rather than sleeping, so a body that finishes early wakes it immediately instead of stranding a thread for the full duration; (WITH-TIMEOUT (3600) ...) must not hold a thread for an hour. The wait is a deadline loop because CONDITION-VARIABLE-TIMEDWAIT may return early, and a single unguarded wait would produce spurious timeouts. Both threads touch the completion flag only under the lock. TIMEOUT is a subtype of ERROR rather than SERIOUS-CONDITION, where SBCL and bordeaux-threads put theirs. A condition that slips through HANDLER-CASE on ERROR and IGNORE-ERRORS is a footgun, and a timeout does mean the operation failed. This is a deliberate deviation. Regression suite 2018 -> 2021: the body completes, the body times out, and the timeout does not fire late after the body has already returned.
… its body
MP:WITH-TIMEOUT could not interrupt a body blocked in a foreign call. The
deadline expired, the interrupt was queued, and nothing happened until the call
returned on its own -- after which the interrupt was delivered with the caller's
handler long gone, so TIMEOUT escaped to the debugger instead of unwinding the
body.
The wake-up machinery already existed and was correct. Process_O::interrupt does
pthread_kill(SIGCONT) and handle_SIGCONT exists precisely to interrupt a thread
blocked on a syscall. Both gate on ThreadLocalState::blockingp(), which is set by
BEGIN_PARK -- and ext__system called system() without it, so the thread was never
marked blocking and neither half engaged.
Four Lisp-callable entry points blocked without parking: ext__system, core__wait,
core__select, and the two wait() calls in the fork/exec paths. Each is also a
garbage collection defect independent of interrupts, since a thread blocking
outside a GC-safe region stalls stop-the-world for its whole duration --
(core:wait) with no child exiting stalls it indefinitely. clasp_musleep and
ConditionVariable::wait were already correct and are the model.
Measured on a five second system("sleep 5"): a 0.3s deadline now returns after
0.318s and a 1.0s deadline after 1.001s, where both previously took the full five
seconds.
Separately, CALL-WITH-TIMEOUT had a race that parking narrows but does not close:
DONEP stopped the watchdog from sending a late interrupt, but nothing discarded
one already sent. The interrupt thunk now re-checks DONEP at delivery, and if the
deadline passed without the interrupt landing in time, the expiry is signalled on
return -- inside the caller's dynamic extent, where handlers still exist.
Regression suite 2021 -> 2024. WITH-TIMEOUT-FOREIGN-IS-PROMPT is the test that
distinguishes a prompt wakeup from a late report; the earlier tests passed either
way, which is how this hid.
Three more Lisp-callable entry points blocked without going GC-safe, so a thread in any of them could neither be woken by an interrupt nor let stop-the-world proceed. SERVE-EVENT-INTERNAL::LL-SERVE-EVENT-NO-TIMEOUT passes a NULL timeout to select, so it blocks indefinitely; the with-timeout variant blocks for as long as the caller asks. Both operate on fd_sets held in ForeignData, i.e. malloc'd memory, so parking around them raises no question about object lifetime. CORE:READ-FD blocks in read(2) until data arrives. It writes through a raw pointer into a SimpleBaseString, which is only safe to hold across a park because no collector here relocates -- NON_MOVING_GC in memoryManagement.h. If a moving plan is ever adopted this park needs the buffer pinned. The new test cancels a thread blocked reading an empty pipe, built from CORE:PIPE so it adds no external dependency. It is deliberately bounded rather than waiting on the read to return: an unbounded form would hang the suite instead of failing it, which is how a six hour CI timeout happens. Verified as a real discriminator rather than assumed: the same form answers NO on a build without these parks and YES with them. Regression suite 2024 -> 2025.
Five blocking entry points ran outside a GC-safe region: accept and connect for inet and local sockets, and the select in ll-socket-receive-timeout, which passes NULL when given no timeout and so blocks indefinitely. Sockets block by default here -- non-blocking mode is opt-in -- so socket flags do not save them. Each was both an interruptibility bug and a garbage collection bug: a thread in accept could not be cancelled, and stalled stop-the-world for as long as no connection arrived, which for a server socket is unbounded. All five act on stack-local sockaddr structs, or a ForeignData pointer for the select, so unlike CORE:READ-FD no pointer into the Lisp heap is held across the park. The test blocks a thread in accept on a local socket and cancels it, needing neither a port nor the network. It answered NO before these parks and YES after. CANCELLED-WITHIN-P now requires the process to still be running when it is killed. A thunk that dies on its own was previously indistinguishable from one that was cancelled, so a broken test could pass vacuously -- which one of mine briefly did. Regression suite 2025 -> 2026.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MP:PROCESS-KILLcannot stop a loop whose body compiles to pure opcodes, in either engine. Interrupts are delivered only at function-entry safepoints —gctools::handle_all_queued_interrupts()inbytecode_call(bytecode.cc:1571) andcc_safepointin the XEP before the tail call (cleavir/translate.lisp:1953) — and neither instruments loop back edges. A loop that calls nothing never reaches a safepoint, so no interrupt ever arrives.Measured before the change, all surviving
PROCESS-KILL:(loop)(loop until *flag*)(loop for i from 0)(loop (setq x (1+ x)))(do ((i 0 (1+ i))) (nil))(loop (funcall #'identity 1))(sleep …)So this is specifically the call-free case; anything containing a call was always fine.
The return value is not a signal
Worth stating because it misleads:
MP:PROCESS-KILLandMP:PROCESS-CANCELare both(MP:INTERRUPT process 'MP:CANCELLATION-INTERRUPT)and the C++ entry point isvoid, so both answerNILwhether or not the target dies.MP:PROCESS-ACTIVE-Pis the only way to tell. (They are also literally the same function; the in-tree FIXME says kill should be the forceful one. Not addressed here.)Three independent commits
1. Bytecode — poll when the jump offset is negative. All eight jump sites route through one helper.
vm._pc/vm._stackPointerare synced first, since the handler can cons or unwind and the GC scans to_stackPointer. The fast path uses theThreadLocalState*the VM already caches per frame rather thanmy_thread, which on Darwin would cost a_tlv_get_addrthunk per iteration. It cannot useinterrupt_queue_validp()— the queue keeps a dummy head node so that predicate is true whenever the queue exists; the real test is the head'scdr.Cost: ~1–2% on a loop whose entire body is one opcode, one of three alternating rounds inverted.
2. Native — BIR has no loop or dominance analysis, but iblocks are laid out in forward flow order, so a branch to an already-laid-out iblock is a back edge. Emitted from
translate-terminator :around, coveringjump/ifi/casein one place.Polling every back edge is not affordable:
cc_safepointis an out-of-line call andprimitive-unwinds, so LLVM cannot hoist loop-invariant work or keep values in registers across it — measured 3.0× on the same worst case. It cannot be redeclared non-throwing; the handler really can unwind, andcleavir/primitives.lisp:55warns against exactly that. So this samples: a counter in the alloca block, one call per 256 back edges, bounding cancellation latency at 255 iterations.Cost: ~8%, nine alternating rounds, down from 3×.
I'd flag that 8% as the weakest part of this PR. The residual is the inline counter, not the call, so raising the interval buys little. Two better options I did not take: read a global pending-interrupt flag (needs an atomic load, or LLVM hoists it out of the loop and silently disables the safepoint), or instrument only call-free loops — which would make almost all real code pay nothing, and is the one I'd pursue if you want this cheaper. Happy to drop commit 2 entirely if the cost isn't worth it to you; commits 1 and 3 stand without it.
3.
MP:WITH-TIMEOUT— there was no way to bound a form's running time; the only timeout primitive wasCONDITION-VARIABLE-TIMEDWAIT, which bounds one wait. The watchdog waits on a condition variable rather than sleeping, so an early finish doesn't strand a thread for the full duration, and the wait is a deadline loop becausetimedwaitmay return early.This depends on commit 1: the deadline is delivered as an interrupt, so the workload a timeout most needs to interrupt — a spinning thread — was the one workload it could not reach.
(MP:WITH-TIMEOUT (0.2) (LOOP))now signalsMP:TIMEOUT.MP:TIMEOUTis a subtype ofERROR, notSERIOUS-CONDITIONwhere SBCL and bordeaux-threads put theirs. A condition that slips throughHANDLER-CASEonERRORandIGNORE-ERRORSis a footgun. Deliberate, and easy to change if you disagree.Testing
Regression suite 2014 → 2021, seven new tests, same five expected failures by name, same five unexpected successes,
compilation unit aborted= 2. There were no cancellation tests at all before this;cancel-loop-with-callis included as a control that passed before and after, so a future regression in the general interrupt machinery stays distinguishable from one in the back-edge poll.Two caveats I'd rather state than have you find:
SIMPLE-CORE-FUNso it cannot pass by silently testing bytecode, but with no native compiler it returnsTwithout testing anything. Green in the twobytecode/no/nocells proves nothing about commit 2.boehmprecise, on a tree that also carried Stop allocating a scratch vector for every bytecode &key call #1820. The onlybytecode.ccdifference from this branch is Stop allocating a scratch vector for every bytecode &key call #1820's unrelatedparse_key_argsrework; CI here exercises the exact branch.The three commits are independent and cherry-pickable — happy to split them into separate PRs if you'd prefer to take them at different speeds.