fix(renderer): prevent display teardown starvation during output floods
Closing a busy split or confirming a tab close could freeze GTK while unrealizing the GLArea. Both reported GDB traces show the main thread waiting for the CPU frame-state mutex while the renderer keeps rebuilding dense text. GTK unrealizes the display before stopping the workers, so continuous output can keep the competing renderer active indefinitely. Use parking_lot fair mutexes for CPU frame state and GPU draw state. A waiting lifecycle or draw callback receives ownership instead of losing it to the next frame update. Keep the existing lock order and resource cleanup; no output, scrollback, or frame-rate limits are introduced. The fair-lock handoff adds scheduling overhead when the locks are contended. Correct the threading documentation: the CPU state is also accessed by apprt callbacks, and the Rust-only mutex is therefore contended. Test plan: - Add a mock GPU regression continuously rebuilding 200x150 dense cells while another thread unrealizes and realizes the display. It failed its three-second deadline with the old locks and passes with fair locks. - All 235 renderer/core tests pass, including Mesa EGL rendering tests. - Clippy for renderer/core/GTK, all targets, passes with -D warnings. - Nightly formatting check and release binary build pass. - GTK/Wayland replay with font size 9 and 1 GiB scrollback runs base64 floods in two splits, confirms split and tab closes, then successfully executes and renders a command in the surviving terminal. Docs: docs/rewrite/ARCHITECTURE.md
This commit is contained in:
1 parent
9010117d08
commit
2e5ec06b4e
5 files changed
+124
-40
No files matched your search
Generated
+1
@@ -1673,6 +1673,7 @@ dependencies = [
|
||||
"objc2-metal",
|
||||
"objc2-quartz-core",
|
||||
"onig",
|
||||
"parking_lot",
|
||||
"png",
|
||||
"thiserror 2.0.21",
|
||||
]
|
||||
|
||||
@@ -23,6 +23,7 @@ khronos-egl = { version = "6.0.0", features = ["dynamic"] }
|
||||
log = "0.4.34"
|
||||
naga = { version = "30.0.1", features = ["glsl-in", "glsl-out", "msl-out", "wgsl-in"] }
|
||||
onig = "6.5.3"
|
||||
parking_lot = "0.12.5"
|
||||
png = "0.18.1"
|
||||
thiserror = "2.0.21"
|
||||
|
||||
|
||||
@@ -24,14 +24,19 @@
|
||||
//! The renderer state is split to make that safe without `unsafe`:
|
||||
//!
|
||||
//! * `draw` ([`DrawState`], Zig's `draw_mutex`): everything `draw_frame`
|
||||
//! reads. Held by the renderer thread only for the short "publish" part of
|
||||
//! `update_frame` (`rebuild_cells`, uniforms) and by `set_*`/`change_config`.
|
||||
//! * `thread` ([`ThreadState`]): data only the renderer thread touches
|
||||
//! (`RenderState`, font shaper + cache, overlay, search highlights). It is a
|
||||
//! `Mutex` only so that `Renderer` is `Sync`; it is uncontended.
|
||||
//! reads. Held by the renderer thread while rebuilding cells/publishing
|
||||
//! uniforms in `update_frame`, and by `set_*`/`change_config`.
|
||||
//! * `thread` ([`ThreadState`]): render snapshots, font shaper + cache, overlay
|
||||
//! and search highlights. Display teardown and dirty marking also access
|
||||
//! this state from the apprt thread.
|
||||
//!
|
||||
//! Lock order is always: renderer mutex (`SharedState`) is *not* held while
|
||||
//! taking `draw`; `thread` before `draw` before the font grid lock.
|
||||
//! Both locks are fair: a worker continuously rebuilding dense frames must
|
||||
//! hand ownership to a waiting apprt callback instead of immediately taking
|
||||
//! the lock again. In particular, GLArea unrealize runs before worker shutdown
|
||||
//! when closing tabs/splits and must make progress while output is still live.
|
||||
//!
|
||||
//! Lock order is `thread`, terminal (`SharedState`), `draw`, then the font
|
||||
//! grid lock. Terminal snapshotting releases `SharedState` before shaping.
|
||||
//!
|
||||
//! # GL context requirements
|
||||
//!
|
||||
@@ -88,6 +93,7 @@ use ghostty_terminal::{
|
||||
terminal::ScrollViewport,
|
||||
};
|
||||
use ghostty_vt::{color::Rgb, modes::Mode, sgr::Underline};
|
||||
use parking_lot::FairMutex;
|
||||
|
||||
use crate::{
|
||||
backend::Health,
|
||||
@@ -684,7 +690,7 @@ struct DrawState<G: GraphicsApi> {
|
||||
shaders: Option<G::Shaders>,
|
||||
}
|
||||
|
||||
/// State only touched by the renderer thread.
|
||||
/// CPU frame state shared with apprt display lifecycle and dirty marking.
|
||||
struct ThreadState {
|
||||
/// The thread's copy of the derived config (kept in sync with
|
||||
/// `DrawState::config`).
|
||||
@@ -727,8 +733,8 @@ struct ThreadState {
|
||||
|
||||
/// The generic renderer. See the module docs for the threading model.
|
||||
pub struct Renderer<G: GraphicsApi = OpenGL> {
|
||||
draw: Mutex<DrawState<G>>,
|
||||
thread: Mutex<ThreadState>,
|
||||
draw: FairMutex<DrawState<G>>,
|
||||
thread: FairMutex<ThreadState>,
|
||||
|
||||
/// Health of the most recently completed frame.
|
||||
health: Arc<AtomicU8>,
|
||||
@@ -1448,8 +1454,8 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
Ok(Renderer {
|
||||
display_link: Mutex::new(display_link),
|
||||
draw: Mutex::new(draw),
|
||||
thread: Mutex::new(thread),
|
||||
draw: FairMutex::new(draw),
|
||||
thread: FairMutex::new(thread),
|
||||
health: Arc::new(AtomicU8::new(Health::Healthy as u8)),
|
||||
has_animations,
|
||||
host: opts.host,
|
||||
@@ -1464,7 +1470,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
pub fn display_realized_with(&self, realize: impl FnOnce(&mut G)) -> Result<(), Error> {
|
||||
// Lock the draw mutex so that we can
|
||||
// safely reinitialize our GPU resources.
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// If our API has to do things on realize, let it.
|
||||
realize(&mut d.api);
|
||||
@@ -1495,8 +1501,8 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
// Lock the draw mutex so that we can
|
||||
// safely deinitialize our GPU resources. (The thread state is locked
|
||||
// first, per the lock order.)
|
||||
let mut t = lock(&self.thread);
|
||||
let mut d = lock(&self.draw);
|
||||
let mut t = self.thread.lock();
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// We deinit our swap chain and shaders.
|
||||
//
|
||||
@@ -1517,7 +1523,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
/// Mark the full screen as dirty so that we redraw everything.
|
||||
pub fn mark_dirty(&self) {
|
||||
lock(&self.thread).terminal_state.dirty = Dirty::Full;
|
||||
self.thread.lock().terminal_state.dirty = Dirty::Full;
|
||||
}
|
||||
|
||||
/// True if our renderer has animations so that a higher frequency
|
||||
@@ -1539,17 +1545,17 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
/// The font grid currently used for rendering (for apprts that need
|
||||
/// to read the primary face, e.g. macOS quicklook).
|
||||
pub fn font_grid(&self) -> Arc<SharedGrid> {
|
||||
Arc::clone(&lock(&self.draw).font_grid)
|
||||
Arc::clone(&self.draw.lock().font_grid)
|
||||
}
|
||||
|
||||
/// Called by the renderer thread when it begins (`threadEnter`).
|
||||
pub fn thread_enter(&self) {
|
||||
lock(&self.draw).api.thread_enter();
|
||||
self.draw.lock().api.thread_enter();
|
||||
}
|
||||
|
||||
/// Called by the renderer thread when it exits (`threadExit`).
|
||||
pub fn thread_exit(&self) {
|
||||
lock(&self.draw).api.thread_exit();
|
||||
self.draw.lock().api.thread_exit();
|
||||
}
|
||||
|
||||
/// Called by the renderer thread when it starts the main loop
|
||||
@@ -1559,7 +1565,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
// If our API has to do things on loop enter, let it. (Metal draws
|
||||
// synchronously from the layer's display callback.)
|
||||
let weak = Arc::downgrade(self);
|
||||
lock(&self.draw).api.loop_enter(Arc::new(move || {
|
||||
self.draw.lock().api.loop_enter(Arc::new(move || {
|
||||
if let Some(renderer) = weak.upgrade()
|
||||
&& let Err(err) = renderer.draw_frame(true)
|
||||
{
|
||||
@@ -1586,7 +1592,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
/// (`loopExit`).
|
||||
pub fn loop_exit(&self) {
|
||||
// If our API has to do things on loop exit, let it.
|
||||
lock(&self.draw).api.loop_exit();
|
||||
self.draw.lock().api.loop_exit();
|
||||
|
||||
// Stop our display link. If this fails its okay it just means
|
||||
// that we either never started it or the view its attached to
|
||||
@@ -1620,14 +1626,14 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
/// The current size of everything as the renderer knows it.
|
||||
pub fn size(&self) -> Size {
|
||||
lock(&self.draw).size
|
||||
self.draw.lock().size
|
||||
}
|
||||
|
||||
/// Callback when the focus changes for the terminal this is rendering.
|
||||
///
|
||||
/// Must be called on the render thread.
|
||||
pub fn set_focus(&self, focus: bool) {
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
debug_assert_ne!(d.focused, focus);
|
||||
|
||||
d.focused = focus;
|
||||
@@ -1650,7 +1656,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
// If we're not visible, then we want to stop the display link
|
||||
// because it is a waste of resources and we can move to pure
|
||||
// change-driven updates.
|
||||
let focused = lock(&self.draw).focused;
|
||||
let focused = self.draw.lock().focused;
|
||||
if let Some(link) = lock(&self.display_link).as_ref() {
|
||||
let _ = if visible && focused {
|
||||
link.start()
|
||||
@@ -1664,8 +1670,8 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
///
|
||||
/// Must be called on the render thread.
|
||||
pub fn set_font_grid(&self, grid: Arc<SharedGrid>) {
|
||||
let mut t = lock(&self.thread);
|
||||
let mut d = lock(&self.draw);
|
||||
let mut t = self.thread.lock();
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// Get our metrics from the grid. This doesn't require a lock because
|
||||
// the metrics are never recalculated.
|
||||
@@ -1702,8 +1708,8 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
/// Update the configuration.
|
||||
#[allow(clippy::float_cmp)]
|
||||
pub fn change_config(&self, config: DerivedConfig) -> Result<(), Error> {
|
||||
let mut t = lock(&self.thread);
|
||||
let mut d = lock(&self.draw);
|
||||
let mut t = self.thread.lock();
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// We always redo the font shaper in case font features changed. We
|
||||
// could check to see if there was an actual config change but this is
|
||||
@@ -1773,7 +1779,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
/// Resize the screen.
|
||||
pub fn set_screen_size(&self, size: Size) {
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// We only actually need the padding from this,
|
||||
// everything else is derived elsewhere.
|
||||
@@ -1786,14 +1792,14 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
/// Set the search highlights (from the renderer thread mailbox).
|
||||
pub fn set_search_viewport_matches(&self, matches: Vec<Flattened>) {
|
||||
let mut t = lock(&self.thread);
|
||||
let mut t = self.thread.lock();
|
||||
t.search_matches = Some(matches);
|
||||
t.search_matches_dirty = true;
|
||||
}
|
||||
|
||||
/// Set the selected search match (from the renderer thread mailbox).
|
||||
pub fn set_search_selected_match(&self, m: Option<Flattened>) {
|
||||
let mut t = lock(&self.thread);
|
||||
let mut t = self.thread.lock();
|
||||
t.search_selected_match = m;
|
||||
t.search_matches_dirty = true;
|
||||
}
|
||||
@@ -1836,7 +1842,7 @@ impl Renderer<OpenGL> {
|
||||
/// most recently presented frame. Intended for tests and screenshots;
|
||||
/// requires the GL context to be current.
|
||||
pub fn last_frame_pixels(&self) -> Option<(usize, usize, Vec<u8>)> {
|
||||
let d = lock(&self.draw);
|
||||
let d = self.draw.lock();
|
||||
let target = d.api.last_target()?;
|
||||
let gl = d.api.gl()?.clone();
|
||||
let bind = gl.bind_framebuffer(
|
||||
@@ -1877,7 +1883,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
state: &SharedState,
|
||||
cursor_blink_visible: bool,
|
||||
) -> Result<(), Error> {
|
||||
let mut guard = lock(&self.thread);
|
||||
let mut guard = self.thread.lock();
|
||||
let t = &mut *guard;
|
||||
|
||||
// We fully deinit and reset the terminal state every so often
|
||||
@@ -1944,7 +1950,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
// kitty state on every frame because any cell change can move
|
||||
// an image.
|
||||
{
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
if d.images.kitty_requires_update(&st.terminal) {
|
||||
let cell_size = CellSize {
|
||||
width: d.grid_metrics.cell_width,
|
||||
@@ -2040,7 +2046,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
|
||||
// Acquire the draw mutex for all remaining state updates.
|
||||
{
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
// Build our GPU cells
|
||||
let cursor_style = cursor::style(
|
||||
@@ -2113,7 +2119,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let size = lock(&self.draw).size;
|
||||
let size = self.draw.lock().size;
|
||||
|
||||
// If we had a previous overlay, clear it. Otherwise, init.
|
||||
let reuse = match &t.overlay {
|
||||
@@ -3207,7 +3213,7 @@ impl<G: GraphicsApi> Renderer<G> {
|
||||
pub fn draw_frame(&self, sync: bool) -> Result<(), Error> {
|
||||
// We hold a the draw mutex to prevent changes to any
|
||||
// data we access while we're in the middle of drawing.
|
||||
let mut d = lock(&self.draw);
|
||||
let mut d = self.draw.lock();
|
||||
|
||||
let result = self.draw_frame_locked(&mut d, sync);
|
||||
|
||||
|
||||
@@ -11,7 +11,7 @@ use std::{
|
||||
sync::{
|
||||
Arc,
|
||||
Mutex,
|
||||
atomic::{AtomicUsize, Ordering},
|
||||
atomic::{AtomicBool, AtomicUsize, Ordering},
|
||||
},
|
||||
time::Duration,
|
||||
};
|
||||
@@ -283,6 +283,13 @@ impl Host for TestHost {
|
||||
}
|
||||
|
||||
fn fixture() -> (Renderer<MockApi>, Arc<Mutex<State>>, Log, Arc<AtomicUsize>) {
|
||||
fixture_with_size(20, 5)
|
||||
}
|
||||
|
||||
fn fixture_with_size(
|
||||
columns: u16,
|
||||
rows: u16,
|
||||
) -> (Renderer<MockApi>, Arc<Mutex<State>>, Log, Arc<AtomicUsize>) {
|
||||
let config = Config::default();
|
||||
let set = Arc::new(SharedGridSet::new().unwrap());
|
||||
let (_key, grid) = set
|
||||
@@ -300,7 +307,10 @@ fn fixture() -> (Renderer<MockApi>, Arc<Mutex<State>>, Log, Arc<AtomicUsize>) {
|
||||
width: gc.width,
|
||||
height: gc.height,
|
||||
};
|
||||
let (w, h) = (cell.width * 20 + 20, cell.height * 5 + 20);
|
||||
let (w, h) = (
|
||||
cell.width * u32::from(columns) + 20,
|
||||
cell.height * u32::from(rows) + 20,
|
||||
);
|
||||
let size = Size {
|
||||
screen: ScreenSize {
|
||||
width: w,
|
||||
@@ -315,7 +325,7 @@ fn fixture() -> (Renderer<MockApi>, Arc<Mutex<State>>, Log, Arc<AtomicUsize>) {
|
||||
},
|
||||
};
|
||||
|
||||
let mut term = Terminal::new(TermOptions::new(20, 5)).unwrap();
|
||||
let mut term = Terminal::new(TermOptions::new(columns, rows)).unwrap();
|
||||
term.vt_stream().next_slice(b"hello").unwrap();
|
||||
let state = State::new(term).into_shared();
|
||||
|
||||
@@ -396,3 +406,61 @@ fn display_unrealize_waits_for_inflight_frames() {
|
||||
renderer.draw_frame(true).unwrap();
|
||||
assert_eq!(completed.load(Ordering::SeqCst), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn display_unrealize_makes_progress_during_continuous_updates() {
|
||||
let (renderer, state, _log, _completed) = fixture_with_size(200, 150);
|
||||
// Dense, varied text forces actual shaping and glyph rebuilding rather
|
||||
// than repeatedly taking the clean-frame shortcut.
|
||||
let mut seed = 1_u32;
|
||||
let text: Vec<_> = (0..30_000)
|
||||
.map(|_| {
|
||||
seed = seed.wrapping_mul(1_664_525).wrapping_add(1_013_904_223);
|
||||
b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"
|
||||
[(seed >> 24) as usize % 64]
|
||||
})
|
||||
.collect();
|
||||
state
|
||||
.lock()
|
||||
.unwrap()
|
||||
.terminal
|
||||
.vt_stream()
|
||||
.next_slice(&text)
|
||||
.unwrap();
|
||||
|
||||
let renderer = Arc::new(renderer);
|
||||
let stop = Arc::new(AtomicBool::new(false));
|
||||
let (started_tx, started_rx) = std::sync::mpsc::channel();
|
||||
let updater = {
|
||||
let renderer = Arc::clone(&renderer);
|
||||
let stop = Arc::clone(&stop);
|
||||
std::thread::spawn(move || {
|
||||
renderer.update_frame(&state, true).unwrap();
|
||||
started_tx.send(()).unwrap();
|
||||
while !stop.load(Ordering::Relaxed) {
|
||||
renderer.mark_dirty();
|
||||
renderer.update_frame(&state, true).unwrap();
|
||||
}
|
||||
})
|
||||
};
|
||||
started_rx.recv_timeout(Duration::from_secs(5)).unwrap();
|
||||
|
||||
let (done_tx, done_rx) = std::sync::mpsc::channel();
|
||||
let lifecycle = {
|
||||
let renderer = Arc::clone(&renderer);
|
||||
std::thread::spawn(move || {
|
||||
// Closing a split/tab unrealizes its GLArea before worker shutdown.
|
||||
// Reparenting a surviving surface also realizes it again.
|
||||
renderer.display_unrealized();
|
||||
renderer.display_realized_with(|_| {}).unwrap();
|
||||
done_tx.send(()).unwrap();
|
||||
})
|
||||
};
|
||||
let result = done_rx.recv_timeout(Duration::from_secs(3));
|
||||
// Always release a starved lifecycle thread so a regression fails instead
|
||||
// of leaving the test process hung indefinitely.
|
||||
stop.store(true, Ordering::Relaxed);
|
||||
updater.join().unwrap();
|
||||
lifecycle.join().unwrap();
|
||||
assert!(result.is_ok(), "display lifecycle starved by frame updates");
|
||||
}
|
||||
@@ -32,6 +32,8 @@ ghostty (bin, src/main.rs) CLI `+action` dispatch or GTK app startup
|
||||
│ ├── kitty / search / tmux kitty graphics, search thread, tmux control mode
|
||||
│ └── ghostty-vt byte stream -> Parser -> Stream -> Handler actions
|
||||
├── ghostty-renderer OpenGL renderer (glow), renderer thread, shaders
|
||||
│ ├── generic::Renderer CPU frame state + GPU draw state; fair mutexes
|
||||
│ │ let GTK display teardown run during output
|
||||
│ └── ghostty-font fontconfig/freetype/harfbuzz, sprite font, atlas
|
||||
├── ghostty-inspector Dear ImGui terminal inspector
|
||||
└── ghostty-config config file/CLI parsing, themes, keybinds
|
||||
@@ -128,6 +130,12 @@ GTK invokes `glib-compile-resources` directly so generated XML/UI files in
|
||||
thread, plus a pty read thread. Mailboxes are `crossbeam-channel` /
|
||||
`std::sync::mpsc`; the GTK main thread is woken via `glib` channels.
|
||||
The `Terminal` is shared behind `Arc<Mutex<..>>` (Ghostty's renderer mutex).
|
||||
The generic renderer uses fair mutexes for its CPU frame and GPU draw state:
|
||||
continuous frame updates hand ownership to waiting GTK lifecycle/draw
|
||||
callbacks. Closing a tab/split unrealizes its GLArea before worker shutdown;
|
||||
teardown must acquire these locks while the PTY is still producing output.
|
||||
This is a Rust synchronization fix; Zig does not have the extra CPU-state
|
||||
mutex used by the rewrite.
|
||||
`ghostty-core::App` handles at most 64 mailbox messages per tick and wakes
|
||||
the runtime again if work remains. Continuous renderer notifications must
|
||||
yield to GTK input, timers, and drawing instead of extending one tick
|
||||
|
||||
Reference in new issue
Block a user