[PATCH v9 0/4] MR852: ntdll: Speed up NtQueryVirtualMemory(MemoryWorkingSetExInformation) by conditional pread.
Apex Legends game periodically (every 30 seconds) calls this function with up to 22k virtual addresses. All but 1 of them is valid. Due to amount of queries addresses, and cost of seek+read, this causes this function to take up to about 50ms. So framerate drops from ~150 FPS to 20FPS for about a second. As far as I can see, returning 0 entries from this function, still makes Apex Legend work. But keep code correct, and optimise it by: 1. Opening pagemap file once, and never closing it. This eliminates repeated fopen/fseek/fread/fclose sequences, which helps even in queries with small amount of virtual addresses. 2. Using pread, instead of seek+read. 3. Only performing pagemap read when the address is valid. Future work might recognize continues pages in the query, and perform a batch read of multiple pagemap entries, instead one page at a time, but for now it is not necassary. This change get_working_set_ex peek wall clock runtime from 57ms to 0.29ms. Tested on Linux, but similar change done for the BSD part. `Signed-off-by: Witold Baryluk <witold.baryluk(a)gmail.com>` -- v9: ntdll: Keep pagemap file open after first use of NtQueryVirtualMemory(MemoryWorkingSetExInformation) ntdll: Use pread in NtQueryVirtualMemory(MemoryWorkingSetExInformation) ntdll: Do not use hardcoded page shift in NtQueryVirtualMemory(MemoryWorkingSetExInformation) ntdll: Speed up NtQueryVirtualMemory(MemoryWorkingSetExInformation) by conditional page check https://gitlab.winehq.org/wine/wine/-/merge_requests/852
From: Witold Baryluk <witold.baryluk(a)gmail.com> Legends game periodically (every 30 seconds) calls this function with up to 22k virtual addresses. All but 1 of them is valid. Due to amount of queries addresses, and cost of seek+read, this causes this function to take up to about 50ms. So framerate drops from ~150 FPS to 20FPS for about a second. As far as I can see, returning 0 entries from this function, still makes Apex Legend work. But keep code correct, and optimise it by only performing pagemap read when the address is valid. This change get_working_set_ex reduces peek wall clock runtime from 57ms to 0.29ms. Tested on Linux, but similar change done for the BSD part. Signed-off-by: Witold Baryluk <witold.baryluk(a)gmail.com> --- dlls/ntdll/unix/virtual.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index e0aa410373e..bf9af40ffe6 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -4237,16 +4237,17 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, BYTE vprot; struct file_view *view; - for (i = 0; i < vmentry_count && entry == NULL; i++) - { - if (vmentries[i].kve_start <= (ULONG_PTR)p->VirtualAddress && (ULONG_PTR)p->VirtualAddress <= vmentries[i].kve_end) - entry = &vmentries[i]; - } memset( &p->VirtualAttributes, 0, sizeof(p->VirtualAttributes) ); if ((view = find_view( p->VirtualAddress, 0 )) && get_committed_size( view, p->VirtualAddress, &vprot, VPROT_COMMITTED ) && (vprot & VPROT_COMMITTED)) { + for (i = 0; i < vmentry_count && entry == NULL; i++) + { + if (vmentries[i].kve_start <= (ULONG_PTR)p->VirtualAddress && (ULONG_PTR)p->VirtualAddress <= vmentries[i].kve_end) + entry = &vmentries[i]; + } + p->VirtualAttributes.Valid = !(vprot & VPROT_GUARD) && (vprot & 0x0f) && entry && entry->kve_type != KVME_TYPE_SWAP; p->VirtualAttributes.Shared = !is_view_valloc( view ); if (p->VirtualAttributes.Shared && p->VirtualAttributes.Valid) @@ -4281,17 +4282,17 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, memset( &p->VirtualAttributes, 0, sizeof(p->VirtualAttributes) ); - /* If we don't have pagemap information, default to invalid. */ - if (!f || fseek( f, ((UINT_PTR)p->VirtualAddress >> 12) * sizeof(pagemap), SEEK_SET ) == -1 || - fread( &pagemap, sizeof(pagemap), 1, f ) != 1) - { - pagemap = 0; - } - if ((view = find_view( p->VirtualAddress, 0 )) && get_committed_size( view, p->VirtualAddress, &vprot, VPROT_COMMITTED ) && (vprot & VPROT_COMMITTED)) { + if (!f || fseek( f, ((UINT_PTR)p->VirtualAddress >> 12) * sizeof(pagemap), SEEK_SET ) == -1 || + fread( &pagemap, sizeof(pagemap), 1, f ) != 1) + { + /* If we don't have pagemap information, default to invalid. */ + pagemap = 0; + } + p->VirtualAttributes.Valid = !(vprot & VPROT_GUARD) && (vprot & 0x0f) && (pagemap >> 63); p->VirtualAttributes.Shared = !is_view_valloc( view ) && ((pagemap >> 61) & 1); if (p->VirtualAttributes.Shared && p->VirtualAttributes.Valid) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/852
From: Witold Baryluk <witold.baryluk(a)gmail.com> --- dlls/ntdll/unix/virtual.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index bf9af40ffe6..5c7370b6d94 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -4286,7 +4286,7 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, get_committed_size( view, p->VirtualAddress, &vprot, VPROT_COMMITTED ) && (vprot & VPROT_COMMITTED)) { - if (!f || fseek( f, ((UINT_PTR)p->VirtualAddress >> 12) * sizeof(pagemap), SEEK_SET ) == -1 || + if (!f || fseek( f, ((UINT_PTR)p->VirtualAddress >> page_shift) * sizeof(pagemap), SEEK_SET ) == -1 || fread( &pagemap, sizeof(pagemap), 1, f ) != 1) { /* If we don't have pagemap information, default to invalid. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/852
From: Witold Baryluk <witold.baryluk(a)gmail.com> 1 syscall instead of 2 syscalls. Faster and simpler code. --- dlls/ntdll/unix/virtual.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 5c7370b6d94..58afc87375c 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -4203,7 +4203,7 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, MEMORY_WORKING_SET_EX_INFORMATION *info, SIZE_T len, SIZE_T *res_len ) { - FILE *f = NULL; + int pagemap_fd; MEMORY_WORKING_SET_EX_INFORMATION *p; sigset_t sigset; @@ -4266,8 +4266,8 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, procstat_close( pstat ); } #else - f = fopen( "/proc/self/pagemap", "rb" ); - if (!f) + pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 ); + if (pagemap_fd == -1) { static int once; if (!once++) WARN( "unable to open /proc/self/pagemap\n" ); @@ -4286,8 +4286,8 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, get_committed_size( view, p->VirtualAddress, &vprot, VPROT_COMMITTED ) && (vprot & VPROT_COMMITTED)) { - if (!f || fseek( f, ((UINT_PTR)p->VirtualAddress >> page_shift) * sizeof(pagemap), SEEK_SET ) == -1 || - fread( &pagemap, sizeof(pagemap), 1, f ) != 1) + if (pagemap_fd == -1 || + pread( pagemap_fd, &pagemap, sizeof(pagemap), ((UINT_PTR)p->VirtualAddress >> page_shift) * sizeof(pagemap) ) != sizeof(pagemap)) { /* If we don't have pagemap information, default to invalid. */ pagemap = 0; @@ -4304,8 +4304,8 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, server_leave_uninterrupted_section( &virtual_mutex, &sigset ); #endif - if (f) - fclose( f ); + if (pagemap_fd != -1) + close( pagemap_fd ); if (res_len) *res_len = (UINT_PTR)p - (UINT_PTR)info; return STATUS_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/852
From: Witold Baryluk <witold.baryluk(a)gmail.com> --- dlls/ntdll/unix/virtual.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 58afc87375c..6dc2abf4666 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -199,6 +199,10 @@ static BYTE **pages_vprot; static BYTE *pages_vprot; #endif +#if !defined(HAVE_LIBPROCSTAT) +static int pagemap_fd = -1; +#endif + static struct file_view *view_block_start, *view_block_end, *next_free_view; static const size_t view_block_size = 0x100000; static void *preload_reserve_start; @@ -4203,7 +4207,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, MEMORY_WORKING_SET_EX_INFORMATION *info, SIZE_T len, SIZE_T *res_len ) { - int pagemap_fd; MEMORY_WORKING_SET_EX_INFORMATION *p; sigset_t sigset; @@ -4266,14 +4269,16 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, procstat_close( pstat ); } #else - pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 ); - if (pagemap_fd == -1) + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); { - static int once; - if (!once++) WARN( "unable to open /proc/self/pagemap\n" ); + static int once = 0; + if (once == 0 && pagemap_fd == -1) { + once = 1; + pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 ); + if (pagemap_fd == -1) WARN( "unable to open /proc/self/pagemap\n" ); + } } - server_enter_uninterrupted_section( &virtual_mutex, &sigset ); for (p = info; (UINT_PTR)(p + 1) <= (UINT_PTR)info + len; p++) { BYTE vprot; @@ -4304,8 +4309,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, server_leave_uninterrupted_section( &virtual_mutex, &sigset ); #endif - if (pagemap_fd != -1) - close( pagemap_fd ); if (res_len) *res_len = (UINT_PTR)p - (UINT_PTR)info; return STATUS_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/852
On 9/17/22 12:13, Witold Baryluk wrote:
From: Witold Baryluk <witold.baryluk(a)gmail.com>
--- dlls/ntdll/unix/virtual.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
This looks good, just a few nitpicks occur to me...
diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 58afc87375c..6dc2abf4666 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -199,6 +199,10 @@ static BYTE **pages_vprot; static BYTE *pages_vprot; #endif
+#if !defined(HAVE_LIBPROCSTAT) +static int pagemap_fd = -1; +#endif +
This doesn't actually need to be moved to global scope.
static struct file_view *view_block_start, *view_block_end, *next_free_view; static const size_t view_block_size = 0x100000; static void *preload_reserve_start; @@ -4203,7 +4207,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, MEMORY_WORKING_SET_EX_INFORMATION *info, SIZE_T len, SIZE_T *res_len ) { - int pagemap_fd; MEMORY_WORKING_SET_EX_INFORMATION *p; sigset_t sigset;
@@ -4266,14 +4269,16 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, procstat_close( pstat ); } #else - pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 ); - if (pagemap_fd == -1) + server_enter_uninterrupted_section( &virtual_mutex, &sigset ); { - static int once; - if (!once++) WARN( "unable to open /proc/self/pagemap\n" ); + static int once = 0;
Static variables are implicitly initialized to zero. That said, you could get rid of "once" by using some other negative value as a sentinel for pagemap_fd (e.g. -2).
+ if (once == 0 && pagemap_fd == -1) {
Braces go on the new line, as in surrounding code.
+ once = 1; + pagemap_fd = open( "/proc/self/pagemap", O_RDONLY, 0 ); + if (pagemap_fd == -1) WARN( "unable to open /proc/self/pagemap\n" ); + } }
- server_enter_uninterrupted_section( &virtual_mutex, &sigset ); for (p = info; (UINT_PTR)(p + 1) <= (UINT_PTR)info + len; p++) { BYTE vprot; @@ -4304,8 +4309,6 @@ static NTSTATUS get_working_set_ex( HANDLE process, LPCVOID addr, server_leave_uninterrupted_section( &virtual_mutex, &sigset ); #endif
- if (pagemap_fd != -1) - close( pagemap_fd ); if (res_len) *res_len = (UINT_PTR)p - (UINT_PTR)info; return STATUS_SUCCESS;
On Sat Sep 17 17:13:13 2022 +0000, Witold Baryluk wrote:
changed this line in [version 8 of the diff](/wine/wine/-/merge_requests/852/diffs?diff_id=11323&start_sha=537251f3d8aab78b5a3ac234fed2e3e37495d196#584f4313ed133393a8f1903c21dcaf4967ba7ab9_4292_4294) Ok. Pushed again, now it worked.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/852#note_8749
participants (3)
-
Witold Baryluk -
Witold Baryluk (＠baryluk)
-
Zebediah Figura