From 82435075a5c727b8231b94f901027d94f37a74ef Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Mon, 24 Jun 2024 15:31:49 +0200 Subject: [PATCH] Revert "linux/x11: Reduce input latency and ensure rerender priority (#13355)" (#13465) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f69c8ca74eb934f37d29cbe2f6ee31f94c7aaf63 after it has already been partially reverted in https://github.com/zed-industries/zed/pull/13458. Why the revert? The changes in that commit/PR fix one type of problem — dropping of frames when being blasted with input events — but trades it for another one that I can't explain yet: when the system is under load, then input becomes _laggy_ and input events seem to be delayed. Two examples of how that shows up: 1. When the system is under load* and you hold down the `down` key to scroll, then lift the finger, the cursor stops sometimes. If you then produce another input event by jiggling the mouse cursor you'll see more `down`-key events coming up and the cursor moving down. It feels as if the event loop is not being woken up even though there are still events. I suspect it might have something to do with XIM, because if it's disabled, it seems as if problems become less severe. 2. When the system is under load* and you click-and-drag a selection in the editor, you can see how the selection is delayed and takes 500ms-1s to catch up to where the cursor is. * system under load: start Zed, then in another terminal window create a release build of Zed, for example. With the changes reverted, the failure mode looks different: we skip frames. But that, I think, is the better of two bad options, because skipping frames means that you see what's happening vs. input events seemingly still coming in seconds after you stopped using the keyboard. Release Notes: - N/A --- crates/gpui/src/platform/linux/x11/client.rs | 92 +++++++++++++------- crates/gpui/src/platform/linux/x11/window.rs | 45 ---------- 2 files changed, 59 insertions(+), 78 deletions(-) diff --git a/crates/gpui/src/platform/linux/x11/client.rs b/crates/gpui/src/platform/linux/x11/client.rs index bde4dfb4fa..f2ad5b9a86 100644 --- a/crates/gpui/src/platform/linux/x11/client.rs +++ b/crates/gpui/src/platform/linux/x11/client.rs @@ -13,6 +13,7 @@ use util::ResultExt; use x11rb::connection::{Connection, RequestConnection}; use x11rb::cursor; use x11rb::errors::ConnectionError; +use x11rb::protocol::randr::ConnectionExt as _; use x11rb::protocol::xinput::ConnectionExt; use x11rb::protocol::xkb::ConnectionExt as _; use x11rb::protocol::xproto::{ChangeWindowAttributesAux, ConnectionExt as _}; @@ -45,7 +46,7 @@ use crate::platform::linux::xdg_desktop_portal::{Event as XDPEvent, XDPEventSour pub(super) const XINPUT_MASTER_DEVICE: u16 = 1; pub(crate) struct WindowRef { - pub window: X11WindowStatePtr, + window: X11WindowStatePtr, refresh_event_token: RegistrationToken, } @@ -297,33 +298,7 @@ impl X11Client { { let xcb_connection = xcb_connection.clone(); move |_readiness, _, client| { - let windows = client - .0 - .borrow() - .windows - .values() - .map(|window_ref| window_ref.window.clone()) - .collect::>(); - while let Some(event) = xcb_connection.poll_for_event()? { - for window in &windows { - let last_render_at; - let refresh_rate; - { - let window_state = window.state.borrow(); - last_render_at = window_state.last_render_at; - refresh_rate = window_state.refresh_rate; - } - - if let Some(last_render_at) = last_render_at { - if last_render_at.elapsed() >= refresh_rate { - window.refresh(); - } - } else { - window.refresh(); - } - } - let mut state = client.0.borrow_mut(); if state.ximc.is_none() || state.xim_handler.is_none() { drop(state); @@ -980,18 +955,60 @@ impl LinuxClient for X11Client { state.common.appearance, )?; + let screen_resources = state + .xcb_connection + .randr_get_screen_resources(x_window) + .unwrap() + .reply() + .expect("Could not find available screens"); + + let mode = screen_resources + .crtcs + .iter() + .find_map(|crtc| { + let crtc_info = state + .xcb_connection + .randr_get_crtc_info(*crtc, x11rb::CURRENT_TIME) + .ok()? + .reply() + .ok()?; + + screen_resources + .modes + .iter() + .find(|m| m.id == crtc_info.mode) + }) + .expect("Unable to find screen refresh rate"); + let refresh_event_token = state .loop_handle .insert_source(calloop::timer::Timer::immediate(), { - let window = window.0.clone(); - let refresh_rate = window.state.borrow().refresh_rate; - move |mut instant, (), _| { - window.refresh(); - + let refresh_duration = mode_refresh_rate(mode); + move |mut instant, (), client| { + let state = client.0.borrow_mut(); + state + .xcb_connection + .send_event( + false, + x_window, + xproto::EventMask::EXPOSURE, + xproto::ExposeEvent { + response_type: xproto::EXPOSE_EVENT, + sequence: 0, + window: x_window, + x: 0, + y: 0, + width: 0, + height: 0, + count: 1, + }, + ) + .unwrap(); + let _ = state.xcb_connection.flush().unwrap(); // Take into account that some frames have been skipped let now = Instant::now(); while instant < now { - instant += refresh_rate; + instant += refresh_duration; } calloop::timer::TimeoutAction::ToInstant(instant) } @@ -1129,6 +1146,15 @@ impl LinuxClient for X11Client { } } +// Adatpted from: +// https://docs.rs/winit/0.29.11/src/winit/platform_impl/linux/x11/monitor.rs.html#103-111 +pub fn mode_refresh_rate(mode: &randr::ModeInfo) -> Duration { + let millihertz = mode.dot_clock as u64 * 1_000 / (mode.htotal as u64 * mode.vtotal as u64); + let micros = 1_000_000_000 / millihertz; + log::info!("Refreshing at {} micros", micros); + Duration::from_micros(micros) +} + fn fp3232_to_f32(value: xinput::Fp3232) -> f32 { value.integral as f32 + value.frac as f32 / u32::MAX as f32 } diff --git a/crates/gpui/src/platform/linux/x11/window.rs b/crates/gpui/src/platform/linux/x11/window.rs index 8f9bf5910e..4dea09393d 100644 --- a/crates/gpui/src/platform/linux/x11/window.rs +++ b/crates/gpui/src/platform/linux/x11/window.rs @@ -14,7 +14,6 @@ use util::{maybe, ResultExt}; use x11rb::{ connection::Connection, protocol::{ - randr::{self, ConnectionExt as _}, xinput::{self, ConnectionExt as _}, xproto::{ self, ClientMessageEvent, ConnectionExt as _, EventMask, TranslateCoordinatesReply, @@ -32,7 +31,6 @@ use std::{ ptr::NonNull, rc::Rc, sync::{self, Arc}, - time::{Duration, Instant}, }; use super::{X11Display, XINPUT_MASTER_DEVICE}; @@ -161,8 +159,6 @@ pub struct Callbacks { pub struct X11WindowState { pub destroyed: bool, - pub last_render_at: Option, - pub refresh_rate: Duration, client: X11ClientStatePtr, executor: ForegroundExecutor, atoms: XcbAtoms, @@ -401,31 +397,6 @@ impl X11WindowState { }; xcb_connection.map_window(x_window).unwrap(); - let screen_resources = xcb_connection - .randr_get_screen_resources(x_window) - .unwrap() - .reply() - .expect("Could not find available screens"); - - let mode = screen_resources - .crtcs - .iter() - .find_map(|crtc| { - let crtc_info = xcb_connection - .randr_get_crtc_info(*crtc, x11rb::CURRENT_TIME) - .ok()? - .reply() - .ok()?; - - screen_resources - .modes - .iter() - .find(|m| m.id == crtc_info.mode) - }) - .expect("Unable to find screen refresh rate"); - - let refresh_rate = mode_refresh_rate(mode); - Ok(Self { client, executor, @@ -442,8 +413,6 @@ impl X11WindowState { appearance, handle, destroyed: false, - last_render_at: None, - refresh_rate, }) } @@ -613,11 +582,6 @@ impl X11WindowStatePtr { let mut cb = self.callbacks.borrow_mut(); if let Some(ref mut fun) = cb.request_frame { fun(); - - self.state - .borrow_mut() - .last_render_at - .replace(Instant::now()); } } @@ -1064,12 +1028,3 @@ impl PlatformWindow for X11Window { false } } - -// Adatpted from: -// https://docs.rs/winit/0.29.11/src/winit/platform_impl/linux/x11/monitor.rs.html#103-111 -pub fn mode_refresh_rate(mode: &randr::ModeInfo) -> Duration { - let millihertz = mode.dot_clock as u64 * 1_000 / (mode.htotal as u64 * mode.vtotal as u64); - let micros = 1_000_000_000 / millihertz; - log::info!("Refreshing at {} micros", micros); - Duration::from_micros(micros) -}