search

rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.88k stars 911 forks source link

Improve waiting for messages on Windows #3950

Closed AngelicosPhosphoros closed 4 weeks ago

AngelicosPhosphoros commented 1 month ago

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 https://github.com/rust-windowing/winit/issues/1610

AngelicosPhosphoros commented 1 month ago

Current version in crates.io:

https://github.com/user-attachments/assets/bf52ce3e-c05d-4afa-b8c4-a20784b0e98d

With my changes:

https://github.com/user-attachments/assets/412e502d-ca59-43b8-a222-1778fa043553

AngelicosPhosphoros commented 1 month ago

Rebased on top of master branch.

AngelicosPhosphoros commented 1 month ago

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

filnet commented 1 month ago

The timer resolution issue was fixed in Feb 2021 : https://github.com/rust-windowing/winit/commit/cfbe8462ccd7d442f1dd7aa4807adf8b3e75e8e3 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: https://github.com/rust-windowing/winit/commit/420840278b5d708e34b10194aba02ce37e9ac7bf#diff-bb05c5e1236bdaa7f268bcbbbb8cc9f378a48eb3ed480884f0c6471a9cc933edL467 It is not clear why it was removed, the commit log does not mention that change.

Just sayin...

AngelicosPhosphoros commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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

filnet commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

I rebased on top of master branch and also made changes requested by at https://github.com/rust-windowing/winit/pull/3950#discussion_r1800184479

AngelicosPhosphoros commented 1 month ago

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

AngelicosPhosphoros commented 1 month ago

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

notgull commented 1 month ago

I will review this today or tomorrow.

AngelicosPhosphoros commented 4 weeks ago

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