[PATCH v2 0/6] MR78: NSI and GetAdaptersInfo() performance improvements
This is inspired by Street Fighter V game which performance has regressed in multiplayer mode since iphlpapi reimplementation on top of NSI. Speficically, the game calls GetAdapatersInfo() very often which turnaround time increased dramatically. While I was checking the effect against GetAdapatersInfo() only I hope the patchset might improve the performance of some other iphlpapi functions as well. Here is the patch I used as a top level measure of the effect: https://gist.github.com/gofman/c08e58f15105d60b4576c9ae25bcec9d The table below lists the times of GetAdaptersInfo() in ms measured with this test (this is the time for GetAdapatersInfo(NULL, ...) for estimating the buffer length plus actual data query call. The major difference between Machine 1 and Machine 2 is most likely in the number of ip routing table entries (8 on "Machine 2" and 4 on "Machine 1"). Windows has less interfaces and routes configured so it is probably not suitable for back to back performance comparison this way although the datapoint may still be interesting for very rough comparison. The baseline here is the performance before iphlpapi redesign (which I measured on Proton 6.3). There are maybe more opportunities for optimization but after this patchset GetAdapatersInfo() performance is slightly improved over baseline so I stopped here for now. ``` Machine 1 Machine 2 Windows 0.7 Proton 6.3 2.7 8.7 Current Wine 12.2 99.8 After patch 1 10.0 59.2 After patch 4 5.5 27.2 After patch 5 3.6 20.2 After patch 6 2.4 7.6 ``` -- v2: iphlpapi: Don't request unused dynamic interface data. nsi: Allocate a small buffer at once in NsiAllocateAndGetTable(). nsiproxy.sys: Update interface table only if LUID is not found in convert_unix_name_to_luid(). nsiproxy.sys: Update interface table only if LUID is not found in convert_luid_to_unix_name(). nsiproxy.sys: Finish search once found in convert_luid_to_unix_name(). https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index ba4933ff16b..a3a10148109 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -495,9 +495,11 @@ static NTSTATUS ifinfo_get_all_parameters( const void *key, UINT key_size, void pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_luid( (const NET_LUID *)key ); + if (!(entry = find_entry_from_luid( (const NET_LUID *)key ))) + { + update_if_table(); + entry = find_entry_from_luid( (const NET_LUID *)key ); + } if (entry) { ifinfo_fill_entry( entry, NULL, rw_data, dynamic_data, static_data ); @@ -557,9 +559,11 @@ static NTSTATUS ifinfo_get_parameter( const void *key, UINT key_size, UINT param pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_luid( (const NET_LUID *)key ); + if (!(entry = find_entry_from_luid( (const NET_LUID *)key ))) + { + update_if_table(); + entry = find_entry_from_luid( (const NET_LUID *)key ); + } if (entry) { switch (param_type) @@ -591,9 +595,11 @@ static NTSTATUS index_luid_get_parameter( const void *key, UINT key_size, UINT p pthread_mutex_lock( &if_list_lock ); - update_if_table(); - - entry = find_entry_from_index( *(UINT *)key ); + if (!(entry = find_entry_from_index( *(UINT *)key ))) + { + update_if_table(); + entry = find_entry_from_index( *(UINT *)key ); + } if (entry) { *(NET_LUID *)data = entry->if_luid; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index a3a10148109..929bda9cd90 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -644,7 +644,7 @@ BOOL convert_luid_to_unix_name( const NET_LUID *luid, const char **unix_name ) { *unix_name = entry->if_unix_name; ret = TRUE; - + break; } pthread_mutex_unlock( &if_list_lock ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index 929bda9cd90..152e808994e 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -634,18 +634,24 @@ BOOL convert_luid_to_unix_name( const NET_LUID *luid, const char **unix_name ) { struct if_entry *entry; BOOL ret = FALSE; + int updated = 0; pthread_mutex_lock( &if_list_lock ); - update_if_table(); + do + { + LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) + if (entry->if_luid.Value == luid->Value) + { + *unix_name = entry->if_unix_name; + ret = TRUE; + goto done; + } + update_if_table(); + } + while (!updated++); - LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) - if (entry->if_luid.Value == luid->Value) - { - *unix_name = entry->if_unix_name; - ret = TRUE; - break; - } +done: pthread_mutex_unlock( &if_list_lock ); return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsiproxy.sys/ndis.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/dlls/nsiproxy.sys/ndis.c b/dlls/nsiproxy.sys/ndis.c index 152e808994e..47781d1db5d 100644 --- a/dlls/nsiproxy.sys/ndis.c +++ b/dlls/nsiproxy.sys/ndis.c @@ -613,18 +613,24 @@ BOOL convert_unix_name_to_luid( const char *unix_name, NET_LUID *luid ) { struct if_entry *entry; BOOL ret = FALSE; + int updated = 0; pthread_mutex_lock( &if_list_lock ); - update_if_table(); + do + { + LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) + if (!strcmp( entry->if_unix_name, unix_name )) + { + *luid = entry->if_luid; + ret = TRUE; + goto done; + } + update_if_table(); + } + while (!updated++); - LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) - if (!strcmp( entry->if_unix_name, unix_name )) - { - *luid = entry->if_luid; - ret = TRUE; - break; - } +done: pthread_mutex_unlock( &if_list_lock ); return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/nsi/nsi.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dlls/nsi/nsi.c b/dlls/nsi/nsi.c index efb10c0f615..345dce6dfe6 100644 --- a/dlls/nsi/nsi.c +++ b/dlls/nsi/nsi.c @@ -39,7 +39,7 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR void **rw_data, DWORD rw_size, void **dynamic_data, DWORD dynamic_size, void **static_data, DWORD static_size, DWORD *count, DWORD unk2 ) { - DWORD err, num = 0; + DWORD err, num = 64; void *data[4] = { NULL }; DWORD sizes[4] = { key_size, rw_size, dynamic_size, static_size }; int i, attempt; @@ -49,9 +49,6 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR for (attempt = 0; attempt < 5; attempt++) { - err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); - if (err) return err; - for (i = 0; i < ARRAY_SIZE(data); i++) { if (sizes[i]) @@ -68,9 +65,11 @@ DWORD WINAPI NsiAllocateAndGetTable( DWORD unk, const NPI_MODULEID *module, DWOR err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, data[0], sizes[0], data[1], sizes[1], data[2], sizes[2], data[3], sizes[3], &num ); if (err != ERROR_MORE_DATA) break; - + TRACE( "Short buffer, attempt %d.\n", attempt ); NsiFreeTable( data[0], data[1], data[2], data[3] ); memset( data, 0, sizeof(data) ); + err = NsiEnumerateObjectsAllParameters( unk, 0, module, table, NULL, 0, NULL, 0, NULL, 0, NULL, 0, &num ); + if (err) return err; } if (!err) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/iphlpapi/iphlpapi_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 0bd7cdc06c8..5a4acdb9c5a 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -598,7 +598,6 @@ DWORD WINAPI GetAdaptersInfo( IP_ADAPTER_INFO *info, ULONG *size ) DWORD len, i, uni, fwd; NET_LUID *if_keys = NULL; struct nsi_ndis_ifinfo_rw *if_rw = NULL; - struct nsi_ndis_ifinfo_dynamic *if_dyn = NULL; struct nsi_ndis_ifinfo_static *if_stat = NULL; struct nsi_ipv4_unicast_key *uni_keys = NULL; struct nsi_ip_unicast_rw *uni_rw = NULL; @@ -612,7 +611,8 @@ DWORD WINAPI GetAdaptersInfo( IP_ADAPTER_INFO *info, ULONG *size ) err = NsiAllocateAndGetTable( 1, &NPI_MS_NDIS_MODULEID, NSI_NDIS_IFINFO_TABLE, (void **)&if_keys, sizeof(*if_keys), (void **)&if_rw, sizeof(*if_rw), - (void **)&if_dyn, sizeof(*if_dyn), (void **)&if_stat, sizeof(*if_stat), &if_count, 0 ); + NULL, 0, (void **)&if_stat, sizeof(*if_stat), &if_count, 0 ); + if (err) return err; for (i = 0; i < if_count; i++) { @@ -726,7 +726,7 @@ err: heap_free( wins_servers ); NsiFreeTable( fwd_keys, NULL, NULL, NULL ); NsiFreeTable( uni_keys, uni_rw, NULL, NULL ); - NsiFreeTable( if_keys, if_rw, if_dyn, if_stat ); + NsiFreeTable( if_keys, if_rw, NULL, if_stat ); return err; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/78
v2: - do not depend on the interfaces being updated to the end o fthe list in patches 3, 4; - fix the title of patch 2 (Wine module name). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/78#note_942
Huw Davies (@huw) commented about dlls/nsiproxy.sys/ndis.c:
pthread_mutex_lock( &if_list_lock );
- update_if_table(); + do + { + LIST_FOR_EACH_ENTRY( entry, &if_list, struct if_entry, entry ) + if (entry->if_luid.Value == luid->Value) + { + *unix_name = entry->if_unix_name; + ret = TRUE; + goto done; + } + update_if_table(); + } + while (!updated++);
Now we could end up calling `update_if_table()` twice which seems wasteful. How about making `update_if_table()` return the number of entries it adds, then we could do something like ```c do { } while (!updated++ && update_if_table()); ``` Also, please keep the `while` on the same line as the closing brace (so it doesn't look like a while loop) and add braces around the `LIST_FOR_EACH_ENTRY()` block. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/78#note_943
participants (3)
-
Huw Davies (＠huw) -
Paul Gofman
-
Paul Gofman (＠gofman)