[PATCH v2 0/2] MR825: winex11.drv: optimize repeated ClipCursor calls
At least Doom 64 calls ClipCursor() with identical parameters repeatedly, which seems to cause a considerable overhead. Together with high polling rate mouse input this causes the game to almost freeze while the mouse is being moved. So if the clipping did not change we can bail out early because it will not cause any observable difference anyway. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46976 Signed-off-by: Jan Klötzke <jan(a)kloetzke.net> -- v2: winex11.drv: optimize repeated ClipCursor calls winex11.drv: create win32 clipping window only once https://gitlab.winehq.org/wine/wine/-/merge_requests/825
From: Jan Klötzke <jan(a)kloetzke.net> Applications might change the cursor clipping very often. Creating and destroying a win32 window every time for this is very costly. Instead, create the window only once and use a sequence number to guard against improper clipping resets that are caused by asynchronous message handling in the desktop window. Signed-off-by: Jan Klötzke <jan(a)kloetzke.net> --- dlls/winex11.drv/mouse.c | 68 +++++++++++++++++----------------- dlls/winex11.drv/window.c | 2 +- dlls/winex11.drv/x11drv.h | 4 +- dlls/winex11.drv/x11drv_main.c | 2 + 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index 96be81df6e3..1a09c578c26 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -376,7 +376,6 @@ static BOOL grab_clipping_window( const RECT *clip ) struct x11drv_thread_data *data = x11drv_thread_data(); UNICODE_STRING class_name; Window clip_window; - HWND msg_hwnd = 0; POINT pos; if (NtUserGetWindowThread( NtUserGetDesktopWindow(), NULL ) == GetCurrentThreadId()) @@ -385,11 +384,14 @@ static BOOL grab_clipping_window( const RECT *clip ) if (!data) return FALSE; if (!(clip_window = init_clip_window())) return TRUE; - RtlInitUnicodeString( &class_name, messageW ); - if (!(msg_hwnd = NtUserCreateWindowEx( 0, &class_name, &class_name, NULL, 0, 0, 0, 0, 0, - HWND_MESSAGE, 0, NtCurrentTeb()->Peb->ImageBaseAddress, - NULL, 0, NULL, 0, FALSE ))) - return TRUE; + if (!data->clip_hwnd) + { + RtlInitUnicodeString( &class_name, messageW ); + if (!(data->clip_hwnd = NtUserCreateWindowEx( 0, &class_name, &class_name, NULL, 0, 0, 0, 0, 0, + HWND_MESSAGE, 0, NtCurrentTeb()->Peb->ImageBaseAddress, + NULL, 0, NULL, 0, FALSE ))) + return TRUE; + } if (keyboard_grabbed) { @@ -405,26 +407,25 @@ static BOOL grab_clipping_window( const RECT *clip ) } /* enable XInput2 unless we are already clipping */ - if (!data->clip_hwnd) enable_xinput2(); + if (!data->clipping) enable_xinput2(); if (data->xi2_state != xi_enabled) { WARN( "XInput2 not supported, refusing to clip to %s\n", wine_dbgstr_rect(clip) ); - NtUserDestroyWindow( msg_hwnd ); NtUserClipCursor( NULL ); return TRUE; } TRACE( "clipping to %s win %lx\n", wine_dbgstr_rect(clip), clip_window ); - if (!data->clip_hwnd) XUnmapWindow( data->display, clip_window ); + if (!data->clipping) XUnmapWindow( data->display, clip_window ); pos = virtual_screen_to_root( clip->left, clip->top ); XMoveResizeWindow( data->display, clip_window, pos.x, pos.y, max( 1, clip->right - clip->left ), max( 1, clip->bottom - clip->top ) ); XMapWindow( data->display, clip_window ); /* if the rectangle is shrinking we may get a pointer warp */ - if (!data->clip_hwnd || clip->left > clip_rect.left || clip->top > clip_rect.top || + if (!data->clipping || clip->left > clip_rect.left || clip->top > clip_rect.top || clip->right < clip_rect.right || clip->bottom < clip_rect.bottom) data->warp_serial = NextRequest( data->display ); @@ -436,14 +437,15 @@ static BOOL grab_clipping_window( const RECT *clip ) if (!clipping_cursor) { disable_xinput2(); - NtUserDestroyWindow( msg_hwnd ); return FALSE; } clip_rect = *clip; - if (!data->clip_hwnd) sync_window_cursor( clip_window ); - InterlockedExchangePointer( (void **)&cursor_window, msg_hwnd ); - data->clip_hwnd = msg_hwnd; - send_notify_message( NtUserGetDesktopWindow(), WM_X11DRV_CLIP_CURSOR_NOTIFY, 0, (LPARAM)msg_hwnd ); + if (!data->clipping) sync_window_cursor( clip_window ); + InterlockedExchangePointer( (void **)&cursor_window, data->clip_hwnd ); + data->clipping = TRUE; + data->clip_seq++; + send_notify_message( NtUserGetDesktopWindow(), WM_X11DRV_CLIP_CURSOR_NOTIFY, + (WPARAM)data->clip_seq, (LPARAM)data->clip_hwnd ); return TRUE; #else WARN( "XInput2 was not available at compile time\n" ); @@ -500,34 +502,32 @@ void retry_grab_clipping_window(void) * * Notification function called upon receiving a WM_X11DRV_CLIP_CURSOR_NOTIFY. */ -LRESULT clip_cursor_notify( HWND hwnd, HWND prev_clip_hwnd, HWND new_clip_hwnd ) +LRESULT clip_cursor_notify( HWND hwnd, DWORD new_clip_seq, HWND new_clip_hwnd ) { struct x11drv_thread_data *data = x11drv_init_thread_data(); if (hwnd == NtUserGetDesktopWindow()) /* change the clip window stored in the desktop process */ { static HWND clip_hwnd; + static DWORD clip_seq; - HWND prev = clip_hwnd; + HWND prev_hwnd = clip_hwnd; + DWORD prev_seq = clip_seq; clip_hwnd = new_clip_hwnd; - if (prev || new_clip_hwnd) TRACE( "clip hwnd changed from %p to %p\n", prev, new_clip_hwnd ); - if (prev) send_notify_message( prev, WM_X11DRV_CLIP_CURSOR_NOTIFY, (WPARAM)prev, 0 ); + clip_seq = new_clip_seq; + + if (prev_hwnd != new_clip_hwnd) + { + TRACE( "clip hwnd changed from %p@%u to %p@%u\n", prev_hwnd, prev_seq, new_clip_hwnd, new_clip_seq ); + if (prev_hwnd) send_notify_message( prev_hwnd, WM_X11DRV_CLIP_CURSOR_NOTIFY, (WPARAM)prev_seq, 0 ); + } } - else if (hwnd == data->clip_hwnd) /* this is a notification that clipping has been reset */ + else if (data->clipping && data->clip_seq == new_clip_seq) /* this is a notification that clipping has been reset */ { TRACE( "clip hwnd reset from %p\n", hwnd ); - data->clip_hwnd = 0; + data->clipping = FALSE; data->clip_reset = NtGetTickCount(); disable_xinput2(); - NtUserDestroyWindow( hwnd ); - } - else if (prev_clip_hwnd) - { - /* This is a notification send by the desktop window to an old - * dangling clip window. - */ - TRACE( "destroying old clip hwnd %p\n", prev_clip_hwnd ); - NtUserDestroyWindow( prev_clip_hwnd ); } return 0; } @@ -558,7 +558,7 @@ BOOL clip_fullscreen_window( HWND hwnd, BOOL reset ) if (!fullscreen) return FALSE; if (!(thread_data = x11drv_thread_data())) return FALSE; if (NtGetTickCount() - thread_data->clip_reset < 1000) return FALSE; - if (!reset && clipping_cursor && thread_data->clip_hwnd) return FALSE; /* already clipping */ + if (!reset && clipping_cursor && thread_data->clipping) return FALSE; /* already clipping */ monitor = NtUserMonitorFromWindow( hwnd, MONITOR_DEFAULTTONEAREST ); if (!monitor) return FALSE; @@ -606,7 +606,7 @@ static void map_event_coords( HWND hwnd, Window window, Window event_root, int x if (!hwnd) { thread_data = x11drv_thread_data(); - if (!thread_data->clip_hwnd) return; + if (!thread_data->clipping) return; if (thread_data->clip_window != window) return; pt.x += clip_rect.left; pt.y += clip_rect.top; @@ -652,7 +652,7 @@ static void send_mouse_input( HWND hwnd, Window window, unsigned int state, INPU struct x11drv_thread_data *thread_data = x11drv_thread_data(); HWND clip_hwnd = thread_data->clip_hwnd; - if (!clip_hwnd) return; + if (!thread_data->clipping) return; if (thread_data->clip_window != window) return; if (InterlockedExchangePointer( (void **)&cursor_window, clip_hwnd ) != clip_hwnd || input->u.mi.time - last_cursor_change > 100) @@ -1596,7 +1596,7 @@ BOOL X11DRV_ClipCursor( LPCRECT clip ) else /* if currently clipping, check if we should switch to fullscreen clipping */ { struct x11drv_thread_data *data = x11drv_thread_data(); - if (data && data->clip_hwnd) + if (data && data->clipping) { if (EqualRect( clip, &clip_rect ) || clip_fullscreen_window( foreground, TRUE )) return TRUE; diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 70a29d24fb1..63cfb627cfe 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -3041,7 +3041,7 @@ LRESULT X11DRV_WindowMessage( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) return 0; } case WM_X11DRV_CLIP_CURSOR_NOTIFY: - return clip_cursor_notify( hwnd, (HWND)wp, (HWND)lp ); + return clip_cursor_notify( hwnd, (DWORD)wp, (HWND)lp ); case WM_X11DRV_CLIP_CURSOR_REQUEST: return clip_cursor_request( hwnd, (BOOL)wp, (BOOL)lp ); case WM_X11DRV_DELETE_TAB: diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 5e1faf8a16e..2a5a45b1643 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -375,6 +375,8 @@ struct x11drv_thread_data Window clip_window; /* window used for cursor clipping */ HWND clip_hwnd; /* message window stored in desktop while clipping is active */ DWORD clip_reset; /* time when clipping was last reset */ + DWORD clip_seq; /* clipping configuration sequence number */ + BOOL clipping; /* clipping is active */ #ifdef HAVE_X11_EXTENSIONS_XINPUT2_H enum { xi_unavailable = -1, xi_unknown, xi_disabled, xi_enabled } xi2_state; /* XInput2 state */ void *xi2_devices; /* list of XInput2 devices (valid when state is enabled) */ @@ -669,7 +671,7 @@ extern XContext cursor_context DECLSPEC_HIDDEN; extern void X11DRV_SetFocus( HWND hwnd ) DECLSPEC_HIDDEN; extern void set_window_cursor( Window window, HCURSOR handle ) DECLSPEC_HIDDEN; extern void sync_window_cursor( Window window ) DECLSPEC_HIDDEN; -extern LRESULT clip_cursor_notify( HWND hwnd, HWND prev_clip_hwnd, HWND new_clip_hwnd ) DECLSPEC_HIDDEN; +extern LRESULT clip_cursor_notify( HWND hwnd, DWORD new_clip_seq, HWND new_clip_hwnd ) DECLSPEC_HIDDEN; extern LRESULT clip_cursor_request( HWND hwnd, BOOL fullscreen, BOOL reset ) DECLSPEC_HIDDEN; extern void ungrab_clipping_window(void) DECLSPEC_HIDDEN; extern void reset_clipping_window(void) DECLSPEC_HIDDEN; diff --git a/dlls/winex11.drv/x11drv_main.c b/dlls/winex11.drv/x11drv_main.c index 497e270ee8a..9b3af044877 100644 --- a/dlls/winex11.drv/x11drv_main.c +++ b/dlls/winex11.drv/x11drv_main.c @@ -728,6 +728,7 @@ void X11DRV_ThreadDetach(void) if (data) { + HWND clip_hwnd = data->clip_hwnd; vulkan_thread_detach(); if (data->xim) XCloseIM( data->xim ); if (data->font_set) XFreeFontSet( data->display, data->font_set ); @@ -735,6 +736,7 @@ void X11DRV_ThreadDetach(void) free( data ); /* clear data in case we get re-entered from user32 before the thread is truly dead */ NtUserGetThreadInfo()->driver_data = 0; + if (clip_hwnd) NtUserDestroyWindow( clip_hwnd ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/825
From: Jan Klötzke <jan(a)kloetzke.net> At least Doom 64 calls ClipCursor() with identical parameters repeatedly, which seems to cause a considerable overhead. Together with high polling rate mouse input this causes the game to almost freeze while the mouse is being moved. So if the clipping did not change we can bail out early because it will not cause any observable difference anyway. Care must be taken if the grab was lost. In this case we must retry until we finally got the cursor grab again. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46976 Signed-off-by: Jan Klötzke <jan(a)kloetzke.net> --- dlls/winex11.drv/mouse.c | 6 ++++++ dlls/winex11.drv/x11drv.h | 1 + dlls/winex11.drv/x11drv_main.c | 1 + 3 files changed, 8 insertions(+) diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index 1a09c578c26..c259d2bf38f 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -382,6 +382,10 @@ static BOOL grab_clipping_window( const RECT *clip ) return TRUE; /* don't clip in the desktop process */ if (!data) return FALSE; + + if (data->clipping && EqualRect( clip, &clip_rect ) && !grab_retry) + return TRUE; + if (!(clip_window = init_clip_window())) return TRUE; if (!data->clip_hwnd) @@ -440,6 +444,7 @@ static BOOL grab_clipping_window( const RECT *clip ) return FALSE; } clip_rect = *clip; + grab_retry = FALSE; if (!data->clipping) sync_window_cursor( clip_window ); InterlockedExchangePointer( (void **)&cursor_window, data->clip_hwnd ); data->clipping = TRUE; @@ -491,6 +496,7 @@ void reset_clipping_window(void) */ void retry_grab_clipping_window(void) { + grab_retry = TRUE; if (clipping_cursor) NtUserClipCursor( &clip_rect ); else if (last_clip_refused && NtUserGetForegroundWindow() == last_clip_foreground_window) diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 2a5a45b1643..1d5c3b97600 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -434,6 +434,7 @@ extern BOOL use_system_cursors DECLSPEC_HIDDEN; extern BOOL show_systray DECLSPEC_HIDDEN; extern BOOL grab_pointer DECLSPEC_HIDDEN; extern BOOL grab_fullscreen DECLSPEC_HIDDEN; +extern BOOL grab_retry DECLSPEC_HIDDEN; extern BOOL usexcomposite DECLSPEC_HIDDEN; extern BOOL managed_mode DECLSPEC_HIDDEN; extern BOOL decorated_mode DECLSPEC_HIDDEN; diff --git a/dlls/winex11.drv/x11drv_main.c b/dlls/winex11.drv/x11drv_main.c index 9b3af044877..efbe8699f00 100644 --- a/dlls/winex11.drv/x11drv_main.c +++ b/dlls/winex11.drv/x11drv_main.c @@ -79,6 +79,7 @@ BOOL use_system_cursors = TRUE; BOOL show_systray = TRUE; BOOL grab_pointer = TRUE; BOOL grab_fullscreen = FALSE; +BOOL grab_retry = FALSE; BOOL managed_mode = TRUE; BOOL decorated_mode = TRUE; BOOL private_color_map = FALSE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/825
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125675 Your paranoid android. === debian11 (32 bit report) === user32: monitor.c:1613: Test failed: SetThreadDesktop failed, error 0xaa. monitor.c:1614: Test failed: Expected 00000014, got 00000348.
On Tue Sep 13 07:08:21 2022 +0000, Rémi Bernon wrote:
Indeed, the overhead does seem to come from the Win32 window creation. Interestingly the allocation of a new window every time the clipping changes, was introduced in dc801b9831f990c5b9e61da93c258404820b2b38 to "avoid stale notifications". Now I stared at all the code for almost two hours and don't yet get what notification could be outdated. Maybe this is not relevant any more? It's always hard to tell, especially with `winex11`. I will gladly refactor the code to allocate the Win32 window only once but I guess some hints could be helpful to not fall into traps that have been fixed some 11 years ago... Well, feel free to try but it probably won't be easy. I tried for Proton to move the clipping logic out of the main process, to avoid this overhead, but it caused subtle issues with some games and we reverted the changes. I believe `winex11` mouse handling should be globally refactored to avoid blocking applications threads, and that could be part of it, but such a big change will be hard to get accepted. Ok, I've created two commits that are both relevant for Doom64 to work smoothly. The first one avoids the destruction and recreation of the win32 window every time `ClipCursor` is called. But this alone is not yet sufficient. The X11 calls still add visible overhead and the 2nd change optimizes for the case that the clipping window did not change.
X11 may override cursor grab without notifying the application, you will end up with the mouse free to move when the game and Wine still believe it's constrained.
Isn't this a bug in the X11 server then? Also the current code would only hide this problem if the application would issue redundant calls to `ClipCursor`. Or do I overlook something? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/825#note_12852
Isn't this a bug in the X11 server then? Also the current code would only hide this problem if the application would issue redundant calls to `ClipCursor`. Or do I overlook something?
No, the X server is free to release the mouse whenever it likes. It's the case for instance when the WM decides to take over when you want to switch to another application. The application is in charge of retrying often enough if it wants the cursor to be grabbed. We actually probably receive a NotifyGrabbed focus event, when for instance the WM takes a keyboard grab and frees the mouse, but trying to grab it again in response will simply fail. This is also how it works on Windows and why games are calling ClipCursor repeatedly. In any case, like I said, this is touching a very sensitive and complex area, so I wouldn't get much hopes to get any changes in, here unless this gets thoroughly tested somehow. I also think the issue is part of a bigger problem we have, and that it would better be solved by moving all the logic out of the application threads, instead of adding more complexity to the mess. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/825#note_12854
On Wed Nov 2 21:41:04 2022 +0000, Rémi Bernon wrote:
Isn't this a bug in the X11 server then? Also the current code would only hide this problem if the application would issue redundant calls to `ClipCursor`. Or do I overlook something? No, the X server is free to release the mouse whenever it likes. It's the case for instance when the WM decides to take over when you want to switch to another application. The application is in charge of retrying often enough if it wants the cursor to be grabbed. We actually probably receive a NotifyGrabbed focus event, when for instance the WM takes a keyboard grab and frees the mouse, but trying to grab it again in response will simply fail. This is also how it works on Windows and why games are calling ClipCursor repeatedly. In any case, like I said, this is touching a very sensitive and complex area, so I wouldn't get much hopes to get any changes in, here unless this gets thoroughly tested somehow. I also think the issue is part of a bigger problem we have, and that it would better be solved by moving all the logic out of the application threads, instead of adding more complexity to the mess. @jkloetzke has this merge request been abandoned or superseded by another approach?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/825#note_90235
I notice that in _any_ wine application (even winecfg), the CPU usage increases from 10% to about 35% on my machine when the mouse is being moved (500Hz mouse poll); then it drops to zero when the mouse is not moving. Is this due to ClipCursor? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/825#note_90244
On Wed Dec 18 02:28:09 2024 +0000, stephematician stephematician wrote:
I notice that in _any_ wine application (even winecfg), the CPU usage increases from 10% to about 35% on my machine when the mouse is being moved (500Hz mouse poll); then it drops to zero when the mouse is not moving. Is this due to ClipCursor? I see that there was an improvement to performance, here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3072
However, I was still seeing slow-downs as recently as wine-staging 9.22 (particularly in Horizon Zero Dawn) - which _should_ have included that improvement. In 10.0-rc2, I'm getting much better performance (no frame rate drops) - although still something of an increase in CPU usage when the mouse is moving (versus not). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/825#note_90776
participants (4)
-
Jan Klötzke -
Marvin -
Rémi Bernon
-
stephematician stephematician (＠stephematician)