Skip to content

SDAP: reduce logger load in the hot paths#8540

Merged
alexey-tikhonov merged 2 commits intoSSSD:masterfrom
alexey-tikhonov:initgroups-nested
Mar 25, 2026
Merged

SDAP: reduce logger load in the hot paths#8540
alexey-tikhonov merged 2 commits intoSSSD:masterfrom
alexey-tikhonov:initgroups-nested

Conversation

@alexey-tikhonov
Copy link
Member

@alexey-tikhonov alexey-tikhonov commented Mar 20, 2026

Logger usage in sdap_get_generic_ext_step() accounts for ~2..3% in the test

time id jambo@ldap.test | tr ',' '\n' | wc -l
2802

when ignore_group_members = false and there are 12k users in total.

Other logs changes contribute ~1% in the same test.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors logging within the sdap_get_generic_ext_step function to reduce performance overhead in this hot path. The changes include consolidating multiple DEBUG calls into a single statement and using DEBUG_CONDITIONAL for logging requested attributes. This prevents populating the backtrace with attribute information unless a specific debug level is enabled. The changes are consistent with the goal of reducing logger load. I have reviewed the implementation and found no issues.

@alexey-tikhonov alexey-tikhonov added the Performance Performance related patches label Mar 20, 2026
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review March 20, 2026 15:45
@alexey-tikhonov alexey-tikhonov changed the title SDAP: reduce logger load in the hot path SDAP: reduce logger load in the hot paths Mar 20, 2026
@alexey-tikhonov alexey-tikhonov added the coverity Trigger a coverity scan label Mar 20, 2026
@alexey-tikhonov
Copy link
Member Author

Note: Covscan is green.

@alexey-tikhonov alexey-tikhonov removed the coverity Trigger a coverity scan label Mar 20, 2026
@alexey-tikhonov
Copy link
Member Author

Users : 12006 (1 user is a member of all groups)
Groups : 2801 nested (~2.4k leaf groups, 5 users / leaf group) + 1 private (that contains all members)
Profiling time id jambo@ldap.test | tr ',' '\n' | wc -l (2802 groups):

(I) ignore_group_members = false

  • without patches: sss_debug_fn() ~ 9.4%
  • this PR: sss_debug_fn() ~ 4%

(II) ignore_group_members = true

  • without patches: sss_debug_fn() ~ 8.5%
  • this PR: sss_debug_fn() ~ 3.96%

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the patch. I agree that is is acceptable to loose the messages which are now send with DEBUG_CONDITIONAL from the backtrace, ACK.

bye,
Sumit

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This patch reduced number of *sprintf() keeping the same level
of details in the resulting log.
Besides, list of attrs being requested was excluded from the backtrace.

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
@sssd-bot
Copy link
Contributor

The pull request was accepted by @ikerexxe with the following PR CI status:


🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot sssd-bot force-pushed the initgroups-nested branch from 662d7d2 to 7374851 Compare March 25, 2026 10:32
@alexey-tikhonov alexey-tikhonov merged commit 87c7bce into SSSD:master Mar 25, 2026
13 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants