The idea sounds good, even if it only reduces the locking for base searches. Do you have any numbers from tests ?

The implemention is, as far as I checked, also ok. Do you need the calls to target_entry_return on all the places or would it be sufficient after the switch/break and at free_and_return ?

Thank you lkrispen for your review. I need to check op_shared to see if target_entry_return could be called only once. I was afraid of missing the one that is at the end of the switch and called it at each goto/break. But you are right it can possibly done later in the function.

Regarding the numbers here is what I got on local reproducer

A regular user dereferencing a group of 1000 members, retrieving 4 attributes from the members
It shows the number of client SRCH returned in N seconds
 with the patch                   main
             with 200 clients   
0 sec : 0                       0 sec: 0
1 sec : 42                      1 sec: 1
2 sec : 47                      2 sec: 0
3 sec : 52                      3 sec: 1
4 sec : 57                      4 sec: 4
5 sec : 2                       5 sec: 9
                                6 sec: 121
                                7 sec: 65
            with 100 clients    
1 sec : 11                      1 sec: 1
2 sec : 84                      2 sec: 2
                                3 sec: 20
                                4 sec: 78

I think one place is probably not enough, since on error you either break out of the switch and continue the loop, or goto error_return. So there might be two places.

The numbers look really promising, and I would after some more QE and a customer feedback (hotfix) vote to include it

What does this do in the case that this if statement fails because that would leave **e as an invalid pointer wouldn't it?

@lkrispen, the calls to cache_return_target_entry at 'break' in the switch can be removed. Indeed on 'break' it will fall through the end of the switch and will call target_entry_return.

The problem with others calls to target_entry_return is that it requires a backend (be) and preferably after the be->be_search where the target_entry has been found/acquired.

The code can jump to free_and_return before the loop (where the backend is defined), inside the loop but before and after the call be->be_search
I think the following test should work but it adds complexity to the various paths that can lead to 'free_and_return'

static void
cache_return_target_entry(Slapi_PBlock *pb, Slapi_Backend *be, Slapi_Operation *operation)
{
    if (be && operation_get_target_entry(operation) && be->be_entry_release) {
    ...

@firstyear, this function is called in the phase to send back entries. At this time, all entries except the base search (target entry), have been acquired (in id2entry) in that phase and should be return in that phase also.

The base search entry is now managed at the operation level (opshared) where it is acquired in the phase where candidates list is prepared and is released (cache_return_target_entry) when all entries have been returned.

In short, if the statement fails (the entry is the base entry) it does not return the entry (leave refcnt unchanged). The entry will be returned at the upper level (opshared).

It might be worth adding a comment like that into the code then? I guess I feel a but un-easy about any function that doesn't always set pointers to valid values is all.

Just an update regarding performance tests.

I have been working to make an automatic test case and I realized that statement https://pagure.io/389-ds-base/pull-request/50543#comment-94174 was comparing two runs with a different number of worker and that was wrong.

I did several tests changing the number of workers. Basically the more there are workers the more likely we can see contention. I updated the test case to bind with DM. As soon as we eliminate ACI as RC of the perf issue, binding with a specific user impacts the performance results.

I can attach the results (bound with directory manager, applying 1000 deref search)
It shows 3 runs with 200, 100 and 30 workers.
The left columns shows result of the patch and the right columns the result with master branch. Each runs present the throughput (#deref_search/second) and layout of the response time. For example '1 sec: 475' means that 475 deref_search out of 1000 last 1 sec.

    200 workers
            1000 SRCH
(55/sec)                      (52/sec)
0 sec : 473                     0 sec : 4
1 sec : 475                     1 sec : 101
2 sec : 54                      2 sec : 455
                                3 sec : 421
                                4 sec : 21
avg: 0.6s                       avg: 2.3s
    100 workers
            1000 SRCH
(52/sec)                       (52/sec)
0 sec : 111                     0 sec : 15
1 sec : 527                     1 sec : 284
2 sec : 356                     2 sec : 599
3 sec : 8                       3 sec : 104
avg: 1.2s                       avg: 1.8s
    30 workers
            1000 SRCH
(62/sec)                        (55/sec)
0 sec : 780                     0 sec : 66
1 sec : 222                     1 sec : 382
                                2 sec : 454
                                3 sec : 100
avg: 0.2s                       avg: 1.6s

In conclusion, with the patch :
- throughput may increase up to 15%
- etime is faster by 30% to more than 100%

Now I am working to complete the automatic testcase

I suggest to push the patch, it would allow to provide a hotfix and get feedback, and would undergo the QE tests. It can still be refined when the automatic test case is finished

@lkrispen , @firstyear, if you are okay with the attached patch, would you be okay to push it upstream without a finalized testcase. ?
I am working on the testcase but making slow progress it will likely not be ready before end of the month.

I think I want to have another review to be sure (but probably okay). I'd just want to confirm this passes other tests in our system and passes under ASAN, but if that is true then it's okay for you to proceed :) . Is there a ticket for the test case? Or just part of the existing ticket number?

I guess we could PR_ASSERT(op) here too?

Anyway, besides that I think this is okay to merge. We'll see if @lkrispen has more to say :)

Running replication asan tests @lkrispen discovered a bug in the patch.
The in memory RUV entry has no 'entryid', so when it is added in the operation target_entry it is added with a id = '0'. It triggered a leak.
@lkrispen found the fix. Instead of storing target_entry_id=entry[entryid] it stores 'entry.ep_id' that is actually the key of the entry in the entry cache hash.
That fix is slighly faster than previous fix but it is not noticeable during a run.

@lkrispen, @firstyear , @vashirov thank you very much for your reviews and help to run the tests.

Ack, just a minor remark: the comment in slap.h where o_target_entry_id is defined it still sounds like entryid. And ep_id is ID is u_int32_t - so you shouldn't need the cast

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/3599

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.