[Freeipa-devel] [PATCH] Improve performance of get_group_sids()

[Simo Sorce]

On Tue, 2012-07-10 at 23:04 +0200, Sumit Bose wrote:
> Hi,
> 
> the following two patches are the first step to fix
> https://fedorahosted.org/freeipa/ticket/2881. Unit tests with time
> measurements are added and the performance of the get_group_sids()
> function is improved by an order of magnitude.
> 
> The caching of the LDAP results is still missing. I will finish this
> after my PTO. But I haven't started to work on this. So if you think it
> should be fixed earlier feel free to take the ticket.

Nack, for minor issues.

Patch1:
- Please create a tests/ directory in daemons/ipa-kdb for the tests

- Please fit the whole file within 79 columns, there are a number of
lines towards the end of the tests that go beyond that.

- There is a spurious comment at the top:
+/* talloc leak checks written by Martin Nagy for sssd */

- please add a dlinkslist.h file to /util instead of copying macros in
the tests file

Patch2:

- Needs spacing in arithmetic operations. This line:
+    idx = group_sids_buf+(p*sid_len);
should be:
+    idx = group_sids_buf + (p * sid_len);
and other similar cases.

- Should we align the sids in the big memory allocation you make ?
+    dom_sid_str_len = strlen(dom_sid_str);
+    sid_len = dom_sid_str_len + 12;
this can result in any alignment, should we align32/64 sid_len ?
It would waste a bit of space, but may be beneficial on some archs.

- Please fit the whole file within 79 columns, there are a number of
lines that go beyond that.

- A Rid is a 32 bit unsigned, no need to cast to (unsigned long), just
declare the format string as %u (unsigned)

- Not introduced with this patch, but can you remove most of the debug
stuff going to syslog ?
I do not think we want to spam syslog with this low level stuff, we must
not confuse debugging with logging like it happens in samba-land.
If you really want to retain that stuff, wrap it around a macro that is
enabled only with a debugging option passed to configure or make.


Simo.

-- 
Simo Sorce * Red Hat, Inc * New York