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

Conversation

AngelicosPhosphoros
Copy link
Contributor

@AngelicosPhosphoros AngelicosPhosphoros commented Oct 13, 2024

Previous version used SetTimer with GetMessageW for waiting.
The downside of UI timers like ones created by SetTimer,
is that they may be late by up to 15-16 ms.

To fix this behaviour, I added use of high resolution timers created by CreateWaitableTimerExW with the flag CREATE_WAITABLE_TIMER_HIGH_RESOLUTION.
In my previous experience, waiting on such timers have precision of roundly 0.5 ms which is the best available on Windows at the moment.
I use MsgWaitForMultipleObjectsEx to wait simultaneously for both timer and newly arriving events.

Unfortunately, high resolution timers are available only since Windows 10 1803. However:

  1. Win 10 is already getting to the end of support, like all previous versions, so it is OK to rely on APIs introduced in it;
  2. I use dwMilliseconds parameter of MsgWaitForMultipleObjectsEx as a fallback. It should perform not worse compared to waiting for events from SetTimer.

I also refactored code to remove event dispatching from function responsible for waiting for events. This provides more clear separations of concern and avoids unnecessary duplication of dispatching logic.

After review from @rib, I also moved the waiting itself from wait_for_messages method to separate function, so it is clearly seen that wait_for_messages do 3 things: notify app that we about to wait, wait, notify that we have new events.

I have tested behaviour using a egui app with Vulkan rendering with VK_PRESENT_MODE_IMMEDIATE_KHR, and older version consistently have twice less FPS than requested (e.g. 30 FPS when limit is 60 and 60 FPS when limit is 120) while newer version works more correctly (almost always 60 FPS when limit is 60, and only 5-10 frames missing when FPS is set to 120 or more).

Fixes #1610

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@AngelicosPhosphoros
Copy link
Contributor Author

Current version in crates.io:

TestWith0.30.5.mp4

With my changes:

TestWithPatch.mp4

@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch 4 times, most recently from 124513a to c858ac7 Compare October 13, 2024 19:00
@AngelicosPhosphoros
Copy link
Contributor Author

Rebased on top of master branch.

@AngelicosPhosphoros
Copy link
Contributor Author

I may be unavailable during a week but if you have questions/comments, I would happily address them in next Saturday.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Initial review

// 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_precision_timer: Option<HANDLE>,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use OwnedHandle for this instead to simplify cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, it looks much messier as result because types used in std and in windows-sys crate are not compatible.
Btw, why use windows-sys crate instead of windows crate? windows crate does all error checking itself and has more ergonomic interface overall.

Comment on lines 248 to 266
self.wait_and_dispatch_message(None);

if let Some(code) = self.exit_code() {
break code;
}

self.dispatch_peeked_messages();

if let Some(code) = self.exit_code() {
break code;
}

self.wait_for_messages(None);
Copy link
Member

Choose a reason for hiding this comment

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

Please clarify why we've removed the exit_code() check between waiting for messages and dispatching peeked messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, it was here to handle cases when exit codes were set in callbacks to messages that called in wait_and_dispatch_message. I don't do any dispatch in new self.wait_for_messages so there is no new exit code that can appear before dispatch_peeked_messages. Also, before the loop, exit code is reset at

self.window_target.clear_exit();
so it cannot be set before first iteration of loop either.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems notable that the ordering of dispatch_peeked_ followed by wait_for_messages (instead of waiting then dispatching) here isn't consistent with the pump_app_events implementation, which seems like it could affect the order of dispatching some initial events for an app.

I believe wait_for_messages dispatches the AboutToWait event (runner.prepare_wait()) to notify the app that it's about to block and wait for input so I wonder if it might still make sense to check for a possible exit code from the app after wait_for_messages()

Comment on lines 368 to 385
fn create_high_precision_timer() -> Option<HANDLE> {
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)
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 {
PumpStatus::Continue
Some(handle)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I would rather make these freestanding functions than having them inlined into wait_for_messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, would fix tomorrow. I just kept the structure that was used before.

Comment on lines 428 to 438
{
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))
},
};
timeout = min_timeout(timeout, control_flow_timeout);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
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))
},
};
timeout = min_timeout(timeout, control_flow_timeout);
}
let timeout = {
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))
},
};
min_timeout(timeout, control_flow_timeout)
};

Slightly cleaner this way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I declare new binding let timeout or just update existing variable timeout? I would update code tomorrow.

// 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.

@filnet
Copy link
Contributor

filnet commented Oct 14, 2024

The timer resolution issue was fixed in Feb 2021 : cfbe846
It used timeapi::timeBeginPeriod and timeapi::timeEndPeriod as described here : https://randomascii.wordpress.com/2020/10/04/windows-timer-resolution-the-great-rule-change/

It was later removed in July 2023: 4208402#diff-bb05c5e1236bdaa7f268bcbbbb8cc9f378a48eb3ed480884f0c6471a9cc933edL467
It is not clear why it was removed, the commit log does not mention that change.

Just sayin...

@AngelicosPhosphoros
Copy link
Contributor Author

AngelicosPhosphoros commented Oct 14, 2024

@filnet Well, timeapi::timeBeginPeriod and timeapi::timeEndPeriod should generally NOT be used because it causes big increase of energy consumption. It makes ALL system thread scheduler run more frequently which affects all programs and cause more frequent thread rescheduling. Also, I think, it also reduces time slices that each thread receive when scheduled to execution. And it is still less precise compared to CREATE_TIMER_HIGH_RESOLUTION.

As for the issue being fixed, well, I see that it is still open. And as can be seen in attached video, currently WaitUntil just don't work correctly (e.g. getting 30 FPS when I ask for 60).

@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch 4 times, most recently from 66f208e to 6f60c87 Compare October 14, 2024 20:37
@AngelicosPhosphoros
Copy link
Contributor Author

@notgull I addressed all comments except about missing messages by MsgWaitForMultipleObjectsEx. I would look which ones can be missed myself and wait for @rib .

@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch from 6f60c87 to 33ec57a Compare October 14, 2024 22:05
@AngelicosPhosphoros
Copy link
Contributor Author

I just noticed that I used word "precision" instead of "resolution" everywhere. Fixed it now.

}
}

/// Dispatch all queued messages via `PeekMessageW`
fn dispatch_peeked_messages(&mut self) {
let runner = &self.window_target.runner_shared;

// Before we potentially exit, make sure to consistently emit an event for the wake up
runner.wakeup();

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be coupled with the implementation of wait_for_messages.

Conceptually the intention was to have a sequence like:

  1. notify the app that it's about to block/wait
  2. use OS API to block
  3. notify the app of a wake up
    ... then dispatch/handle all events that triggered the wake up

So long as dispatch_peeked_messages only gets called once immediately after wait_for_messages returns then notifying the wake up here is OK, but it feels to me like it would be more correct to notify about the wakeup immediately after MsgWaitForMultipleObjectsEx returns and avoid any risk of dispatching multiple wake ups if someone makes a change later that ends up calling dispatch_peeked_messages multiple times per wait.

@filnet
Copy link
Contributor

filnet commented Oct 15, 2024

@filnet Well, timeapi::timeBeginPeriod and timeapi::timeEndPeriod should generally NOT be used because it causes big increase of energy consumption. It makes ALL system thread scheduler run more frequently which affects all programs and cause more frequent thread rescheduling. Also, I think, it also reduces time slices that each thread receive when scheduled to execution. And it is still less precise compared to CREATE_TIMER_HIGH_RESOLUTION.

As for the issue being fixed, well, I see that it is still open. And as can be seen in attached video, currently WaitUntil just don't work correctly (e.g. getting 30 FPS when I ask for 60).

I just wanted to mention the previous fix for completeness sake... If there is a better solution then we should use it.
The issue is still open because the previous fix was removed. Which issue btw ?

EDIT: found the issue.
It dates back to 2020. The original fix was removed in 2023 so I am at a loss.
But the timeapi::timeBeginPeriod fix did fix the timer precision issue. I had similar code in my Vulkan engine and could remove it once it was introduced in winit.

@AngelicosPhosphoros
Copy link
Contributor Author

@filnet Sorry if my previous message come out as a too forceful, just I wanted to make downsides of changing timer frequency clear. If you want to read what the problems with changing frequency timer, you may read this article: https://randomascii.wordpress.com/2013/07/08/windows-timer-resolution-megawatts-wasted/

AngelicosPhosphoros added a commit to AngelicosPhosphoros/winit that referenced this pull request Oct 18, 2024
Previous version used [`SetTimer`] with `GetMessageW` for waiting.
The downside of UI timers like ones created by `SetTimer`,
is that they may be late by up to 15-16 ms.

To fix this behaviour, I added use of high resolution timers created by [`CreateWaitableTimerExW`] with the flag `CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`.
In my previous experience, waiting on such timers have precision of roundly 0.5 ms which is the best available on Windows at the moment.
I use [`MsgWaitForMultipleObjectsEx`] to wait simultaneously for both timer and newly arriving events.

Unfortunately, high resolution timers are available only since Windows 10 1803. However:

1. Win 10 is already getting to the end of support, like all previous versions, so it is OK to rely on APIs introduced in it;
2. I use `dwMilliseconds` parameter of `MsgWaitForMultipleObjectsEx` as a fallback. It should perform not worse compared to waiting for events from `SetTimer`.

I also refactored code to remove event dispatching from function responsible for waiting for events. This provides more clear separations of concern and avoids unnecessary duplication of dispatching logic.

After [review] from @rib, I also moved the waiting itself from `wait_for_messages` method to separate function, so it is clearly seen that `wait_for_messages` do 3 things: notify app that we about to wait, wait, notify that we have new events.

I have tested behaviour using a egui app with Vulkan rendering with `VK_PRESENT_MODE_IMMEDIATE_KHR`, and older version consistently have twice less FPS than requested (e.g. 30 FPS when limit is 60 and 60 FPS when limit is 120) while newer version works more correctly (almost always 60 FPS when limit is 60, and only 5-10 frames missing when FPS is set to 120 or more).

Fixes rust-windowing#1610

[`CreateWaitableTimerExW`]: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createwaitabletimerexw
[`MsgWaitForMultipleObjectsEx`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-msgwaitformultipleobjectsex
[`SetTimer`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-settimer
[review]: rust-windowing#3950 (comment)
@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch from 33ec57a to 821b50a Compare October 18, 2024 17:39
AngelicosPhosphoros added a commit to AngelicosPhosphoros/winit that referenced this pull request Oct 18, 2024
Previous version used [`SetTimer`] with `GetMessageW` for waiting.
The downside of UI timers like ones created by `SetTimer`,
is that they may be late by up to 15-16 ms.

To fix this behaviour, I added use of high resolution timers created by [`CreateWaitableTimerExW`] with the flag `CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`.
In my previous experience, waiting on such timers have precision of roundly 0.5 ms which is the best available on Windows at the moment.
I use [`MsgWaitForMultipleObjectsEx`] to wait simultaneously for both timer and newly arriving events.

Unfortunately, high resolution timers are available only since Windows 10 1803. However:

1. Win 10 is already getting to the end of support, like all previous versions, so it is OK to rely on APIs introduced in it;
2. I use `dwMilliseconds` parameter of `MsgWaitForMultipleObjectsEx` as a fallback. It should perform not worse compared to waiting for events from `SetTimer`.

I also refactored code to remove event dispatching from function responsible for waiting for events. This provides more clear separations of concern and avoids unnecessary duplication of dispatching logic.

After [review] from @rib, I also moved the waiting itself from `wait_for_messages` method to separate function, so it is clearly seen that `wait_for_messages` do 3 things: notify app that we about to wait, wait, notify that we have new events.

I have tested behaviour using a egui app with Vulkan rendering with `VK_PRESENT_MODE_IMMEDIATE_KHR`, and older version consistently have twice less FPS than requested (e.g. 30 FPS when limit is 60 and 60 FPS when limit is 120) while newer version works more correctly (almost always 60 FPS when limit is 60, and only 5-10 frames missing when FPS is set to 120 or more).

Fixes rust-windowing#1610

[`CreateWaitableTimerExW`]: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createwaitabletimerexw
[`MsgWaitForMultipleObjectsEx`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-msgwaitformultipleobjectsex
[`SetTimer`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-settimer
[review]: rust-windowing#3950 (comment)
@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch from 821b50a to cb2f4de Compare October 18, 2024 17:40
@AngelicosPhosphoros
Copy link
Contributor Author

I rebased on top of master branch and also made changes requested by at #3950 (comment)


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.

@AngelicosPhosphoros
Copy link
Contributor Author

CI failure seems to be irrelevant because it is about another platform (Redox).

AngelicosPhosphoros added a commit to AngelicosPhosphoros/winit that referenced this pull request Oct 23, 2024
Previous version used [`SetTimer`] with `GetMessageW` for waiting.
The downside of UI timers like ones created by `SetTimer`,
is that they may be late by up to 15-16 ms.

To fix this behaviour, I added use of high resolution timers created by [`CreateWaitableTimerExW`] with the flag `CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`.
In my previous experience, waiting on such timers have precision of roundly 0.5 ms which is the best available on Windows at the moment.
I use [`MsgWaitForMultipleObjectsEx`] to wait simultaneously for both timer and newly arriving events.

Unfortunately, high resolution timers are available only since Windows 10 1803. However:

1. Win 10 is already getting to the end of support, like all previous versions, so it is OK to rely on APIs introduced in it;
2. I use `dwMilliseconds` parameter of `MsgWaitForMultipleObjectsEx` as a fallback. It should perform not worse compared to waiting for events from `SetTimer`.

I also refactored code to remove event dispatching from function responsible for waiting for events. This provides more clear separations of concern and avoids unnecessary duplication of dispatching logic.

After [review] from @rib, I also moved the waiting itself from `wait_for_messages` method to separate function, so it is clearly seen that `wait_for_messages` do 3 things: notify app that we about to wait, wait, notify that we have new events.

I have tested behaviour using a egui app with Vulkan rendering with `VK_PRESENT_MODE_IMMEDIATE_KHR`, and older version consistently have twice less FPS than requested (e.g. 30 FPS when limit is 60 and 60 FPS when limit is 120) while newer version works more correctly (almost always 60 FPS when limit is 60, and only 5-10 frames missing when FPS is set to 120 or more).

Fixes rust-windowing#1610

[`CreateWaitableTimerExW`]: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createwaitabletimerexw
[`MsgWaitForMultipleObjectsEx`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-msgwaitformultipleobjectsex
[`SetTimer`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-settimer
[review]: rust-windowing#3950 (comment)
@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch from cb2f4de to d15d1fe Compare October 23, 2024 21:12
@AngelicosPhosphoros
Copy link
Contributor Author

@notgull Just reminding that I still wait for a code review.

@notgull
Copy link
Member

notgull commented Oct 27, 2024

I will review this today or tomorrow.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

LGTM aside from some minor nits

Comment on lines 673 to 677
/// Implementation detail of [EventLoop::wait_for_messages].
/// Does actual system-level waiting and doesn't process any messages itself,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Implementation detail of [EventLoop::wait_for_messages].
/// Does actual system-level waiting and doesn't process any messages itself,
/// Implementation detail of [EventLoop::wait_for_messages].
///
/// Does actual system-level waiting and doesn't process any messages itself,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 649 to 656
/// This function should not return error if parameters 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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This function should not return error if parameters 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.
/// This function should not return error if parameters 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Previous version used [`SetTimer`] with `GetMessageW` for waiting.
The downside of UI timers like ones created by `SetTimer`,
is that they may be late by up to 15-16 ms.

To fix this behaviour, I added use of high resolution timers created by [`CreateWaitableTimerExW`] with the flag `CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`.
In my previous experience, waiting on such timers have precision of roundly 0.5 ms which is the best available on Windows at the moment.
I use [`MsgWaitForMultipleObjectsEx`] to wait simultaneously for both timer and newly arriving events.

Unfortunately, high resolution timers are available only since Windows 10 1803. However:

1. Win 10 is already getting to the end of support, like all previous versions, so it is OK to rely on APIs introduced in it;
2. I use `dwMilliseconds` parameter of `MsgWaitForMultipleObjectsEx` as a fallback. It should perform not worse compared to waiting for events from `SetTimer`.

I also refactored code to remove event dispatching from function responsible for waiting for events. This provides more clear separations of concern and avoids unnecessary duplication of dispatching logic.

After [review] from @rib, I also moved the waiting itself from `wait_for_messages` method to separate function, so it is clearly seen that `wait_for_messages` do 3 things: notify app that we about to wait, wait, notify that we have new events.

I have tested behaviour using a egui app with Vulkan rendering with `VK_PRESENT_MODE_IMMEDIATE_KHR`, and older version consistently have twice less FPS than requested (e.g. 30 FPS when limit is 60 and 60 FPS when limit is 120) while newer version works more correctly (almost always 60 FPS when limit is 60, and only 5-10 frames missing when FPS is set to 120 or more).

Fixes rust-windowing#1610

[`CreateWaitableTimerExW`]: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createwaitabletimerexw
[`MsgWaitForMultipleObjectsEx`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-msgwaitformultipleobjectsex
[`SetTimer`]: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-settimer
[review]: rust-windowing#3950 (comment)
@AngelicosPhosphoros AngelicosPhosphoros force-pushed the angelicos_phosphosoros/fix_on_development_branch branch from d15d1fe to 8301d4d Compare October 28, 2024 21:27
@AngelicosPhosphoros
Copy link
Contributor Author

@notgull I rebased on top of master branch and applied proposed changes.

@notgull notgull merged commit bb4aa22 into rust-windowing:master Oct 28, 2024
58 checks passed
@kchibisov kchibisov added this to the Version 0.30.6 milestone Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ControlFlow::WaitUntil seems to not work on Windows10, causing high CPU usage
5 participants