Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve waiting for messages on Windows #3950

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ windows-sys = { version = "0.52.0", features = [
"Win32_System_Com",
"Win32_System_LibraryLoader",
"Win32_System_Ole",
"Win32_Security",
"Win32_System_SystemInformation",
"Win32_System_SystemServices",
"Win32_System_Threading",
Expand Down
1 change: 1 addition & 0 deletions src/changelog/unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,4 @@ changelog entry.
default activation policy, unless explicitly provided during initialization.
- On macOS, fix crash when calling `drag_window()` without a left click present.
- On X11, key events forward to IME anyway, even when it's disabled.
- On Windows, make `ControlFlow::WaitUntil` work more precisely using `CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`.
283 changes: 181 additions & 102 deletions src/platform_impl/windows/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mod runner;

use std::cell::Cell;
use std::ffi::c_void;
use std::os::windows::io::{AsRawHandle as _, FromRawHandle as _, OwnedHandle, RawHandle};
use std::rc::Rc;
use std::sync::atomic::{AtomicU32, Ordering};
use std::sync::{Arc, Mutex, MutexGuard};
Expand All @@ -12,13 +13,18 @@ use std::{mem, panic, ptr};

use runner::EventLoopRunner;
use windows_sys::Win32::Devices::HumanInterfaceDevice::MOUSE_MOVE_RELATIVE;
use windows_sys::Win32::Foundation::{HWND, LPARAM, LRESULT, POINT, RECT, WPARAM};
use windows_sys::Win32::Foundation::{
GetLastError, FALSE, HANDLE, HWND, LPARAM, LRESULT, POINT, RECT, WAIT_FAILED, WPARAM,
};
use windows_sys::Win32::Graphics::Gdi::{
GetMonitorInfoW, MonitorFromRect, MonitorFromWindow, RedrawWindow, ScreenToClient,
ValidateRect, MONITORINFO, MONITOR_DEFAULTTONULL, RDW_INTERNALPAINT, SC_SCREENSAVE,
};
use windows_sys::Win32::System::Ole::RevokeDragDrop;
use windows_sys::Win32::System::Threading::{GetCurrentThreadId, INFINITE};
use windows_sys::Win32::System::Threading::{
CreateWaitableTimerExW, GetCurrentThreadId, SetWaitableTimer,
CREATE_WAITABLE_TIMER_HIGH_RESOLUTION, INFINITE, TIMER_ALL_ACCESS,
};
use windows_sys::Win32::UI::Controls::{HOVER_DEFAULT, WM_MOUSELEAVE};
use windows_sys::Win32::UI::Input::Ime::{GCS_COMPSTR, GCS_RESULTSTR, ISC_SHOWUICOMPOSITIONWINDOW};
use windows_sys::Win32::UI::Input::KeyboardAndMouse::{
Expand All @@ -34,14 +40,15 @@ use windows_sys::Win32::UI::Input::Touch::{
use windows_sys::Win32::UI::Input::{RAWINPUT, RIM_TYPEKEYBOARD, RIM_TYPEMOUSE};
use windows_sys::Win32::UI::WindowsAndMessaging::{
CreateWindowExW, DefWindowProcW, DestroyWindow, DispatchMessageW, GetClientRect, GetCursorPos,
GetMenu, GetMessageW, KillTimer, LoadCursorW, PeekMessageW, PostMessageW, RegisterClassExW,
RegisterWindowMessageA, SetCursor, SetTimer, SetWindowPos, TranslateMessage, CREATESTRUCTW,
GWL_STYLE, GWL_USERDATA, HTCAPTION, HTCLIENT, MINMAXINFO, MNC_CLOSE, MSG, NCCALCSIZE_PARAMS,
PM_REMOVE, PT_TOUCH, RI_MOUSE_HWHEEL, RI_MOUSE_WHEEL, SC_MINIMIZE, SC_RESTORE, SIZE_MAXIMIZED,
SWP_NOACTIVATE, SWP_NOMOVE, SWP_NOSIZE, SWP_NOZORDER, WHEEL_DELTA, WINDOWPOS, WMSZ_BOTTOM,
WMSZ_BOTTOMLEFT, WMSZ_BOTTOMRIGHT, WMSZ_LEFT, WMSZ_RIGHT, WMSZ_TOP, WMSZ_TOPLEFT,
WMSZ_TOPRIGHT, WM_CAPTURECHANGED, WM_CLOSE, WM_CREATE, WM_DESTROY, WM_DPICHANGED,
WM_ENTERSIZEMOVE, WM_EXITSIZEMOVE, WM_GETMINMAXINFO, WM_IME_COMPOSITION, WM_IME_ENDCOMPOSITION,
GetMenu, LoadCursorW, MsgWaitForMultipleObjectsEx, PeekMessageW, PostMessageW,
RegisterClassExW, RegisterWindowMessageA, SetCursor, SetWindowPos, TranslateMessage,
CREATESTRUCTW, GWL_STYLE, GWL_USERDATA, HTCAPTION, HTCLIENT, MINMAXINFO, MNC_CLOSE, MSG,
MWMO_INPUTAVAILABLE, NCCALCSIZE_PARAMS, PM_REMOVE, PT_TOUCH, QS_ALLEVENTS, RI_MOUSE_HWHEEL,
RI_MOUSE_WHEEL, SC_MINIMIZE, SC_RESTORE, SIZE_MAXIMIZED, SWP_NOACTIVATE, SWP_NOMOVE,
SWP_NOSIZE, SWP_NOZORDER, WHEEL_DELTA, WINDOWPOS, WMSZ_BOTTOM, WMSZ_BOTTOMLEFT,
WMSZ_BOTTOMRIGHT, WMSZ_LEFT, WMSZ_RIGHT, WMSZ_TOP, WMSZ_TOPLEFT, WMSZ_TOPRIGHT,
WM_CAPTURECHANGED, WM_CLOSE, WM_CREATE, WM_DESTROY, WM_DPICHANGED, WM_ENTERSIZEMOVE,
WM_EXITSIZEMOVE, WM_GETMINMAXINFO, WM_IME_COMPOSITION, WM_IME_ENDCOMPOSITION,
WM_IME_SETCONTEXT, WM_IME_STARTCOMPOSITION, WM_INPUT, WM_KEYDOWN, WM_KEYUP, WM_KILLFOCUS,
WM_LBUTTONDOWN, WM_LBUTTONUP, WM_MBUTTONDOWN, WM_MBUTTONUP, WM_MENUCHAR, WM_MOUSEHWHEEL,
WM_MOUSEMOVE, WM_MOUSEWHEEL, WM_NCACTIVATE, WM_NCCALCSIZE, WM_NCCREATE, WM_NCDESTROY,
Expand Down Expand Up @@ -127,6 +134,10 @@ pub(crate) enum ProcResult {
pub struct EventLoop {
window_target: ActiveEventLoop,
msg_hook: Option<Box<dyn FnMut(*const c_void) -> bool + 'static>>,
// It is a timer used on timed waits.
// It is created lazily in case if we have `ControlFlow::WaitUntil`.
// Keep it as a field to avoid recreating it on every `ControlFlow::WaitUntil`.
high_resolution_timer: Option<OwnedHandle>,
}

pub(crate) struct PlatformSpecificEventLoopAttributes {
Expand Down Expand Up @@ -201,6 +212,7 @@ impl EventLoop {
Ok(EventLoop {
window_target: ActiveEventLoop { thread_id, thread_msg_target, runner_shared },
msg_hook: attributes.msg_hook.take(),
high_resolution_timer: None,
})
}

Expand Down Expand Up @@ -245,8 +257,9 @@ impl EventLoop {
}

let exit_code = loop {
self.wait_and_dispatch_message(None);

self.wait_for_messages(None);
// wait_for_messages calls user application before and after waiting
// so it may have decided to exit.
if let Some(code) = self.exit_code() {
break code;
}
Expand Down Expand Up @@ -312,8 +325,11 @@ impl EventLoop {
}
}

self.wait_and_dispatch_message(timeout);

if self.exit_code().is_none() {
self.wait_for_messages(timeout);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concern here. Right now, it looks like this sequence happens here:

  1. Call to runner.wakeup().
  2. Call to wait_for_messages
  3. Inside wait_for_messages immeadiately calls runner.prepare_wait().
  4. Waits for events.
  5. Call runner.wakeup()
  6. wait_for_messages finishes
  7. dispatch events
  8. Call runner.prepare_wait()

My concern that there is no check if there are any new events between first wakeup and first prepare_wait. Maybe we should do sequence of wakeup. dispatch, wait_for_messages, dispatch, prepare_wait?

@rib I tag you because you previously asked to change calls to wakeup and prepare_wait.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry that I don't have much time to follow the details here.

If we say that prepare_wait and wakeup are conceptually supposed to surround the "OS wait point" then it looks like we currently have two competing definitions of what that OS wait point should be...

It looks pump_app_events is intending to define the relinquish of control to the application loop (via PumpStatus::Continue) as the point where the Winit event loop will wait.

This arguably makes sense because pump_app_events is supposed to be non-blocking and any application using pump_app_events in their own loop should do something to throttle that loop before repeated called to pump_app_events (so applications using pump_app_events are arguably responsible for the wait, and they would also be expected to call pump_app_events after they wake up).

Sorry if I confused matters by overlooking these details when first commenting about changes here.

It looks like this needs to pick one definition.

Either:

  1. Define the wait point by the act of relinquishing control to the app via ::Continue and have some argument for wait_for_messages() that tells that function to skip over prepare_wait and wakeup when used from here.
  2. Define the wait point as the call to wait_for_messages_impl and instead remove the call to .wakeup() at the start of pump_app_events and remove the call to prepare_wait just before we return with a ::Continue status.

I guess option 1 would be most consistent with the current semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, maybe we should fix it in another PR? This PR doesn't change behaviour at this point and it is focused on fixing waiting precision.

// wait_for_messages calls user application before and after waiting
// so it may have decided to exit.
if self.exit_code().is_none() {
self.dispatch_peeked_messages();
}
Expand Down Expand Up @@ -343,101 +359,27 @@ impl EventLoop {
status
}

/// Wait for one message and dispatch it, optionally with a timeout
fn wait_and_dispatch_message(&mut self, timeout: Option<Duration>) {
fn get_msg_with_timeout(msg: &mut MSG, timeout: Option<Duration>) -> PumpStatus {
unsafe {
// A timeout of None means wait indefinitely (so we don't need to call SetTimer)
let timer_id = timeout.map(|timeout| SetTimer(0, 0, dur2timeout(timeout), None));
let get_status = GetMessageW(msg, 0, 0, 0);
if let Some(timer_id) = timer_id {
KillTimer(0, timer_id);
}
// A return value of 0 implies `WM_QUIT`
if get_status == 0 {
PumpStatus::Exit(0)
} else {
PumpStatus::Continue
}
}
}

/// Fetch the next MSG either via PeekMessage or GetMessage depending on whether the
/// requested timeout is `ZERO` (and so we don't want to block)
///
/// Returns `None` if no MSG was read, else a `Continue` or `Exit` status
fn wait_for_msg(msg: &mut MSG, timeout: Option<Duration>) -> Option<PumpStatus> {
if timeout == Some(Duration::ZERO) {
unsafe {
if PeekMessageW(msg, 0, 0, 0, PM_REMOVE) != 0 {
Some(PumpStatus::Continue)
} else {
None
}
}
} else {
Some(get_msg_with_timeout(msg, timeout))
}
}

/// Waits until new event messages arrive to be peeked.
/// Doesn't peek messages itself.
///
/// Parameter timeout is optional. This method would wait for the smaller timeout
/// between the argument and a timeout from control flow.
fn wait_for_messages(&mut self, timeout: Option<Duration>) {
let runner = &self.window_target.runner_shared;

// We aim to be consistent with the MacOS backend which has a RunLoop
// observer that will dispatch AboutToWait when about to wait for
// events, and NewEvents after the RunLoop wakes up.
//
// We emulate similar behaviour by treating `GetMessage` as our wait
// We emulate similar behaviour by treating `MsgWaitForMultipleObjectsEx` as our wait
// point and wake up point (when it returns) and we drain all other
// pending messages via `PeekMessage` until we come back to "wait" via
// `GetMessage`
// `MsgWaitForMultipleObjectsEx`.
//
runner.prepare_wait();

let control_flow_timeout = match runner.control_flow() {
ControlFlow::Wait => None,
ControlFlow::Poll => Some(Duration::ZERO),
ControlFlow::WaitUntil(wait_deadline) => {
let start = Instant::now();
Some(wait_deadline.saturating_duration_since(start))
},
};
let timeout = min_timeout(control_flow_timeout, timeout);

// # Safety
// The Windows API has no documented requirement for bitwise
// initializing a `MSG` struct (it can be uninitialized memory for the C
// API) and there's no API to construct or initialize a `MSG`. This
// is the simplest way avoid uninitialized memory in Rust
let mut msg = unsafe { mem::zeroed() };
let msg_status = wait_for_msg(&mut msg, timeout);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we stop using wait_for_msg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I rewrote code to use MsgWaitForMultipleObjectsEx instead of GetMessageW. MsgWaitForMultipleObjectsEx only wait until a new message is available but doesn't take it and doesn't remove it from queue, while GetMessageW does wait and take it.

In new version, wait_for_messages only waits until message is available, then dispatch_peeked_messages takes them from queue and dispatches, until the queue is empty.

So, as I understand, old version of code needed to manage both waiting and dispatching messages so it was split it into subfunctions. New version of code only does waiting while all dispatching done by dispatch_peeked_messages so there is no need to create a subfunction which just repeats the name of outer function wait_for_messages.

Also, after I move inner functions out of wait_for_messages, as you requested in #3950 (comment) , wait_for_messages would be tiny and splitting it further would be pointless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are some messages that MsgWaitForMultipleObjectsEx does not receive, which is why we used the timer system and GetMessageW here.

cc @rib you were the one to set up this system

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really recall there being a particular issue with MsgWaitForMultipleObjectsEx and at the time I suppose it just seemed simpler to use GetMessage while we were only waiting for messages and not waiting on any object handles.

The main concern I had when last iterating on this code was to do with the indirection of a dedicated waiter thread and there were some funky details to do with redraw handling I don't really recall.

Ref: fae4cbd and #2767

This change to use hrtimers seems good to me, and can see that it makes sense to use MsgWaitForMultipleObjectsEx to integrate the timer handle.

It seems to make sense that the waiting is now decoupled, since MsgWaitForMultipleObjectsEx doesn't take any data when it has something to wake up for - unlike GetMessageW which would wait and take.


wait_for_messages_impl(&mut self.high_resolution_timer, runner.control_flow(), timeout);
// Before we potentially exit, make sure to consistently emit an event for the wake up
runner.wakeup();

match msg_status {
None => {}, // No MSG to dispatch
Some(PumpStatus::Exit(code)) => {
runner.set_exit_code(code);
},
Some(PumpStatus::Continue) => {
unsafe {
let handled = if let Some(callback) = self.msg_hook.as_deref_mut() {
callback(&mut msg as *mut _ as *mut _)
} else {
false
};
if !handled {
TranslateMessage(&msg);
DispatchMessageW(&msg);
}
}

if let Err(payload) = runner.take_panic_error() {
runner.reset_runner();
panic::resume_unwind(payload);
}
},
}
}

/// Dispatch all queued messages via `PeekMessageW`
Expand All @@ -456,7 +398,7 @@ impl EventLoop {
// initializing a `MSG` struct (it can be uninitialized memory for the C
// API) and there's no API to construct or initialize a `MSG`. This
// is the simplest way avoid uninitialized memory in Rust
let mut msg = unsafe { mem::zeroed() };
let mut msg: MSG = unsafe { mem::zeroed() };

loop {
unsafe {
Expand Down Expand Up @@ -495,6 +437,14 @@ impl EventLoop {
}
}

impl Drop for EventLoop {
fn drop(&mut self) {
unsafe {
DestroyWindow(self.window_target.thread_msg_target);
}
}
}

impl ActiveEventLoop {
#[inline(always)]
pub(crate) fn create_thread_executor(&self) -> EventLoopThreadExecutor {
Expand Down Expand Up @@ -668,10 +618,139 @@ fn dur2timeout(dur: Duration) -> u32 {
.unwrap_or(INFINITE)
}

impl Drop for EventLoop {
fn drop(&mut self) {
unsafe {
DestroyWindow(self.window_target.thread_msg_target);
/// Set upper limit for waiting time to avoid overflows.
/// I chose 50 days as a limit because it is used in dur2timeout.
const FIFTY_DAYS: Duration = Duration::from_secs(50_u64 * 24 * 60 * 60);
/// Waitable timers use 100 ns intervals to indicate due time.
/// <https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-setwaitabletimer#parameters>
/// And there is no point waiting using other ways for such small timings
/// because they are even less precise (can overshoot by few ms).
const MIN_WAIT: Duration = Duration::from_nanos(100);

fn create_high_resolution_timer() -> Option<OwnedHandle> {
unsafe {
let handle: HANDLE = CreateWaitableTimerExW(
ptr::null(),
ptr::null(),
CREATE_WAITABLE_TIMER_HIGH_RESOLUTION,
TIMER_ALL_ACCESS,
);
// CREATE_WAITABLE_TIMER_HIGH_RESOLUTION is supported only after
// Win10 1803 but it is already default option for rustc
// (std uses it to implement `std::thread::sleep`).
if handle == 0 {
None
} else {
Some(OwnedHandle::from_raw_handle(handle as *mut c_void))
}
}
}

/// This function should not return error if parameters are valid
/// but there is no guarantee about that at MSDN docs
/// so we return result of GetLastError if fail.
///
/// ## Safety
///
/// timer must be a valid timer handle created by [create_high_resolution_timer].
/// timeout divided by 100 nanoseconds must be more than 0 and less than i64::MAX.
unsafe fn set_high_resolution_timer(timer: RawHandle, timeout: Duration) -> Result<(), u32> {
const INTERVAL_NS: u32 = MIN_WAIT.subsec_nanos();
const INTERVALS_IN_SEC: u64 = (Duration::from_secs(1).as_nanos() / INTERVAL_NS as u128) as u64;
let intervals_to_wait: u64 =
timeout.as_secs() * INTERVALS_IN_SEC + u64::from(timeout.subsec_nanos() / INTERVAL_NS);
debug_assert!(intervals_to_wait < i64::MAX as u64, "Must be called with smaller duration",);
// Use negative time to indicate relative time.
let due_time: i64 = -(intervals_to_wait as i64);
unsafe {
let set_result = SetWaitableTimer(timer as HANDLE, &due_time, 0, None, ptr::null(), FALSE);
if set_result != FALSE {
Ok(())
} else {
Err(GetLastError())
}
}
}

/// Implementation detail of [EventLoop::wait_for_messages].
///
/// Does actual system-level waiting and doesn't process any messages itself,
/// including winits internal notifications about waiting and new messages arrival.
fn wait_for_messages_impl(
high_resolution_timer: &mut Option<OwnedHandle>,
control_flow: ControlFlow,
timeout: Option<Duration>,
) {
let timeout = {
let control_flow_timeout = match control_flow {
ControlFlow::Wait => None,
ControlFlow::Poll => Some(Duration::ZERO),
ControlFlow::WaitUntil(wait_deadline) => {
let start = Instant::now();
Some(wait_deadline.saturating_duration_since(start))
},
};
let timeout = min_timeout(timeout, control_flow_timeout);
if timeout == Some(Duration::ZERO) {
// Do not wait if we don't have time.
return;
}
// Now we decided to wait so need to do some clamping
// to avoid problems with overflow and calling WinAPI with invalid parameters.
timeout
.map(|t| t.min(FIFTY_DAYS))
// If timeout is less than minimally supported by Windows,
// increase it to that minimum. Who want less than microsecond delays anyway?
.map(|t| t.max(MIN_WAIT))
};

if timeout.is_some() && high_resolution_timer.is_none() {
*high_resolution_timer = create_high_resolution_timer();
}

let high_resolution_timer: Option<RawHandle> =
high_resolution_timer.as_ref().map(OwnedHandle::as_raw_handle);

let use_timer: bool;
if let (Some(handle), Some(timeout)) = (high_resolution_timer, timeout) {
let res = unsafe {
// Safety: handle can be Some only if we succeeded in creating high resolution
// timer. We properly clamped timeout so it can be used as argument
// to timer.
set_high_resolution_timer(handle, timeout)
};
if let Err(error_code) = res {
// We successfully got timer but failed to set it?
// Should be some bug in our code.
tracing::trace!("Failed to set high resolution timer: last error {}", error_code);
use_timer = false;
} else {
use_timer = true;
}
} else {
use_timer = false;
}

unsafe {
// Either:
// 1. User wants to wait indefinely if timeout is not set.
// 2. We failed to get and set high resolution timer and we need something instead of it.
let wait_duration_ms = timeout.map(dur2timeout).unwrap_or(INFINITE);

let (num_handles, raw_handles) =
if use_timer { (1, [high_resolution_timer.unwrap()]) } else { (0, [ptr::null_mut()]) };

let result = MsgWaitForMultipleObjectsEx(
num_handles,
raw_handles.as_ptr() as *const _,
wait_duration_ms,
QS_ALLEVENTS,
MWMO_INPUTAVAILABLE,
);
if result == WAIT_FAILED {
// Well, nothing smart to do in such case.
// Treat it as spurious wake up.
tracing::warn!("Failed to MsgWaitForMultipleObjectsEx: error code {}", GetLastError(),);
}
}
}
Expand Down