From 2f0808a6108c0bc8f8ec6192772494659c9a1288 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 7 Oct 2022 12:42:02 +0200 Subject: [PATCH 1/4] std-aux: add _nm_always_inline for "__attribute__((__always_inline__))" --- src/libnm-std-aux/nm-std-aux.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-std-aux/nm-std-aux.h b/src/libnm-std-aux/nm-std-aux.h index b404b835c5..1ff547d5dd 100644 --- a/src/libnm-std-aux/nm-std-aux.h +++ b/src/libnm-std-aux/nm-std-aux.h @@ -16,6 +16,7 @@ #define _nm_packed __attribute__((__packed__)) #define _nm_unused __attribute__((__unused__)) +#define _nm_always_inline __attribute__((__always_inline__)) #define _nm_used __attribute__((__used__)) #define _nm_pure __attribute__((__pure__)) #define _nm_const __attribute__((__const__)) From 28fa48aee44cae8349b3614ee2b9c1837fea0074 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 7 Oct 2022 12:25:58 +0200 Subject: [PATCH 2/4] glib-aux: add an inlinable version of nm_array_find_bsearch() To implement binary search is not very hard. It's almost easy enough to just open-code it, without using the existing nm_array_find_bsearch() function. In particular, because nm_array_find_bsearch() won't be inlined, and thus it is slower than implementing it by hand. Add nm_array_find_bsearch_inline() as a variant that will be inlined. This actually is as fast as reimplementing it by hand (I measured), which takes away any reason to avoid the function. However, our headers get huge. That may be a problem for complication time. To counter that a bit, only define the function when the caller requests it with a NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE define. --- src/libnm-glib-aux/nm-shared-utils.c | 34 ++---------------- src/libnm-glib-aux/nm-shared-utils.h | 53 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index f60ae1f2d6..66eb17e709 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -3,6 +3,8 @@ * Copyright (C) 2016 Red Hat, Inc. */ +#define NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE + #include "libnm-glib-aux/nm-default-glib-i18n-lib.h" #include "nm-shared-utils.h" @@ -3943,37 +3945,7 @@ nm_array_find_bsearch(gconstpointer list, GCompareDataFunc cmpfcn, gpointer user_data) { - gssize imax; - gssize imid; - gssize imin; - int cmp; - - nm_assert(list || len == 0); - nm_assert(cmpfcn); - nm_assert(elem_size > 0); - - imin = 0; - if (len == 0) - return ~imin; - - imax = len - 1; - - while (imin <= imax) { - imid = imin + (imax - imin) / 2; - - cmp = cmpfcn(&((const char *) list)[elem_size * imid], needle, user_data); - if (cmp == 0) - return imid; - - if (cmp < 0) - imin = imid + 1; - else - imax = imid - 1; - } - - /* return the inverse of @imin. This is a negative number, but - * also is ~imin the position where the value should be inserted. */ - return ~imin; + return nm_array_find_bsearch_inline(list, len, elem_size, needle, cmpfcn, user_data); } /*****************************************************************************/ diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 3383e6d03f..c1325a8c41 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2135,6 +2135,57 @@ gssize nm_ptrarray_find_bsearch_range(gconstpointer *list, NULL); \ }) +/*****************************************************************************/ + +#ifdef NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE +/** + * nm_array_find_bsearch_inline: + * + * An inlined version of nm_array_find_bsearch(). See there. + * Define NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE to get it. + */ +_nm_always_inline static inline gssize +nm_array_find_bsearch_inline(gconstpointer list, + gsize len, + gsize elem_size, + gconstpointer needle, + GCompareDataFunc cmpfcn, + gpointer user_data) +{ + gssize imax; + gssize imid; + gssize imin; + int cmp; + + nm_assert(list || len == 0); + nm_assert(cmpfcn); + nm_assert(elem_size > 0); + + imin = 0; + if (len == 0) + return ~imin; + + imax = len - 1; + + while (imin <= imax) { + imid = imin + (imax - imin) / 2; + + cmp = cmpfcn(&((const char *) list)[elem_size * imid], needle, user_data); + if (cmp == 0) + return imid; + + if (cmp < 0) + imin = imid + 1; + else + imax = imid - 1; + } + + /* return the inverse of @imin. This is a negative number, but + * also is ~imin the position where the value should be inserted. */ + return ~imin; +} +#endif + gssize nm_array_find_bsearch(gconstpointer list, gsize len, gsize elem_size, @@ -2142,6 +2193,8 @@ gssize nm_array_find_bsearch(gconstpointer list, GCompareDataFunc cmpfcn, gpointer user_data); +/*****************************************************************************/ + gssize nm_utils_ptrarray_find_first(gconstpointer *list, gssize len, gconstpointer needle); /*****************************************************************************/ From 5f4eb5eed53e86e8a84c163a19b8c7a84807f752 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Sep 2022 11:29:20 +0200 Subject: [PATCH 3/4] libnm: cleanup implementations for nm_meta_setting_infos_by_gtype() The file "nm-meta-setting-base-impl.c" is present twice in our source tree and compiled twice. Once with internal headers of libnm-core and once with only public headers. Consequently, there are two implementations for nm_meta_setting_infos_by_gtype(), one that can benefit from internal access to the data structure, and the one for libnmc, which cannot. Refactor the implementations, in the hope to have it clearer. --- .../nm-meta-setting-base-impl.c | 57 ++++++++++--------- .../nm-meta-setting-base-impl.c | 57 ++++++++++--------- 2 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/libnm-core-impl/nm-meta-setting-base-impl.c b/src/libnm-core-impl/nm-meta-setting-base-impl.c index ebc04556a7..31e1012b1d 100644 --- a/src/libnm-core-impl/nm-meta-setting-base-impl.c +++ b/src/libnm-core-impl/nm-meta-setting-base-impl.c @@ -703,19 +703,21 @@ nm_meta_setting_infos_by_name(const char *name) return idx >= 0 ? &nm_meta_setting_infos[idx] : NULL; } -const NMMetaSettingInfo * -nm_meta_setting_infos_by_gtype(GType gtype) -{ +/*****************************************************************************/ + #if _NM_META_SETTING_BASE_IMPL_LIBNM +static const NMMetaSettingInfo * +_infos_by_gtype_from_class(GType gtype) +{ nm_auto_unref_gtypeclass GTypeClass *gtypeclass_unref = NULL; GTypeClass *gtypeclass; NMSettingClass *klass; if (!g_type_is_a(gtype, NM_TYPE_SETTING)) - goto out_none; + return NULL; gtypeclass = g_type_class_peek(gtype); - if (!gtypeclass) + if (G_UNLIKELY(!gtypeclass)) gtypeclass = gtypeclass_unref = g_type_class_ref(gtype); nm_assert(NM_IS_SETTING_CLASS(gtypeclass)); @@ -723,38 +725,41 @@ nm_meta_setting_infos_by_gtype(GType gtype) klass = (NMSettingClass *) gtypeclass; if (!klass->setting_info) - goto out_none; + return NULL; nm_assert(klass->setting_info->get_setting_gtype); nm_assert(klass->setting_info->get_setting_gtype() == gtype); - return klass->setting_info; +} +#endif -out_none: +static const NMMetaSettingInfo * +_infos_by_gtype_search(GType gtype) +{ + int i; - if (NM_MORE_ASSERTS > 10) { - int i; - - /* this might hint to a bug, but it would be expected for NM_TYPE_SETTING - * and NM_TYPE_SETTING_IP_CONFIG. - * - * Assert that we didn't lookup for a gtype, which we would expect to find. - * An assertion failure here, hints to a bug in nm_setting_*_class_init(). - */ - for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) - nm_assert(nm_meta_setting_infos[i].get_setting_gtype() != gtype); - } - - return NULL; -#else - guint i; - - for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) { + for (i = 0; i < (int) _NM_META_SETTING_TYPE_NUM; i++) { if (nm_meta_setting_infos[i].get_setting_gtype() == gtype) return &nm_meta_setting_infos[i]; } return NULL; +} + +const NMMetaSettingInfo * +nm_meta_setting_infos_by_gtype(GType gtype) +{ + const NMMetaSettingInfo *setting_info; + +#if _NM_META_SETTING_BASE_IMPL_LIBNM + setting_info = _infos_by_gtype_from_class(gtype); + + if (NM_MORE_ASSERTS > 20) + nm_assert(setting_info == _infos_by_gtype_search(gtype)); +#else + setting_info = _infos_by_gtype_search(gtype); #endif + + return setting_info; } /*****************************************************************************/ diff --git a/src/libnmc-setting/nm-meta-setting-base-impl.c b/src/libnmc-setting/nm-meta-setting-base-impl.c index ebc04556a7..31e1012b1d 100644 --- a/src/libnmc-setting/nm-meta-setting-base-impl.c +++ b/src/libnmc-setting/nm-meta-setting-base-impl.c @@ -703,19 +703,21 @@ nm_meta_setting_infos_by_name(const char *name) return idx >= 0 ? &nm_meta_setting_infos[idx] : NULL; } -const NMMetaSettingInfo * -nm_meta_setting_infos_by_gtype(GType gtype) -{ +/*****************************************************************************/ + #if _NM_META_SETTING_BASE_IMPL_LIBNM +static const NMMetaSettingInfo * +_infos_by_gtype_from_class(GType gtype) +{ nm_auto_unref_gtypeclass GTypeClass *gtypeclass_unref = NULL; GTypeClass *gtypeclass; NMSettingClass *klass; if (!g_type_is_a(gtype, NM_TYPE_SETTING)) - goto out_none; + return NULL; gtypeclass = g_type_class_peek(gtype); - if (!gtypeclass) + if (G_UNLIKELY(!gtypeclass)) gtypeclass = gtypeclass_unref = g_type_class_ref(gtype); nm_assert(NM_IS_SETTING_CLASS(gtypeclass)); @@ -723,38 +725,41 @@ nm_meta_setting_infos_by_gtype(GType gtype) klass = (NMSettingClass *) gtypeclass; if (!klass->setting_info) - goto out_none; + return NULL; nm_assert(klass->setting_info->get_setting_gtype); nm_assert(klass->setting_info->get_setting_gtype() == gtype); - return klass->setting_info; +} +#endif -out_none: +static const NMMetaSettingInfo * +_infos_by_gtype_search(GType gtype) +{ + int i; - if (NM_MORE_ASSERTS > 10) { - int i; - - /* this might hint to a bug, but it would be expected for NM_TYPE_SETTING - * and NM_TYPE_SETTING_IP_CONFIG. - * - * Assert that we didn't lookup for a gtype, which we would expect to find. - * An assertion failure here, hints to a bug in nm_setting_*_class_init(). - */ - for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) - nm_assert(nm_meta_setting_infos[i].get_setting_gtype() != gtype); - } - - return NULL; -#else - guint i; - - for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) { + for (i = 0; i < (int) _NM_META_SETTING_TYPE_NUM; i++) { if (nm_meta_setting_infos[i].get_setting_gtype() == gtype) return &nm_meta_setting_infos[i]; } return NULL; +} + +const NMMetaSettingInfo * +nm_meta_setting_infos_by_gtype(GType gtype) +{ + const NMMetaSettingInfo *setting_info; + +#if _NM_META_SETTING_BASE_IMPL_LIBNM + setting_info = _infos_by_gtype_from_class(gtype); + + if (NM_MORE_ASSERTS > 20) + nm_assert(setting_info == _infos_by_gtype_search(gtype)); +#else + setting_info = _infos_by_gtype_search(gtype); #endif + + return setting_info; } /*****************************************************************************/ From 611f2c3a60af1895575e0a0d2802952ff35f3c6c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 6 Sep 2022 11:29:20 +0200 Subject: [PATCH 4/4] libnm: use binary search for nm_meta_setting_infos_by_gtype() for libnmc The file "nm-meta-setting-base-impl.c" is shared by "libnm-core-impl" and "libnmc-setting". For "libnm-core-impl" it uses a efficient lookup from the gtype. For "libnmc-setting", that class information is not available, so it did a linear search. Instead, do a binary search. Tested: diff --git a/src/libnm-core-impl/nm-meta-setting-base-impl.c b/src/libnm-core-impl/nm-meta-setting-base-impl.c index 3434c858391f..62c366d2ca42 100644 --- a/src/libnm-core-impl/nm-meta-setting-base-impl.c +++ b/src/libnm-core-impl/nm-meta-setting-base-impl.c @@ -821,6 +821,11 @@ nm_meta_setting_infos_by_gtype(GType gtype) { const NMMetaSettingInfo *setting_info; +#if _NM_META_SETTING_BASE_IMPL_LIBNM + return _infos_by_gtype_binary_search(gtype); +#endif + nm_assert_not_reached(); + #if _NM_META_SETTING_BASE_IMPL_LIBNM setting_info = _infos_by_gtype_from_class(gtype); #else diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 85d549eb766c..65fcafd076c9 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -5134,6 +5134,29 @@ main(int argc, char **argv) { nmtst_init(&argc, &argv, TRUE); + { + gs_unref_object NMConnection *con = NULL; + guint8 ctr = 0; + guint i; + + con = nmtst_create_minimal_connection("test", NULL, NM_SETTING_WIRED_SETTING_NAME, NULL); + + nm_connection_add_setting(con, nm_setting_wired_new()); + + nmtst_connection_normalize(con); + nmtst_assert_connection_verifies_without_normalization(con); + + for (i = 0; i < 10000000; i++) { + ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_WIRED)); + ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_CONNECTION)); + ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_PROXY)); + ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_WIREGUARD)); + ctr += GPOINTER_TO_UINT(nm_connection_get_setting(con, NM_TYPE_SETTING_IP4_CONFIG)); + } + + return !!ctr; + } + g_test_add_func("/libnm/test_connection_uuid", test_connection_uuid); g_test_add_func("/libnm/settings/test_nm_meta_setting_types_by_priority", Results of `make src/libnm-core-impl/tests/test-setting && libtool --mode=execute perf stat -r 5 -B src/libnm-core-impl/tests/test-setting`: 1) previous linear search: 3,182.81 msec 2) bsearch not inlined: 1,611.19 msec 3) bsearch open-coded: 1,214.45 msec 4) bsearch inlined: 1,167.70 msec 5) lookup from class: 1,147.64 msec 1) previous implementation 2) using nm_array_find_bsearch() 3) manually implementing binary search 4) using nm_array_find_bsearch_inline() 5) only available to libnm-core as it uses internal meta data Note that for "libnm-core-impl" the implementation was already fast (5), and it didn't change. It only changed to binary search for "libnmc-setting", which arguably is a less strong use-case. --- .../nm-meta-setting-base-impl.c | 81 ++++++++++++++++++- .../nm-meta-setting-base-impl.c | 81 ++++++++++++++++++- 2 files changed, 154 insertions(+), 8 deletions(-) diff --git a/src/libnm-core-impl/nm-meta-setting-base-impl.c b/src/libnm-core-impl/nm-meta-setting-base-impl.c index 31e1012b1d..5e627cfc7d 100644 --- a/src/libnm-core-impl/nm-meta-setting-base-impl.c +++ b/src/libnm-core-impl/nm-meta-setting-base-impl.c @@ -3,6 +3,8 @@ * Copyright (C) 2017 - 2018 Red Hat, Inc. */ +#define NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE + #include "libnm-glib-aux/nm-default-glib-i18n-lib.h" #include "nm-meta-setting-base.h" @@ -745,6 +747,75 @@ _infos_by_gtype_search(GType gtype) return NULL; } +typedef struct { + GType gtype; + const NMMetaSettingInfo *setting_info; +} LookupData; + +_nm_always_inline static inline int +_lookup_data_cmp(gconstpointer ptr_a, gconstpointer ptr_b, gpointer user_data) +{ + const GType *const a = ptr_a; + const GType *const b = ptr_b; + + nm_assert(a); + nm_assert(b); + nm_assert(a != b); + + NM_CMP_DIRECT(*a, *b); + return 0; +} + +static const NMMetaSettingInfo * +_infos_by_gtype_binary_search(GType gtype) +{ + static LookupData static_array[_NM_META_SETTING_TYPE_NUM]; + static const LookupData *static_ptr = NULL; + const LookupData *ptr; + gssize idx; + +again: + ptr = g_atomic_pointer_get(&static_ptr); + if (G_UNLIKELY(!ptr)) { + static gsize g_lock = 0; + int i; + + if (!g_once_init_enter(&g_lock)) + goto again; + + for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) { + const NMMetaSettingInfo *m = &nm_meta_setting_infos[i]; + + static_array[i] = (LookupData){ + .gtype = m->get_setting_gtype(), + .setting_info = m, + }; + } + + g_qsort_with_data(static_array, + _NM_META_SETTING_TYPE_NUM, + sizeof(static_array[0]), + _lookup_data_cmp, + NULL); + + ptr = static_array; + g_atomic_pointer_set(&static_ptr, ptr); + + g_once_init_leave(&g_lock, 1); + } + + idx = nm_array_find_bsearch_inline(ptr, + _NM_META_SETTING_TYPE_NUM, + sizeof(ptr[0]), + >ype, + _lookup_data_cmp, + NULL); + if (idx < 0) + return NULL; + + return ptr[idx].setting_info; +} + const NMMetaSettingInfo * nm_meta_setting_infos_by_gtype(GType gtype) { @@ -752,13 +823,15 @@ nm_meta_setting_infos_by_gtype(GType gtype) #if _NM_META_SETTING_BASE_IMPL_LIBNM setting_info = _infos_by_gtype_from_class(gtype); - - if (NM_MORE_ASSERTS > 20) - nm_assert(setting_info == _infos_by_gtype_search(gtype)); #else - setting_info = _infos_by_gtype_search(gtype); + setting_info = _infos_by_gtype_binary_search(gtype); #endif + if (NM_MORE_ASSERTS > 20) { + nm_assert(setting_info == _infos_by_gtype_search(gtype)); + nm_assert(setting_info == _infos_by_gtype_binary_search(gtype)); + } + return setting_info; } diff --git a/src/libnmc-setting/nm-meta-setting-base-impl.c b/src/libnmc-setting/nm-meta-setting-base-impl.c index 31e1012b1d..5e627cfc7d 100644 --- a/src/libnmc-setting/nm-meta-setting-base-impl.c +++ b/src/libnmc-setting/nm-meta-setting-base-impl.c @@ -3,6 +3,8 @@ * Copyright (C) 2017 - 2018 Red Hat, Inc. */ +#define NM_WANT_NM_ARRAY_FIND_BSEARCH_INLINE + #include "libnm-glib-aux/nm-default-glib-i18n-lib.h" #include "nm-meta-setting-base.h" @@ -745,6 +747,75 @@ _infos_by_gtype_search(GType gtype) return NULL; } +typedef struct { + GType gtype; + const NMMetaSettingInfo *setting_info; +} LookupData; + +_nm_always_inline static inline int +_lookup_data_cmp(gconstpointer ptr_a, gconstpointer ptr_b, gpointer user_data) +{ + const GType *const a = ptr_a; + const GType *const b = ptr_b; + + nm_assert(a); + nm_assert(b); + nm_assert(a != b); + + NM_CMP_DIRECT(*a, *b); + return 0; +} + +static const NMMetaSettingInfo * +_infos_by_gtype_binary_search(GType gtype) +{ + static LookupData static_array[_NM_META_SETTING_TYPE_NUM]; + static const LookupData *static_ptr = NULL; + const LookupData *ptr; + gssize idx; + +again: + ptr = g_atomic_pointer_get(&static_ptr); + if (G_UNLIKELY(!ptr)) { + static gsize g_lock = 0; + int i; + + if (!g_once_init_enter(&g_lock)) + goto again; + + for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) { + const NMMetaSettingInfo *m = &nm_meta_setting_infos[i]; + + static_array[i] = (LookupData){ + .gtype = m->get_setting_gtype(), + .setting_info = m, + }; + } + + g_qsort_with_data(static_array, + _NM_META_SETTING_TYPE_NUM, + sizeof(static_array[0]), + _lookup_data_cmp, + NULL); + + ptr = static_array; + g_atomic_pointer_set(&static_ptr, ptr); + + g_once_init_leave(&g_lock, 1); + } + + idx = nm_array_find_bsearch_inline(ptr, + _NM_META_SETTING_TYPE_NUM, + sizeof(ptr[0]), + >ype, + _lookup_data_cmp, + NULL); + if (idx < 0) + return NULL; + + return ptr[idx].setting_info; +} + const NMMetaSettingInfo * nm_meta_setting_infos_by_gtype(GType gtype) { @@ -752,13 +823,15 @@ nm_meta_setting_infos_by_gtype(GType gtype) #if _NM_META_SETTING_BASE_IMPL_LIBNM setting_info = _infos_by_gtype_from_class(gtype); - - if (NM_MORE_ASSERTS > 20) - nm_assert(setting_info == _infos_by_gtype_search(gtype)); #else - setting_info = _infos_by_gtype_search(gtype); + setting_info = _infos_by_gtype_binary_search(gtype); #endif + if (NM_MORE_ASSERTS > 20) { + nm_assert(setting_info == _infos_by_gtype_search(gtype)); + nm_assert(setting_info == _infos_by_gtype_binary_search(gtype)); + } + return setting_info; }