Bug 15737 - Changes from DEBUGC/DEBUGADDC to talloc_asprintf ignoring debug level - performance drops
Summary: Changes from DEBUGC/DEBUGADDC to talloc_asprintf ignoring debug level - perfo...
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.20.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-14 08:06 UTC by Subba Ramanna Bodda
Modified: 2024-10-15 07:56 UTC (History)
1 user (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2024-10-14 11:31 UTC, Volker Lendecke
no flags Details
Patch v1 (1.55 KB, patch)
2024-10-15 07:55 UTC, Subba Ramanna Bodda
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Subba Ramanna Bodda 2024-10-14 08:06:10 UTC
6071220fcb1 libcli: Make debug_unix_user_token() use just one DEBUG statement
09c787c34a9 libcli: Make security_token_debug() use just one DEBUG statement
1ad84c70fe2 libcli: Convert security_token_debug_privileges() to talloc_asprintf

In these commits, original DEBUG/DEBUGADDC considers the debug level. Once changed to talloc_asprintf/talloc_asprintf_addbuf, it is done for any debug level.

Original:
-                               DEBUGADDC(dbg_class, dbg_lev,
-                                         ("  Right[%3lu]: %s\n", (unsigned long)i++,
-                                          rights[idx].name));


New:
+                               talloc_asprintf_addbuf(&s,
+                                                      "  Right[%3zu]: %s\n",
+                                                      i++,
+                                                      rights[idx].name);


I have observed specfs performance degradation, and removal of these commits got back the numbers. In case, these changes are printing it to log for any debug level, that can be the reason, and need a fix. Even otherwise, need to check if we are issuing talloc* without considering the debug level and fix.
Comment 1 Volker Lendecke 2024-10-14 08:22:02 UTC
Thanks for finding this. I would like to keep the "just one DBG" statement for enhanced debugging, so the callers need to be adapted to not call the string generators at all. For example in debug_unix_user_token you could use an early return, likewise for the other ones.

If you need to keep this private to your product, please note this here so that we can fix it upstream in parallel as well at some point in the future.
Comment 2 Volker Lendecke 2024-10-14 11:31:40 UTC
Created attachment 18475 [details]
Patch

Does that help?
Comment 3 Subba Ramanna Bodda 2024-10-14 15:20:08 UTC
Thanks a lot! I have checked logs go off and on fine with and without debug level 10. Have seen some lesser performance numbers though. We will also need "TALLOC_FREE(frame);" I think. I am checking and testing.
Comment 4 Subba Ramanna Bodda 2024-10-15 07:55:31 UTC
Created attachment 18477 [details]
Patch v1
Comment 5 Subba Ramanna Bodda 2024-10-15 07:56:37 UTC
With the TALLOC_FREE added as in attached patch, able to get similar performance numbers as deleting the commits. Also, works well in terms of debug on and off. Thanks a lot for the help!