[PATCH 0/1] MR8802: win32u: Avoid calling server in NtUserGetKeyState() when input keystate is in sync.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/win32u/input.c | 19 ++++++++++++++++--- server/protocol.def | 2 ++ server/queue.c | 6 ++++++ server/winstation.c | 1 + 4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index c7ba334f1c0..c4b0313c72e 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -1019,17 +1019,30 @@ SHORT WINAPI NtUserGetKeyState( INT vkey ) { struct object_lock lock = OBJECT_LOCK_INIT; const input_shm_t *input_shm; - BOOL ret = FALSE; + UINT64 keystate_serial = 0; + int input_state = 0; SHORT retval = 0; NTSTATUS status;
while ((status = get_shared_input( GetCurrentThreadId(), &lock, &input_shm )) == STATUS_PENDING) { - ret = !!input_shm->keystate_lock; /* needs a request for sync_input_keystate */ + /* when input is not locked needs a request for sync_input_keystate if desktop input state + * changed. */ + input_state = input_shm->keystate_lock ? 1 : -1; + keystate_serial = input_shm->desktop_keystate_serial; retval = (signed char)(input_shm->keystate[vkey & 0xff] & 0x81); }
- if (!ret) SERVER_START_REQ( get_key_state ) + if (input_state < 0) + { + struct object_lock lock = OBJECT_LOCK_INIT; + const desktop_shm_t *desktop_shm; + + while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) + input_state = (keystate_serial == desktop_shm->keystate_serial); + } + + if (input_state != 1) SERVER_START_REQ( get_key_state ) { req->key = vkey; if (!wine_server_call( req )) retval = (signed char)(reply->state & 0x81); diff --git a/server/protocol.def b/server/protocol.def index ecc20a8e2d0..c7614f96817 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -985,6 +985,7 @@ typedef volatile struct struct shared_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ unsigned __int64 monitor_serial; /* winstation monitor update counter */ + unsigned __int64 keystate_serial; /* keystate update counter */ } desktop_shm_t;
typedef volatile struct @@ -1010,6 +1011,7 @@ typedef volatile struct int cursor_count; /* cursor show count */ unsigned char keystate[256]; /* key state */ int keystate_lock; /* keystate is locked */ + unsigned __int64 desktop_keystate_serial; /* desktop keystate update counter at last sync */ } input_shm_t;
typedef volatile struct diff --git a/server/queue.c b/server/queue.c index f42d8f57446..78b9ce1162c 100644 --- a/server/queue.c +++ b/server/queue.c @@ -283,6 +283,7 @@ static struct thread_input *create_thread_input( struct thread *thread ) shared->cursor_count = 0; memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); shared->keystate_lock = 0; + shared->desktop_keystate_serial = 0; } SHARED_WRITE_END; } @@ -379,6 +380,7 @@ static void sync_input_keystate( struct thread_input *input ) if (input->desktop_keystate[i] == desktop_shm->keystate[i]) continue; shared->keystate[i] = input->desktop_keystate[i] = desktop_shm->keystate[i]; } + shared->desktop_keystate_serial = desktop_shm->keystate_serial; } SHARED_WRITE_END; } @@ -1525,6 +1527,7 @@ int attach_thread_input( struct thread *thread_from, struct thread *thread_to ) SHARED_WRITE_BEGIN( input_shm, input_shm_t ) { memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); + shared->desktop_keystate_serial = 0; } SHARED_WRITE_END; } @@ -1790,6 +1793,7 @@ static void update_desktop_key_state( struct desktop *desktop, unsigned int msg, SHARED_WRITE_BEGIN( desktop->shared, desktop_shm_t ) { update_key_state( shared->keystate, msg, wparam, 1 ); + ++shared->keystate_serial; } SHARED_WRITE_END; } @@ -3801,6 +3805,7 @@ DECL_HANDLER(get_key_state) { reply->state = shared->keystate[req->key & 0xff]; shared->keystate[req->key & 0xff] &= ~0x40; + ++shared->keystate_serial; } SHARED_WRITE_END; release_object( desktop ); @@ -3838,6 +3843,7 @@ DECL_HANDLER(set_key_state) SHARED_WRITE_BEGIN( desktop->shared, desktop_shm_t ) { memcpy( (void *)shared->keystate, get_req_data(), size ); + ++shared->keystate_serial; } SHARED_WRITE_END; release_object( desktop ); diff --git a/server/winstation.c b/server/winstation.c index bb5596f9a03..bfcd4a53613 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -325,6 +325,7 @@ static struct desktop *create_desktop( const struct unicode_str *name, unsigned shared->cursor.clip.right = 0; shared->cursor.clip.bottom = 0; memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); + shared->keystate_serial = 1; shared->monitor_serial = winstation->monitor_serial; } SHARED_WRITE_END;
Currently the server call is only avoided if keystate is locked (that is, if there are any related messages in the message queue). When the input is not locked GetKeyState() is supposed to sync thread input keystate with desktop and for that reason the call is performed.
The vast majority of time there are no such messages in queue and the thread input is the same as desktop input. Still GetKeyState is called the input is not locked essentially avoiding any optimization.
Tracking desktop input change serial number allows the client to detect if desktop keystate may be different from the thread input and call the server for input sync only when needed, which removes the majority of the server calls from GetKeyState().
Rémi Bernon (@rbernon) commented about dlls/win32u/input.c:
input_state = input_shm->keystate_lock ? 1 : -1;keystate_serial = input_shm->desktop_keystate_serial; retval = (signed char)(input_shm->keystate[vkey & 0xff] & 0x81);
- if (!ret) SERVER_START_REQ( get_key_state )
- if (input_state < 0)
- {
struct object_lock lock = OBJECT_LOCK_INIT;const desktop_shm_t *desktop_shm;while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING)input_state = (keystate_serial == desktop_shm->keystate_serial);- }
- if (input_state != 1) SERVER_START_REQ( get_key_state )
Why change the BOOL to an int when it only has two possible values after all and is later set with a boolean value? What about this instead?
```suggestion:-16+0 ret = !!input_shm->keystate_lock; /* needs a request for sync_input_keystate */ keystate_serial = input_shm->keystate_serial; retval = (signed char)(input_shm->keystate[vkey & 0xff] & 0x81); }
if (!ret) /* check if keystates are synced already and we can safely skip the request */ { struct object_lock lock = OBJECT_LOCK_INIT; const desktop_shm_t *desktop_shm;
while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) ret = keystate_serial == desktop_shm->keystate_serial; }
if (!ret) SERVER_START_REQ( get_key_state ) ```
Rémi Bernon (@rbernon) commented about server/protocol.def:
struct shared_cursor cursor; /* global cursor information */ unsigned char keystate[256]; /* asynchronous key state */ unsigned __int64 monitor_serial; /* winstation monitor update counter */
- unsigned __int64 keystate_serial; /* keystate update counter */
```suggestion:-2+0 unsigned char keystate[256]; /* asynchronous key state */ unsigned __int64 keystate_serial; /* keystate update counter */ unsigned __int64 monitor_serial; /* winstation monitor update counter */ ```
Rémi Bernon (@rbernon) commented about server/protocol.def:
int cursor_count; /* cursor show count */ unsigned char keystate[256]; /* key state */ int keystate_lock; /* keystate is locked */
- unsigned __int64 desktop_keystate_serial; /* desktop keystate update counter at last sync */
I don't see a reason to prefix it with "desktop_", lets name it like the keystate field which is mirrored from desktop shm.
```suggestion:-2+0 unsigned char keystate[256]; /* key state */ unsigned __int64 keystate_serial; /* keystate update counter */ int keystate_lock; /* keystate is locked */ ```
Rémi Bernon (@rbernon) commented about server/queue.c:
shared->cursor_count = 0; memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); shared->keystate_lock = 0;
shared->desktop_keystate_serial = 0;
This could be initialized to 1, or the desktop serial could be initialized to 0, either way there's no reason to make their initial values different.
```suggestion:-2+0 memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); shared->keystate_serial = 0; shared->keystate_lock = 0; ```
Rémi Bernon (@rbernon) commented about server/winstation.c:
shared->cursor.clip.right = 0; shared->cursor.clip.bottom = 0; memset( (void *)shared->keystate, 0, sizeof(shared->keystate) );
shared->keystate_serial = 1;
```suggestion:-0+0 shared->keystate_serial = 0; ```
On Thu Aug 21 08:49:31 2025 +0000, Rémi Bernon wrote:
Why change the BOOL to an int when it only has two possible values after all and is later set with a boolean value? What about this instead?
ret = !!input_shm->keystate_lock; /* needs a request for sync_input_keystate */ keystate_serial = input_shm->keystate_serial; retval = (signed char)(input_shm->keystate[vkey & 0xff] & 0x81); } if (!ret) /* check if keystates are synced already and we can safely skip the request */ { struct object_lock lock = OBJECT_LOCK_INIT; const desktop_shm_t *desktop_shm; while ((status = get_shared_desktop( &lock, &desktop_shm )) == STATUS_PENDING) ret = keystate_serial == desktop_shm->keystate_serial; } if (!ret) SERVER_START_REQ( get_key_state )
The existing code de-facto falls back to server call always if there was an error accessing shared memory. I thought if that can happen this should fallback to server call and not to try to rely on any serial checks as `retval` is wrong already. Relying on serial comparison is possible (if changing default value) but seems not straightforward. Is it not the case?
On Thu Aug 21 08:49:32 2025 +0000, Rémi Bernon wrote:
This could be initialized to 1, or the desktop serial could be initialized to 0, either way there's no reason to make their initial values different.
memset( (void *)shared->keystate, 0, sizeof(shared->keystate) ); shared->keystate_serial = 0; shared->keystate_lock = 0;
I put 0 here (different from desktop possible serial) intentionally because in this place keystate is memset to 0 and not copied from desktop keystate. I guess the logic should be that if the keystate in not synced and not locked the serial should be the same (even if for clarity, if the keystate is synced later after this initialization). Or should we maybe copy the keystate from desktop here instead of memset?
On Thu Aug 21 16:06:57 2025 +0000, Paul Gofman wrote:
The existing code de-facto falls back to server call always if there was an error accessing shared memory. I thought if that can happen this should fallback to server call and not to try to rely on any serial checks as `retval` is wrong already. Relying on serial comparison is possible (if changing default value) but seems not straightforward. Is it not the case?
I don't think we need to worry about shared memory access errors, it's unlikely to happen and if it does then something is going very wrong anyway.
On Thu Aug 21 16:06:57 2025 +0000, Paul Gofman wrote:
I put 0 here (different from desktop possible serial) intentionally because in this place keystate is memset to 0 and not copied from desktop keystate. I guess the logic should be that if the keystate in not synced and not locked the serial should be the same (even if for clarity, if the keystate is synced later after this initialization). Or should we maybe copy the keystate from desktop here instead of memset?
Well serial = 0 would always matches the initial empty keystate, it doesn't really matter if it was actually synced or not.
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon (＠rbernon)