From fca006d8cba5130145c1b8ae31bc78ce887c129b Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Jul 2020 08:58:00 +0200 Subject: [PATCH 1/9] Implement request_timer for the x11 platform Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/util.rs | 35 ++++++++++++++++++++++++++ druid-shell/src/platform/x11/window.rs | 21 ++++++++++------ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/druid-shell/src/platform/x11/util.rs b/druid-shell/src/platform/x11/util.rs index 44ec6796b3..906401f453 100644 --- a/druid-shell/src/platform/x11/util.rs +++ b/druid-shell/src/platform/x11/util.rs @@ -14,13 +14,17 @@ //! Miscellaneous utility functions for working with X11. +use std::cmp::Ordering; use std::rc::Rc; +use std::time::Instant; use anyhow::{anyhow, Error}; use x11rb::protocol::randr::{ConnectionExt, ModeFlag}; use x11rb::protocol::xproto::{Screen, Visualtype, Window}; use x11rb::xcb_ffi::XCBConnection; +use crate::window::TimerToken; + // See: https://github.com/rtbo/rust-xcb/blob/master/examples/randr_screen_modes.rs pub fn refresh_rate(conn: &Rc, window_id: Window) -> Option { let try_refresh_rate = || -> Result { @@ -87,3 +91,34 @@ macro_rules! log_x11 { } }; } + +/// A timer is a deadline (`std::Time::Instant`) and a `TimerToken`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct Timer { + deadline: Instant, + token: TimerToken, +} + +impl Timer { + pub(crate) fn new(deadline: Instant) -> Self { + let token = TimerToken::next(); + Self { deadline, token } + } + + pub(crate) fn token(&self) -> TimerToken { + self.token + } +} + +impl Ord for Timer { + fn cmp(&self, other: &Self) -> Ordering { + // Ordering is so that earliest deadline sorts first + self.deadline.cmp(&other.deadline).reverse() + } +} + +impl PartialOrd for Timer { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index a9a8585c56..365d62d6a8 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -17,9 +17,11 @@ use std::any::Any; use std::cell::RefCell; use std::convert::{TryFrom, TryInto}; +use std::collections::BinaryHeap; use std::os::unix::io::RawFd; use std::rc::{Rc, Weak}; use std::sync::{Arc, Mutex}; +use std::time::Instant; use anyhow::{anyhow, Context, Error}; use cairo::{XCBConnection as CairoXCBConnection, XCBDrawable, XCBSurface, XCBVisualType}; @@ -47,7 +49,7 @@ use crate::window::{IdleToken, Text, TimerToken, WinHandler}; use super::application::Application; use super::keycodes; use super::menu::Menu; -use super::util; +use super::util::{self, Timer}; /// A version of XCB's `xcb_visualtype_t` struct. This was copied from the [example] in x11rb; it /// is used to interoperate with cairo. @@ -296,6 +298,7 @@ impl WindowBuilder { cairo_surface, atoms, state, + timer_queue: Mutex::new(BinaryHeap::new()), idle_queue: Arc::new(Mutex::new(Vec::new())), idle_pipe: self.app.idle_pipe(), present_data: RefCell::new(present_data), @@ -350,6 +353,8 @@ pub(crate) struct Window { cairo_surface: RefCell, atoms: WindowAtoms, state: RefCell, + /// Timers, sorted by "earliest deadline first" + timer_queue: Mutex>, idle_queue: Arc>>, // Writing to this wakes up the event loop, so that it can run idle handlers. idle_pipe: RawFd, @@ -1339,12 +1344,14 @@ impl WindowHandle { Text::new() } - pub fn request_timer(&self, _deadline: std::time::Instant) -> TimerToken { - // TODO(x11/timers): implement WindowHandle::request_timer - // This one might be tricky, since there's not really any timers to hook into in X11. - // Might have to code up our own Timer struct, running in its own thread? - log::warn!("WindowHandle::resizeable is currently unimplemented for X11 platforms."); - TimerToken::INVALID + pub fn request_timer(&self, deadline: Instant) -> TimerToken { + if let Some(w) = self.window.upgrade() { + let timer = Timer::new(deadline); + w.timer_queue.lock().unwrap().push(timer); + timer.token() + } else { + TimerToken::INVALID + } } pub fn set_cursor(&mut self, _cursor: &Cursor) { From 518a4516a18119757123e742b9bf0e88edc4209f Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Jul 2020 09:30:26 +0200 Subject: [PATCH 2/9] Handle timers in the X11 platform's main loop This makes the following command work correctly (as far as I can see): cargo run --example invalidate --no-default-features --features x11 Fixes-half-of: https://github.com/linebender/druid/issues/934 Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 52 +++++++++++++++++---- druid-shell/src/platform/x11/util.rs | 4 ++ druid-shell/src/platform/x11/window.rs | 21 +++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index d346c6389b..a87c01b3f1 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -381,7 +381,18 @@ impl Application { let timeout = Duration::from_millis((1000.0 / refresh_rate) as u64); let mut last_idle_time = Instant::now(); loop { + // Figure out when the next wakeup needs to happen + let next_timeout = if let Ok(state) = self.state.try_borrow() { + state.windows + .values() + .filter_map(|w| w.next_timeout()) + .min() + } else { + log::error!("Getting next timeout, application state already borrowed"); + None + }; let next_idle_time = last_idle_time + timeout; + self.connection.flush()?; // Before we poll on the connection's file descriptor, check whether there are any @@ -390,7 +401,7 @@ impl Application { let mut event = self.connection.poll_for_event()?; if event.is_none() { - poll_with_timeout(&self.connection, self.idle_read, next_idle_time) + poll_with_timeout(&self.connection, self.idle_read, next_timeout, next_idle_time) .context("Error while waiting for X11 connection")?; } @@ -409,6 +420,17 @@ impl Application { } let now = Instant::now(); + if let Some(timeout) = next_timeout { + if timeout <= now { + if let Ok(state) = self.state.try_borrow() { + for w in state.windows.values() { + w.run_timers(now); + } + } else { + log::error!("In timer loop, application state already borrowed"); + } + } + } if now >= next_idle_time { last_idle_time = now; drain_idle_pipe(self.idle_read)?; @@ -510,11 +532,20 @@ fn drain_idle_pipe(idle_read: RawFd) -> Result<(), Error> { /// writing into our idle pipe and the `timeout` has passed. // This was taken, with minor modifications, from the xclock_utc example in the x11rb crate. // https://github.com/psychon/x11rb/blob/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs -fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> Result<(), Error> { +fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Option, idle_timeout: Instant) -> Result<(), Error> { use nix::poll::{poll, PollFd, PollFlags}; use std::os::raw::c_int; use std::os::unix::io::AsRawFd; + fn to_timeout(timeout: Instant, now: Instant) -> c_int { + if timeout < now { + 0 + } else { + c_int::try_from(timeout.duration_since(now).as_millis()) + .unwrap_or(c_int::max_value()) + } + } + let fd = conn.as_raw_fd(); let mut both_poll_fds = [ PollFd::new(fd, PollFlags::POLLIN), @@ -526,6 +557,10 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> // We start with no timeout in the poll call. If we get something from the idle handler, we'll // start setting one. let mut poll_timeout = -1; + // However, we still have to honor the timer_timeout + if let Some(timeout) = timer_timeout { + poll_timeout = to_timeout(timeout, Instant::now()); + } loop { fn readable(p: PollFd) -> bool { p.revents() @@ -539,18 +574,15 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> // There is an X11 event ready to be handled. break; } + let now = Instant::now(); + if now >= idle_timeout.min(timer_timeout.unwrap_or(idle_timeout)) { + break; + } if poll_fds.len() == 1 || readable(poll_fds[1]) { // Now that we got signalled, stop polling from the idle pipe and use a timeout // instead. poll_fds = &mut just_connection; - - let now = Instant::now(); - if now >= timeout { - break; - } else { - poll_timeout = c_int::try_from(timeout.duration_since(now).as_millis()) - .unwrap_or(c_int::max_value()) - } + poll_timeout = to_timeout(idle_timeout, now); } } diff --git a/druid-shell/src/platform/x11/util.rs b/druid-shell/src/platform/x11/util.rs index 906401f453..7d30a33b42 100644 --- a/druid-shell/src/platform/x11/util.rs +++ b/druid-shell/src/platform/x11/util.rs @@ -105,6 +105,10 @@ impl Timer { Self { deadline, token } } + pub(crate) fn deadline(&self) -> Instant { + self.deadline + } + pub(crate) fn token(&self) -> TimerToken { self.token } diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 365d62d6a8..f2def4ac37 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -1009,6 +1009,27 @@ impl Window { } } } + + pub(crate) fn next_timeout(&self) -> Option { + if let Some(timer) = self.timer_queue.lock().unwrap().peek() { + Some(timer.deadline()) + } else { + None + } + } + + pub(crate) fn run_timers(&self, now: Instant) { + while let Some(deadline) = self.next_timeout() { + if deadline > now { + break; + } + // Remove the timer and get the token + let token = self.timer_queue.lock().unwrap().pop().unwrap().token(); + if let Ok(mut handler_borrow) = self.handler.try_borrow_mut() { + handler_borrow.timer(token); + } + } + } } impl Buffers { From 605bbbf8700d78118f7f055d92d7bb72d51eca0b Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 18 Jul 2020 15:29:07 +0200 Subject: [PATCH 3/9] Update changelog Signed-off-by: Uli Schlachter --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74c3023bda..9e6f045714 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ You can find its changes [documented below](#060---2020-06-01). - X11: Support idle callbacks. ([#1072] by [@jneem]) - GTK: Don't interrupt `KeyEvent.repeat` when releasing another key. ([#1081] by [@raphlinus]) - X11: Set some more common window properties. ([#1097] by [@psychon]) +- X11: Support timers. ([#1096] by [@psychon]) ### Visual @@ -361,6 +362,7 @@ Last release without a changelog :( [#1062]: https://github.com/linebender/druid/pull/1062 [#1072]: https://github.com/linebender/druid/pull/1072 [#1081]: https://github.com/linebender/druid/pull/1081 +[#1096]: https://github.com/linebender/druid/pull/1096 [#1097]: https://github.com/linebender/druid/pull/1097 [Unreleased]: https://github.com/linebender/druid/compare/v0.6.0...master From 17f49c4236e498a2bf06c6a6b8daadfb92cc4244 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 19 Jul 2020 11:05:53 +0200 Subject: [PATCH 4/9] Clarify comment on Timer ordering Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/util.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/util.rs b/druid-shell/src/platform/x11/util.rs index 7d30a33b42..98729ba62a 100644 --- a/druid-shell/src/platform/x11/util.rs +++ b/druid-shell/src/platform/x11/util.rs @@ -115,8 +115,10 @@ impl Timer { } impl Ord for Timer { + /// Ordering is so that earliest deadline sorts first + // "Earliest deadline first" that a std::collections::BinaryHeap will have the earliest timer + // at its head, which is just what is needed for timer management. fn cmp(&self, other: &Self) -> Ordering { - // Ordering is so that earliest deadline sorts first self.deadline.cmp(&other.deadline).reverse() } } From 50dd01764e869d80ebbd6afc5748ebe530c9a597 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 19 Jul 2020 11:07:40 +0200 Subject: [PATCH 5/9] Apply suggestion by jneem Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index a87c01b3f1..b5ef073833 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -575,7 +575,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio break; } let now = Instant::now(); - if now >= idle_timeout.min(timer_timeout.unwrap_or(idle_timeout)) { + if timer_timeout.is_some() && now >= timer_timeout.unwrap() { break; } if poll_fds.len() == 1 || readable(poll_fds[1]) { @@ -583,6 +583,9 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio // instead. poll_fds = &mut just_connection; poll_timeout = to_timeout(idle_timeout, now); + if now >= idle_timeout { + break; + } } } From 3bed70bfaaad1cc04bef60205a993fa12581672c Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 19 Jul 2020 11:11:43 +0200 Subject: [PATCH 6/9] Use correct timeout when idle pipe becomes readable Thanks to @jneem for catching this. Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index b5ef073833..8d879eb089 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -546,6 +546,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio } } + let earliest_timeout = idle_timeout.min(timer_timeout.unwrap_or(idle_timeout)); let fd = conn.as_raw_fd(); let mut both_poll_fds = [ PollFd::new(fd, PollFlags::POLLIN), @@ -582,7 +583,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio // Now that we got signalled, stop polling from the idle pipe and use a timeout // instead. poll_fds = &mut just_connection; - poll_timeout = to_timeout(idle_timeout, now); + poll_timeout = to_timeout(earliest_timeout, now); if now >= idle_timeout { break; } From 9afbbfcff7fc6ffed61e99f2d9ebd40bf7cc44bd Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sun, 19 Jul 2020 11:56:50 +0200 Subject: [PATCH 7/9] poll_with_timeout: Recompute timeout on each iteration Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 43 +++++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 8d879eb089..cd098b53bf 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -537,15 +537,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio use std::os::raw::c_int; use std::os::unix::io::AsRawFd; - fn to_timeout(timeout: Instant, now: Instant) -> c_int { - if timeout < now { - 0 - } else { - c_int::try_from(timeout.duration_since(now).as_millis()) - .unwrap_or(c_int::max_value()) - } - } - + let mut now = Instant::now(); let earliest_timeout = idle_timeout.min(timer_timeout.unwrap_or(idle_timeout)); let fd = conn.as_raw_fd(); let mut both_poll_fds = [ @@ -557,11 +549,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio // We start with no timeout in the poll call. If we get something from the idle handler, we'll // start setting one. - let mut poll_timeout = -1; - // However, we still have to honor the timer_timeout - if let Some(timeout) = timer_timeout { - poll_timeout = to_timeout(timeout, Instant::now()); - } + let mut honor_idle_timeout = false; loop { fn readable(p: PollFd) -> bool { p.revents() @@ -569,13 +557,32 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio .contains(PollFlags::POLLIN) }; + // Compute the deadline for when poll() has to wakeup + let deadline = if honor_idle_timeout { + Some(earliest_timeout) + } else { + timer_timeout + }; + // ...and convert the deadline into an argument for poll() + let poll_timeout = if let Some(deadline) = deadline { + if deadline < now { + 0 + } else { + c_int::try_from(deadline.duration_since(now).as_millis()) + .unwrap_or(c_int::max_value()) + } + } else { + // No timeout + -1 + }; + match poll(poll_fds, poll_timeout) { Ok(_) => { if readable(poll_fds[0]) { // There is an X11 event ready to be handled. break; } - let now = Instant::now(); + now = Instant::now(); if timer_timeout.is_some() && now >= timer_timeout.unwrap() { break; } @@ -583,14 +590,16 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio // Now that we got signalled, stop polling from the idle pipe and use a timeout // instead. poll_fds = &mut just_connection; - poll_timeout = to_timeout(earliest_timeout, now); + honor_idle_timeout = true; if now >= idle_timeout { break; } } } - Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} + Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => { + now = Instant::now(); + } Err(e) => return Err(e.into()), } } From 4953e9bfbbcc1822b99eb7d917b75e6c7bad722e Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Mon, 20 Jul 2020 17:04:57 +0200 Subject: [PATCH 8/9] x11: Ensure we wake up after the deadline Duration::as_millis() seems to round down. This means that a timeout of e.g. 5.5 ms results in a call to poll() with a timeout of 5 ms. Thus, we wake up too early. This results in a short spike of CPU usage where poll() is called with a timeout of 0 ms until "it is time". Fix this by adding one to the timeout that is passed to poll(). Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index cd098b53bf..dbb134934f 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -568,8 +568,11 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio if deadline < now { 0 } else { - c_int::try_from(deadline.duration_since(now).as_millis()) - .unwrap_or(c_int::max_value()) + let millis = c_int::try_from(deadline.duration_since(now).as_millis()) + .unwrap_or(c_int::max_value() - 1); + // The above .as_millis() rounds down. This means we would wake up before the + // deadline is reached. Add one to 'simulate' rounding up instead. + millis + 1 } } else { // No timeout From ca9a7b6d00dc9ff34ac98e5d50cd45cab272a63f Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Mon, 20 Jul 2020 17:09:48 +0200 Subject: [PATCH 9/9] Stop polling when the deadline expired Signed-off-by: Uli Schlachter --- druid-shell/src/platform/x11/application.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index dbb134934f..71f0a30c49 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -565,8 +565,8 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timer_timeout: Optio }; // ...and convert the deadline into an argument for poll() let poll_timeout = if let Some(deadline) = deadline { - if deadline < now { - 0 + if deadline <= now { + break; } else { let millis = c_int::try_from(deadline.duration_since(now).as_millis()) .unwrap_or(c_int::max_value() - 1);