[autobackport: sssd-2-9] Bunch of assorted perf improvements of hot path functions#8458
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several performance improvements to hot path functions. The changes in sdap.c and sss_tc_utf8.c are good optimizations. However, the caching mechanism introduced in usertools.c for sss_create_internal_fqname has a critical thread-safety issue. It uses static variables without any locking, which can lead to race conditions in a multi-threaded environment like the SSSD backend. I've added a comment with a suggested fix.
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
backport matches the original version, ACK.
bye,
Sumit
|
The pull request was accepted by @justin-stephenson with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
7da25e4 to
350d4c6
Compare
Both `perf` and manual measurement confirms ~6..8% perf gain in the test case: - INITGROUPS lookup for a user that is a member of 5k groups, no groups were cached; - debug_level = 3 - debug_microseconds = true Note `debug_microseconds = true` - without this setting impact isn't that dramatic. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 09e283e)
In vast majority of cases strings are ascii and lowercase. In other cases overhead added should be negligible. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit 9a2cf21)
This helper is heavily used, including in hot paths. Since number of domains used is very limited, hash table used for caching should be very small and lookup much more efficient as compared with `sss_tc_utf8_str_tolower()` Assisted-by: Claude Code (Opus 4.6) Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com> (cherry picked from commit a5b77e4)
|
The pull request was accepted by @justin-stephenson with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
350d4c6 to
3fe0ec8
Compare
This is an automatic backport of PR#8447 Bunch of assorted perf improvements of hot path functions to branch sssd-2-9, created by @alexey-tikhonov.
Please make sure this backport is correct.
Note
The commits were cherry-picked without conflicts.
You can push changes to this pull request
Original commits
09e283e - SDAP: use
DEBUG_CONDITIONALin hot path9a2cf21 - UTIL:
sss_tc_utf8_str_tolower()optimizationa5b77e4 - UTIL:
sss_create_internal_fqname()optimization (caching)Backported commits
DEBUG_CONDITIONALin hot pathsss_tc_utf8_str_tolower()optimizationsss_create_internal_fqname()optimization (caching)Original Pull Request Body
Those patches are based on a profiling of a following test case:
tu1andtu2are members of 5k LDAP groups (RFC2307 case, no nested groups)time id tu1@ldap.test | tr ',' '\n' | wc -lis executed(Note you need to tweak both LDAP (limits) and SSSD (
timeout,client_idle_timeout) settings to make this req complete successfully).Combined, those patches reduce lookup time on my laptom from ~51+s to ~45-s (at least with
debug_microseconds = true)Note that much more fruitful optimizations are possible under
sdap_initgr_common_store(), including getting rid of O(N^2) loop oversdap_get_group_primary_name()insdap_add_incomplete_groups(), and some of those optimization will make patches in this PR kind of obsolete for this specific test case. But given that those helpers are heavily used across all code base, I think it's still worth consideration.