Silly me, left rust comments here. I'll clean that up.

I won't be able to complete any more testing til early next year due to travel (but I'll still be working).

note to future william: test with various server thread and client cthread combinations.

Okay, I'm going to work on this again shortly, but still happy for people to review this change @tbordaz and @mreynolds especially :)

Rebased to latest master, @tbordaz can you have a look at this? I have some more testing to do about the perf, but I'd appreciate your code review anyway.

Okay, so @tbordaz and @mreynolds I did another test, where I set the conntable to only have 8192 slots, then I used a remote machine with 128 threads to ldclt against it. The difference was:

w_ freelist
ldclt[31972]: Global average rate: 1889.02/thr  (2417.95/sec), total: 241795
w_out freelist
ldclt[32172]: Global average rate: 1667.45/thr  (2134.34/sec), total: 213434

So if that's correct, it's actually about a 12% improvement under highload :)

Anyway, at this point I'm pretty happy with that, so I'll ask you to do a review and decision on merging now!

The approach looks good to me, but I have a few questions. The change gets rid of eg checking "connection_is_free(..,use_lock,..)" which might be ok since we take it from the free list.
But the check in connection_is_free() also takes into account the refcount, if one op is done the conenction might not yet be free.

What I would like to see is these two types of test:
- clients with asynchronous operations, where multiple ops on the same connection can overlap
- more client connections attempting to connect than slots in the connection table, to see if exhausting and recovering of the conn table or free list works

@lkrispen As always, these are good points.

Yes, we no longer need to use the connection_is_free, because the only method for a connection to be in the freelist, is for it to have passed through connection_table_move_connection_out_of_active_list(), which is the final step in making the connection free for re-use. Specifically I account for this checking on line 413 that refcnt == 0 with PR_ASSERT for our development builds to assert we are in the correct state.

A different framing is that for connection_is_free() to assert to true, the connection must have been through connection_table_move_connection_out_of_active_list(), so we no longer need to walk the table and always check is_free, because of the freelist.

I can also do some more testing with these extra scenarios - here's hoping I don't melt my home development server with all these threads :)

Okay, so let's look first at "more connections that slots". This is really showing the ability for the connection table to recycle short lived connections (ie bind, single search) as fast as possible, and really, what this patch really aims to improve.

To do this I used ldclt with it's max thread parameter (1000) against instances with a conntable max of 800.

w_out freelist
Global average rate:  867.41/thr  (8674.12/sec), total: 867412
w_ freelist
Global average rate: 1137.41/thr  (11374.13/sec), total: 1137413

Additionally, 139 of the 1000 threads on the w_out freelist version were "unable to connect at all" causing those ldclt threads to prematurely terminate. This is a great sign in being able to handle much higher incoming load effectively.

Repeating the test with 512 connection table slots, and 1000 threads the following occured.

w_out freelist
Global average rate:  875.00/thr  (8749.99/sec), total: 874999
w_ freelist
Global average rate: 1123.85/thr  (11238.51/sec), total: 1123851

Again, 155 threads failed early, but the freelist was able to cycle the connections so quickly that it did not drop any connections.

To really stress this I tried with 128 conntable slots from 1000 threads:

w_out freelist
Global average rate:  855.68/thr  (8556.81/sec), total: 855681
w_ freelist
Global average rate:  850.29/thr  (8502.91/sec), total: 850291

Without the freelist, 546 connections were dropped, with the freelist only 103 were dropped.

This shows that under high pressure, even a ratio of 7 clients:1 conntable slot, the freelisted connection table is able to allocate connections at such a high rate that many more clients can be served in extreme load conditions compared to the current strategy. This ability to sustain under load is really critical and what will make a massive difference. Compared, the current strategy could only maintain about 3:1.

For the async operation test, I set ldclt to have -a 2 for two async searches at a time, with a connection table of 512 and 512 client threads.

/opt/dirsrv/bin/ldclt -h lt2.dev.blackhats.net.au -p 389 -a 2 -n 512 -N 10 -f '(uid=user_XXXXXXX)' -e bindeach -e esearch,random -r1 -R6000 -I 32 -e randomattrlist=cn:uid:ou
w_out freelist
Global average rate:    4.00/thr  ( 20.48/sec), total:   2048
w_ freelist
Global average rate:    4.00/thr  ( 20.48/sec), total:   2048

This was really weird to see as a result, and maybe there is something about it I do not understand - but it does at least show with async (longer lived connections) we are "no worse" than before. If you have any insights on how to test this more effectively @lkrispen I'd be keen to hear it.

Hope that helps,

Sorry for the late review. The idea, implementation and benchmark are very nice !
I have two comments:

• regarding drop of connection_is_free. At the moment if a connection being "refcnt=0 and sd=invalid_socket and !closing" is not connection_table_move_connection_out_of_active_list, it is however recycle as a free connection. With the patch, the connection will "leak". Is this case impossible ?, should we create a cleaning thread that recycle those connections ?

• the way we go through the CT in connection_table_as_entry (cn=monitor) could be change taking benefit of the the new freelist

Sorry for the late review. The idea, implementation and benchmark are very nice !

Thanks for the review :)

I have two comments:

regarding drop of connection_is_free. At the moment if a connection being "refcnt=0 and sd=invalid_socket and !closing" is not connection_table_move_connection_out_of_active_list, it is however recycle as a free connection. With the patch, the connection will "leak". Is this case impossible ?, should we create a cleaning thread that recycle those connections ?

No? When we move out of active list, that's when a connection is "ready to be reused", and that's how it worked before - the conditions we looked for in move out of active, were how we allocated on the table walk.

So now instead, we just use connection move out of active to define the connection is ready.

So I do not think a leak is possible here,

I think more likely, the best procees is actually to clean this up, because it's really confusing how it's design and unclear what states connections are in, that would probably help us in code reviews and modifications.

the way we go through the CT in connection_table_as_entry (cn=monitor) could be change taking benefit of the the new freelist

No, because cn=monitor uses the linked list that exists between connections already, so it isn't doing a conntable scan.

@firstyear, I agree that connection design is a bit fuzzy this is why I had concern regarding potential leak. Would you please add a comment that connection_table_move_connection_out_of_active_list is the only way to add back a connection on the freelist.

For cn=monitor, we go through the complete connection table, and if the connection is not free dump the connection info. So if we have a large connection table but few active connection the loop is a waste of time. Couldn't we go through thre freelist[conn_free..conn_next[ ?

Yep, I'll add that comment.

For cn=monitor, there is a linked list of active connections that exists between all the conn structs. It's already walked, so there is no benefit to using the freelist - there is an active list already.

https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/conntable.c#_331

There is also an iterator for it:

https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/conntable.c#_206

But yes, reviewing: https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/conntable.c#_404

I think you are right, this isn't used in cn=monitor.

Saying thatt the freelist doesn't help - there is an active list we should use instead. But I also think cn=monitor here is going to slow us down no matter what as we take the ct lock.

Comment added :)

@firstyear, do you mind to add the same comment in the description itself of connection_table_move_connection_out_of_active_list. The patch looks good to me. ACK

Regarding the cn=monitor being not optimal I think we should open an other ticket as it is not related to the new freelist.

@tbordaz https://pagure.io/389-ds-base/issue/50906

Comment added, merging now. Thanks again for your detailed review as always!

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

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.