perf(session): stop background load starving the focused shell [1] - #202
Draft
AbysmalBiscuit wants to merge 14 commits into
Draft
perf(session): stop background load starving the focused shell [1]#202AbysmalBiscuit wants to merge 14 commits into
AbysmalBiscuit wants to merge 14 commits into
Conversation
A build saturating every core starves the shell the user is typing into: the line editor redrawing its prompt waits behind sixteen compilers at the same scheduling class, and the keystroke round trip stretches into seconds. Raising the shell alone does not reach that, because a Windows priority class does not spread to the processes a raised process starts. A job object does: a process joins the job it is created in and comes up at the job's class, at any depth and with no scanning. The session takes the job at spawn rather than on focus, since anything started before the job exists escapes it for good. Only the session on screen, and only while the window has focus, is raised; the GUI follows so it does not lose to the tree it draws. Behind `[ui] focus_priority_boost`, off by default, and Windows only. A Unix nice value is already inherited and cannot be lowered back without privilege, so the platform module there is a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A job's priority class is a ceiling, not a setting. Releasing the boost named normal but left the limit standing, so every process an unfocused session held was capped there and could not raise itself — a build or an agent running under a background tab lost the class it asked for, and kept losing it until that tab was focused again. Release now names normal to lower the members already running, then clears the limit so they own their class again. Drop is the same two steps, so it needs nothing beyond releasing the boost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The console reaps only the programs attached to it, so anything that leaves it — an editor's background search, anything started detached — outlives the terminal that asked for it and piles up until the machine is rebooted. Nothing else holds those processes: the job object is the only thing that has every descendant at every depth. `[ui] reap_descendants_on_close`, off by default, gives the job kill-on-close, so the kernel ends its members when the last handle goes. That covers a killed or crashed alacritree as well as a closed tab, since no cleanup path has to run. Breakaway rides with it: a process that means to outlive the terminal asks with `CREATE_BREAKAWAY_FROM_JOB` and is let go, which is a request the kernel refuses without the flag. The lifetime limits travel with the priority one on every set, because `SetInformationJobObject` replaces `LimitFlags` whole and a change of focus would otherwise drop them. One job serves both options, so it is created when either wants it. The tests tear down real pseudoconsoles and check what outlived them, against an unjobbed baseline that pins any failure on the harness rather than the feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reaping option shipped without a test that the console misses anything, so nothing said whether the job was fixing a real leak. The escaping child is now a process started with `DETACHED_PROCESS`, which has no console and so is nobody's console client — the shape of a completion helper an editor spawns. Only a process already inside the session can start one, so the session runs this binary again and an ignored test does the spawning; that also keeps the desktop clear, where a new console would have opened a window. Two arms make the claim falsifiable: unjobbed, the child outlives the teardown, and a failure there would mean there is no leak to fix; jobbed, it does not. The session is jobbed before its tree is waited for, as a real session does it, because a process joins a job when it is created and anything already running stays outside it for good. Members carry their name and their kernel-reported job membership, so a survivor says on its own whether the model or the flag is at fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cmd.exe subjects and the taskkill sweep carried no CREATE_NO_WINDOW, so each one flashed a console window during a test run, and the subjects holding a `ping` open kept theirs for a minute. command_ext exists for this; the tests pipe their stdio and assert on job membership and priority, neither of which the flag touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A test binary has no `main`, so hardening was left to each test that
opened a pseudoconsole. The focus-priority tests added one that does not,
and once any PTY opens first the module it loaded answers every later
`LoadLibraryW("conpty.dll")` in the process. That is the three-second
console-host stall the handshake test exists to catch, and it started
failing on CI for tests that never touched it.
Harden immediately before each `tty::new` instead, guarded by a `Once` so
the repeat costs nothing and `main` keeps its startup call. The per-test
calls go away with the convention they were upholding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pid came from a subject the test had already killed and reaped, so Windows was free to hand that number to anything. Under a parallel run it handed it to another test's freshly spawned subject, which then read ABOVE_NORMAL where it expected NORMAL. Use a number the kernel never allocates, and assert that it names no process rather than trusting the arithmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every subject was born at whatever Windows chose, and the boost tests then asserted that choice was normal. Windows hands a new process its creator's job class when the creator is in a job, and half of these tests build jobs, so under a parallel run a subject sometimes came up above normal and the first assertion failed before the boost was even applied. Pass the class in the creation flags. The one test whose subject exists to report Windows' choice keeps making none, and says what to suspect if it ever comes up raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`tree_of` walked the process table by parent pid alone. Windows frees a pid for reuse the moment the last handle to it closes, so an orphan goes on naming the number its dead parent had and whoever is given that number next inherits it as a child. The teardown arms taskkill every survivor they find, which turned a wrong answer here into an unrelated process on the machine being killed: a build's stray vctip.exe was reaped as part of a session tree it never belonged to. No child predates its parent, so a start time earlier than the parent's rules the candidate out. A process the snapshot cannot open reports no start time at all, and those keep their place rather than being read as born at the epoch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Font fixtures went into the shared system temporary directory under fixed names. Unique within one binary, but two binaries running at once write the same paths, and a fixture one of them has mapped cannot be rewritten by the other: Windows fails the write with ERROR_USER_MAPPED_FILE. Five fonts tests failed together in one run here, which is what a second binary mapping its whole fixture set looks like. Hang the fixtures off a directory named for the process instead, and sweep the ones whose process has gone. The names lose the prefix that was disambiguating them inside the shared directory. Closes #46 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The probe for a foreign console host asserted that a pane's child
exited inside two seconds, then ten. Both bounds are wall-clock, so a
Windows runner that descheduled the polling thread failed the test with
nothing wrong: once on each bound, both passing on re-run.
What the probe is there to catch is `LoadLibraryW("conpty.dll")`
resolving out of PATH, and that is observable directly. A file named
conpty.dll that is not a module tells the two loader outcomes apart: a
loader that reaches PATH finds it and reports ERROR_BAD_EXE_FORMAT, a
hardened one reports ERROR_MOD_NOT_FOUND. Both arms run as child
processes because SetDefaultDllDirectories is process-wide and cannot
be undone, so the parent cannot host the unhardened arm.
The control arm pins the unhardened code as well, so a plant the loader
never reaches fails the test rather than passing it by default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`the_probe_hands_the_scan_to_the_refresher` asserted the call returned within 5 ms. A bound that tight fails whenever the runner deschedules the thread, and it cannot distinguish that from the failure it is aiming at, since a probe that scanned inline would blow it for the same reason a busy machine does. `probe` reads the published map and registers interest; it never scans. The assertion above it already pins the answer to the default, which only an unscanned probe can return, and the cost the bound stood for is what `report_process_probe_cost` reports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`each_phase_measures_only_its_own_span` slept 20 ms to give the first phase a span, then asserted the second one came in under 10 ms. The bound is what fails when a runner deschedules the thread between two adjacent statements, and 10 ms is close enough to be reachable. Back-dating `since` gives the first phase five seconds without waiting for them, and the claim is stated against it: a second phase still dating from the frame's start would be at least as long as the first, so `second < first` is the property, not an absolute ceiling. The test also stops costing 20 ms of every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The diff-pane title tests spawned a real `cmd /c exit` through ConPTY purely to obtain a `Session`, then polled up to ten seconds for the child to exit so its startup title could not race the injected one. A loaded runner reaches that bound with nothing wrong, which is what broke CI. The session under test needs no PTY: the tests inject their sequence straight into the terminal and drain it. Building it as a struct literal removes the child, the wait and the racing title together. Co-Authored-By: Claude Opus 5 (1M Context) <noreply@anthropic.com>
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.
TL;DR (human written)
DO NOT MERGE YET
I'm still testing this to make sure there are no weird edge cases. Opened the PR already to start setting up the perf pr stack. A UX/feature PR stack will come after that. ^^
This may be mainly a windows problem, where it doesn't manage process priorities as well as UNIX OSs. So when something would load CPU (e.g., several tsc typechecks, cargo build, etc.), it made alacritree unusable (see below for numbers).
This PR fixes the priority problems on Windows by setting a process group for the focused session to boost it's priority 1 level above the default. This fixes most of the UX problems under load.
There's a placeholder file
other.rsif these kinds of issues happen on other OSs, and they would need their own implementation.Perf improvements for opening a shell:
nushellfishPerf improvements for typing: 5-10s -> instant when typing slowly 1-2s lag when mashing keyboard
Closes AbysmalBiscuit#25
Claude summary below:
A build saturating every core starves the shell you are typing into. The line editor redrawing its prompt waits behind sixteen compilers at the same scheduling class, and the keystroke round trip stretches into seconds. Windows Terminal and Zed stay responsive under the same load.
Raising the shell's priority alone does not reach it, because a Windows priority class does not spread to the processes a raised process starts. A job object does: a process joins the job it is created in and comes up at the job's class, at any depth and with no scanning. The session takes its job at spawn rather than on focus, since anything started before the job exists escapes it permanently.
What this adds
[ui] focus_priority_boost, off by default, Windows only. Only the session on screen, and only while the window has focus, is raised. The GUI follows so it does not lose to the tree it draws. On Unix the platform module is a no-op: a nice value is already inherited and cannot be lowered back without privilege.Releasing the boost lowers the members already running and then clears the limit, so an unfocused tab's build or agent owns its own class again. A job's priority class is a ceiling rather than a setting, so clearing it matters.
[ui] reap_descendants_on_close, also off by default, gives the job kill-on-close. The console reaps only the programs attached to it, so anything that leaves it outlives the terminal and piles up until reboot. The job object is the only thing holding every descendant at every depth, and because the kernel does the ending, a killed or crashed alacritree is covered as well as a closed tab. A process that means to outlive the terminal asks withCREATE_BREAKAWAY_FROM_JOB.Both options share one job, created when either wants it. The lifetime limits travel with the priority one on every set, because
SetInformationJobObjectreplacesLimitFlagswhole.Measured
Spawn to a usable prompt for
nu.exeunder 64 spinning burners went from ~15s to ~3s.Tests
The reaping tests tear down real pseudoconsoles and check what outlived them. The escaping child is started with
DETACHED_PROCESS, so it is nobody's console client, the shape of a completion helper an editor spawns. Two arms keep the claim falsifiable: unjobbed the child outlives teardown, jobbed it does not. An unjobbed baseline pins any failure on the harness rather than the feature.Written by Claude Opus 5 in Claude Code.