389-ds-base

#51085 Issue 51076 - remove unnecessary slapi entry dups
Closed by spichugi. Opened by mreynolds.
mreynolds/389-ds-base issue51076  into  master

Download 51085.patch

Description:

So the problem is that slapi_search_internal_get_entry() duplicates the entry twice. It does that as a convenience where it will allocate a pblock, do the search, copy the entry, free search results from the pblock, and then free the pblock itself. I basically split this function into two functions. One function allocates the pblock, does the search and returns the entry. The other function frees the entries and pblock.

99% of time when we call slapi_search_internal_get_entry() we are just reading it and freeing it. It's not being consumed. In these cases we can use the two function approach which eliminates an extra slapi_entry_dup(). Over the life time of an operation/connection we can save quite a bit of mallocing/freeing. This could also help with memory fragmentation.

ASAN: passed

relates: https://pagure.io/389-ds-base/issue/51076

Metadata Update from @mreynolds:
- Request assigned

Combined with the previous patch here are some numbers doing a search, mods, and deletes with and without these patches:

Search (bind as user)
=========================
No Patch:  6 dups
Patch:     4 dups
Modify (change generic attribute)
=========================
No Patch:  8 dups
Patch:     5 dups
Modify (MO plugin - add member to group)
=========================
No Patch:  16 dups
Patch:     10 dups
     *  Biggest change of all the ops I tested
Delete
=========================
No patch:  5 dups
Patch:     3 dups
Delete (RI plugin - delete a member of a group)
=========================
No Patch:  9 dups
Patch:     8 dups
Delete (MO plugin - delete group)
=========================
No Patch:  10 dups
Patch:      8 dups

This all looks reasonable to me, but I think I'd like @tbordaz to check too. I assume this has passed tests?

Here are the perf tests:
(4 worker threads, virtual attributes are off, MEP is off, db env is on /dev/shm)
https://fedorapeople.org/groups/389ds/ci/pr51085/search.html
https://fedorapeople.org/groups/389ds/ci/pr51085/modify.html
https://fedorapeople.org/groups/389ds/ci/pr51085/search-and-mod.html
https://fedorapeople.org/groups/389ds/ci/pr51085/auth.html

Nice performance boost. It looks it gives a 10%-15% improvement on the search path (excluding auth impact and with same #workers). From the tests, I have a doubt about disabling virtual attribute, IMHO it is not realistic for most of the deployment because some users may ignore what is a virtual attribute and if it is safe to disable them.

Here are the perf tests:
(4 worker threads, virtual attributes are off, MEP is off, db env is on /dev/shm)

Not to see be too annoying, but I think too many changes were made in this test. In this case I'd like to see just my patch in place. Since I cleaned up the extra dupping in MEP I'd like to leave that plugin on. So it would be great to see:

  • Just my patch
  • Just /dev/shm set (no patch)
  • Just vattr off (no patch) - I wonder since Thierry changed the locking if this is still a factor.

Sorry for the delay. I had to rerun tests several times, because changing db home dir during the tests screwed up my ds instance several times.
Anyway, here are the requested results:
https://fedorapeople.org/groups/389ds/ci/pr51085_v2/search.html
https://fedorapeople.org/groups/389ds/ci/pr51085_v2/modify.html

And the same with 4 worker threads:
https://fedorapeople.org/groups/389ds/ci/pr51085_v2/search%20(tn=4).html
https://fedorapeople.org/groups/389ds/ci/pr51085_v2/modify%20(tn=4).html

Doesn't seem to really have any significant impact on performance, but it's still a lot less dupping/freeing for every operation.

Turning off vattrs still seems to have the biggest impact.

Right, those perf measurements open more questions than it answer ! Anyway it is very precious results. Thanks

Regarding vattr impact, do the searches ask for all attributes or a specific set ?. If requesting only dn, it should be similar to vattr-off.

In search tests givenName, sn and mail are requested.

I still think we should push this commit, can someone please review it and give it an official Ack (or nack)?

to be honest the fix in mep.c is complex to be evaluated.
entry_pb and 'e' are used in several places. In addition apply_mods are done on the entry 'e' that looks unexpected as your change is for read-only entries.
Also 'e' can be saved in config_copy->template_entry, it is not clear if this reference will be used after you clear the pb_results.

In short, this specific part needs more time to be reviewed. Could it be moved out from your patch ?

The rest of the patch looks okay to me. Need a second looks at mep.c

rebased onto 68ab6a809a2a46fbdb6fb880b1e79f99b09457b1

to be honest the fix in mep.c is complex to be evaluated.
entry_pb and 'e' are used in several places. In addition apply_mods are done on the entry 'e' that looks unexpected as your change is for read-only entries.
Also 'e' can be saved in config_copy->template_entry, it is not clear if this reference will be used after you clear the pb_results.
In short, this specific part needs more time to be reviewed. Could it be moved out from your patch ?

You're right there was one area where it was not safe where it get consumed into the config. I could just dup the entry in that case, but I just removed it all. Please review...

I already reviewed the previous patch and had a concern only for mep.c
As you postpone the change of mep.c, the rest of the patch looks good to me. ACK

Pull-Request has been merged by mreynolds

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4138

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
mreynolds
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.