Fwd: Regression: ldb performance with indexes

[Andrew Bartlett]

On Fri, 2024-03-22 at 17:55 +0100, Andréas LEROUX wrote:
>     Hi Andreas and Andrew,
> 
>     
> 
>     >>>> > Hi,my colleagues discovered a performance
>     issue in libldb:
> 
>     >>>> >
>     https://bugzilla.samba.org/show_bug.cgi?id=15590
> 
>     >>>> > > > > As soon as you use indexes,
>     ldbadd will be magnitudes
> 
>     >> slower than
> 
>     >>>> > itwas before.Could some ldb expert please look
>     into it?
> 
>     >>>> > > Your subject says a regression. What
>     version is this a
> 
>     >>>> regressionagainst?
> 
>     >>>> Isn't that obvious from the bug report?
> 
>     >>> Here is the short summary:
> 
>     >>> $ bash repro.sh 20000 indexesAdded 2 records
>     successfullyAdded
> 
>     >> 20000
> 
>     >>> records successfully
> 
>     >>> On Samba 4.10: 0m01.231sOn Samba 4.19: 1m30.924s
>     (that's 90 times
> 
>     >>> slower)
> 
>     >>>> > The very nature of a DB index is that it will
>     take time to
> 
>     >>>> create,possibly a lot of time, but should make
>     reads faster.
> 
>     >>>> Either the DB index doesn't work at all in Samba
>     4.10 or there
> 
>     >> is a
> 
>     >>> huge performance problem in Samba 4.19. What is it?
> 
>     >> 
> 
>     >> Thanks, that wasn't written as obviously on the bug, thanks
>     for the
> 
>     >> clarification.
> 
>     > 
> 
>     > I used our CentOS 8 Stream CI image for bisecting. You can't
>     bisect
> 
>     > easily on a modern Linux Distribution, as the included waf
>     would not
> 
>     > have support for newer Python versions like 3.12.
> 
>     > 
> 
>     > In case you want to reproduce it, here is my run:
> 
>     
> 
>     I'm Andréas from Tranquil IT dev team. Denis and Yohannès asked
> me
>     this week to take a look at the performance issues on large
> domains,
>     which include this issue in the current thread along the mdb
> large
>     transaction issues.
> 
>     
> 
>     The attached patchset goes through all the tdb and ldb make test.
> 
>     
> 
>     * LMDB : increase MDB_IDL_LOGN from 16 to 18 to accomodate large
>     nested transactions
> 
>     * tdb : fail-fast when record hash doesn't match expected hash to
>     avoid to read/copy the entire record
> 
>     * ldb : increase DEFAULT_INDEX_CACHE_SIZE from 491 to 8089 to
>     increase the number of bucket to have smaller bucket to have
> faster
>     iteration in each buckets in tdb_find
> 
>     
> 
>     With this patchset we can upgrade large domains (>200k
>       objects) to FL2k16 level in approximatly 1 hour instead of 3
> days
>       :-) 
> 
>     
> 
>     [root at srvads1-bl1cw ~]# bash repro.sh 20000 indexes
> Added 2 records successfully
> Added 20000 records successfully
> real    0m0.536s
> user    0m0.798s
> sys     0m0.105s
> 
>     Tranquil IT team is expert at deploying Samba-AD in large
>       domains, but we are not core devs, so I may have missed
> something
>       during my debugging / patching session. Don't hesitate to
> comment
>       and tell me what you think about this patchset, if there are
> some
>       pitfalls that I missed or if the style can be improved.
Firstly, these are very impressive improvements.

Thanks so much for your work debugging this and getting to the root of
the problem, this is very much appreciated. 
Do you have any data on how much of the improvement is due to ldb
patch, and how much is due to each of the other patches? 

Did you happen to use Brendan Greg's FlameGraph tool for the debugging
(this is what we often use), and if so, can you share the graphs?  I
want to understand if perhaps we need to consider restructuring the
caller. 
https://www.brendangregg.com/FlameGraphs/cpuflamegraphs.html#Instructions

For this series if you could please:
 - send in a send in a Samba Developer Declaration per 
https://www.samba.org/samba/devel/copyright-policy.html
 - create a gitlab account
 - let me know the username

Once I give you access to the devel repo, so you can run the full
testsuite under our quota, please follow our contribution steps here:
https://wiki.samba.org/index.php/Contribute#Subsequent_Merge_Requests_(and_complex_first_requests)

Create one merge request for each of the two patches.  I realise that
seems overkill, but the LMDB cache size change is much easier to
justify in the interim. 

This will help ensure we have this as a fully checked merged request
that we can get into the tree.

Of course the LMDB patch needs to go to upstream, but is of course most
valued. 

Andrew Bartlett
-- 

Andrew Bartlett (he/him)       https://samba.org/~abartlet/Samba Team Member (since 2001) https://samba.orgSamba Team Lead                https://catalyst.net.nz/services/sambaCatalyst.Net Ltd
Proudly developing Samba for Catalyst.Net Ltd - a Catalyst IT group
company
Samba Development and Support: https://catalyst.net.nz/services/samba
Catalyst IT - Expert Open Source Solutions