[PATCH] avoid calling casefold_internal if the dn is already casefolded

[Andrew Bartlett]

On Wed, 2012-12-12 at 14:02 -0800, Matthieu Patou wrote:
> On 12/12/2012 03:26 AM, Andrew Bartlett wrote:
> > On Tue, 2012-12-11 at 22:23 -0800, Matthieu Patou wrote:
> >> Hello,
> >>
> >> Can someone review this patch ?
> >>
> >> It comes from analysis of indexed ldb_search with scope = one or subtree.
> >> For each entries in the index we will compare the dn in the index to dn
> >> specified as base for the search. And we keep casefolding the later one
> >> which is useless and costly.
> >>
> >> With a hundreds of entries in the index the cost of cost folding is
> >> starting to be one of the most important one. With this patch we can
> >> avoid useless calls.
> > Did this really help?  I can't see how this is any different to the
> > dn->valid_case check we already do in ldb_dn_casefold_internal().
> >
> > Additionally, the reason that dn->valid_case needs to be checked is that
> > if the DN is changed, the casefolded version may not be valid, and may
> > need to be regenerated.  (ie, there is a correctness issue here as
> Looking at the code it seems that if the dn is changed not only 
> valid_case is changed but also casefold too or at least casefolded is 
> freed and nulled.
> So it seems to be correct still.

We should avoid looking inside the opaque struct ldb_dn.  I'm supprised
it is even available here, but we must be bringing in the private
headers as this is a core ldb module. 

You might be falling into an analysis trap, due to how the profiler
works.  If it finds that ldb_dn_casefold_internal() is expensive (the
timer finds that function in use a lot of the time), and that there are
a lot of calls to ldb_dn_casefold_internal() from the index code, it is
possible to make the false assumption that the calls from the index code
are the expensive calls.  I had similar things when I last ran perf, and
had to discount some of the call stacks as being fantasy. 

Otherwise, I don't know how to explain this, but you might want to
single step a simple case in gdb and prove it one way or the other.

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org