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

windows: Prevent high freq mice from slowing down event loops #4792

Closed
wants to merge 2 commits into from

Conversation

0x0ade
Copy link
Contributor

@0x0ade 0x0ade commented Sep 28, 2021

This PR changes event polling behavior - and in particular WIN_PumpEvents - to not waste time when dealing with high frequency mice continuously generating new messages on every poll / pump on Windows.

Description

I'm going to preface this with the huge code comment that I've been writing for a while now.

/* There are as many WM_MOUSEMOVEs as we are asking for.
   Luckily there is no such thing as a "dropped" WM_MOUSEMOVE as they all get combined.
   For more info, check https://devblogs.microsoft.com/oldnewthing/20031001-00/?p=42343
   Sadly non-removed WM_MOUSEMOVEs will fill the message queue, thus remove all queued ones.

   Also worth noting: PeekMessage-ing an autogenerated mouse move will also make it directly
   call WIN_WindowProc with WM_NCHITTEST and WM_SETCURSOR as if those were *sent* (not posted) messages.
   One huge problem is that SDL_PollEvent can call SDL_PumpEvents, which then goes *through the entire msg loop*!

   High frequency mice can overwhelm the poll loop easily on some PCs, especially with the extra
   handling above happening on *every peek*! (For reference, a 1kHz mouse can already tank an i7-7700K on bad days.)
   We thus must make sure to not overwhelm poll loops and cut off the stream of mouse events,
   instead of waiting for the user to decide where to put their mouse before leaving the event loop.

   Having "start_ticks" match the loop start would also solve that, but this must keep existing peek loop setups as-is. */

Windows mouse movement events seems to work in a "get as much as you want, anything else gets combined" manner, which seems to cause problems with how most SDL applications implement their event polling and handling loops. In particular, polling currently always calls the event pump function, which on Windows goes through the entire message queue including new auto-generated mouse moves, even when there are still old events left in SDL's queue. With high frequency mice, this means that "new" mouse events just kept appearing between polls, keeping the loop stuck for as long as the mouse is being moved! The fact that Windows internally sends and handles WM_HITTEST and WM_SETCURSOR on every WM_MOUSEMOVE autogenerated by PeekMessage didn't make it perform better either.

This PR changes the following:

  • Process the Windows message queue mostly as before, but don't fully process WM_MOUSEMOVE messages immediately.
  • Handle the latest autogenerated WM_MOUSEMOVEs after all other events (which is when they should appear anyway, among a few other auto-generated messages, according to all docs I was able to find), up to N (currently 3) consecutive "mouse move only" pumps.
  • Change SDL_WaitEventTimeout (used by SDL_PollEvent) to check for old events before calling SDL_PumpEvents().

I am concerned about the following things:

  • Are there any "obvious" things which I'm missing in changing how SDL_WaitEventTimeout works? I'm a bit worried about changing something that could affect many platforms only because "it makes sense to this one Windows user."
    • Even if it's worth the change, should it be opt-in / opt-out with a hint? Sadly I'm too inexperienced in how the event pump is implemented across platforms and really need some feedback here.
  • Should MAX_MOUSEMOVE_PUMPS become a hint? What should it default to? (Realistically a single mouse move per "game update" should be enough, but this change possibly affects a very wide variety of applications, and this change still allows for mouse moves between other events.)
  • What about applications which call SDL_PumpEvents in a manner which could cause this PR to introduce negative effects? I personally can't think of any, but I'm still trying to be cautious, and would love to hear alternative solutions to handle the lack of a "poll loop start_ticks" instead of a "pump start_ticks".
  • What about elaborate hooks? And what about applications which implement their own WndProc? This change in its current form also skips / delays the execution of those for WM_MOUSEMOVE, but I figured I'd ask for feedback on the current impl before banging my head against a wall on how to make WIN_WindowProc tell WIN_PumpEvents to handle the last mouse movement with the least spaghetti involved, instead of having WIN_WindowProc process every autogenerated mouse movement.
  • Should I keep that #if 1 for future reference and / or quick testing, or should I clean it up and remove the old loop? CTRL+F-ing for #if 0 across the SDL source gives a few results after all.

Existing Issue(s)

#4376 was already marked as closed, but I've still been able to replicate noticeable performance loss on interaction before and after replacing most of my machine (including reinstalling the OS, upgrading the CPU, RAM, SSD and replacing the USB controller) with the same mouse in 1000 Hz mode and SDL built from source.

@0x0ade
Copy link
Contributor Author

0x0ade commented Sep 28, 2021

cc @cgutman I hope poking you for this is fine, given how I've seen a few semi-recent commits to SDL_events.c by you which refer to Win32-specific behavior. I'd be really happy to hear your feedback on this 😅

Comment on lines +30 to +37
typedef enum
{
SDL_MOUSE_EVENT_SOURCE_UNKNOWN,
SDL_MOUSE_EVENT_SOURCE_MOUSE,
SDL_MOUSE_EVENT_SOURCE_TOUCH,
SDL_MOUSE_EVENT_SOURCE_PEN,
} SDL_MOUSE_EVENT_SOURCE;

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'm not entirely sure if moving the enum here is the right thing to do, given how it's effectively only returned by GetMouseMessageSource in SDL_windowsevents.c. Also, is it safe to assume that driver data layout shouldn't be relied on by applications?

@0x0ade
Copy link
Contributor Author

0x0ade commented Oct 15, 2021

Given how #4794's superior SDL_PollEvent fix was just merged and how this PR doesn't batch / merge WM_INPUT (which might still need some looking into according to #4794 (comment)), I'll close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants