memberOf plugin: mbof_append_muop() optimization#8551
Conversation
alexey-tikhonov
added
backport-to-sssd-2-9
Performance
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new inline helper function, sss_linearized_dn_match, to optimize the comparison of linearized distinguished names (DNs) within mbof_append_muop and mbof_append_addop. The new function aims to handle specific case-sensitivity rules for DNs, particularly for the 'name' attribute. However, a critical review comment points out that the current implementation's use of strcasecmp for DN comparison is incorrect and unsafe, as it fails to properly account for the complex case-insensitivity rules of DN components, potentially leading to false negatives for equivalent DNs. The reviewer suggests either reverting to ldb_dn_compare or implementing a more robust comparison logic.
alexey-tikhonov
changed the title
mbof_append_muop() optimizataionmbof_append_muop() optimization
|
/gemini review b0a82c0 |
There was a problem hiding this comment.
Code Review
This pull request introduces optimizations to the memberof LDB module by adding a hash table for efficient duplicate value checking and a new sss_linearized_dn_match function for improved DN comparisons. A critical issue was identified where the val_table pointer in mbof_memberuid_op might be uninitialized, potentially leading to a crash when hash_has_key() is called.
b0a82c0 to
ce7c304
Compare
| /* we already have this value, get out*/ | ||
| return LDB_SUCCESS; | ||
| if (!op->val_table) { | ||
| hret = hash_create_ex(1024, &op->val_table, 0, 0, 0, 0, |
There was a problem hiding this comment.
SSSD code base in general inits hash table with '0', but 'memberof' plugin:
ldb_modules/memberof.c:3111: ret = hash_create_ex(1024, &mod_ctx->igh->inherited_gh, 0, 0, 0, 0,
ldb_modules/memberof.c:4023: ret = hash_create_ex(1024, &ctx->user_table, 0, 0, 0, 0,
ldb_modules/memberof.c:4122: ret = hash_create_ex(1024, &ctx->group_table, 0, 0, 0, 0,
so kept 1024 for the sake of consistency.
Justification is the same as in 704c31d In certain scenarios this function is a hot path and using heavy `ldb_dn_compare()` adds unnecessary overhead. Reviewed-by: Alejandro López <[email protected]> Reviewed-by: Justin Stephenson <[email protected]>
Justification is the same as in 704c31d In certain scenarios this function is a hot path and using heavy `ldb_dn_compare()` adds unnecessary overhead. Reviewed-by: Alejandro López <[email protected]> Reviewed-by: Justin Stephenson <[email protected]>
Replace O(N) linear `strcmp` scan over `op->el->values[]` with O(1) hash table lookup for duplicate name detection. Implementation assisted-by: Claude Code (Opus 4.6) Reviewed-by: Alejandro López <[email protected]> Reviewed-by: Justin Stephenson <[email protected]>
ce7c304 to
e834d1f
Compare
4 participants
Testing with a following setup: 300 subgroups that all share the same 3000 users, plus one top-level
group containing all subgroups.
ignore_group_members = falsetime id userwith a cold cache:ldb_dn_compare()inmbof_append_addop()#8546 as "vanilla" build: ~14..15 secs,mbof_append_muop()accounts for ~32%mbof_append_muop()accounts for ~1%(to be rebased on top of #8546)