Review Request: little faster sycoca

[Josef Weidendorfer]

On Wednesday 28 September 2011, Jaime Torres Amate wrote:
> 
> > On Sept. 28, 2011, 8:59 a.m., Josef Weidendorfer wrote:
> > > I would expect that the ->constEnd() method completely gets inlined with compiler optimization turned on. So are you sure your change
> > > improves anything for a release build? Anything performance related always needs to be backed by numbers from a release build. Numbers from a debug build are useless.
> 
> I'll check again, but I think I compile with -O2 -g (RELEASE_DEBUG).

Looking at qlist.h, QList::constEnd() can be inlined, and just returns p.end()
where p.end() again is inlinable, and just returns the sum of the values. So
any reasonable compiler should inline that. But if it's inlined, callgrind
would not show calls to constEnd.
That said, your loop change is as easy to read as the original, and slighty better,
so that's fine.

> In any case, it is a minor improvement compared to the use of QBitArray.count,

That's for sure worth it.

> and the removal of a for loop (I'm checking it this has been this way since the beginning, or if fixing it makes other things faster) as Rolf has pointed.

I do not see how that loop can be removed. He probably misread the inner
condition of the first loop as "if (pos < 1 ", but it is actually
"if (pos < entry.length", and pos can be any position in the string.

Anyway, as I understand it, this function is part of an algorithm trying to
search a hash function with as less conflicts as possible. For that, it does
not really matter to have a solution within 90% of the best one. And then,
doing statistics should be fine if this function really takes much time.
Ie. only look at every 2nd/3rd string and also only at every 2nd/3rd position.
I don't think it makes much difference.

Josef


> 
> 
> - Jaime Torres
> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102723/#review6881
> -----------------------------------------------------------
> 
> 
> On Sept. 27, 2011, 8:42 p.m., Jaime Torres Amate wrote:
> > 
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://git.reviewboard.kde.org/r/102723/
> > -----------------------------------------------------------
> > 
> > (Updated Sept. 27, 2011, 8:42 p.m.)
> > 
> > 
> > Review request for kdelibs.
> > 
> > 
> > Description
> > -------
> > 
> > Using this way to build a for loop: for (const_iterator it=list.constBegin, end=list.constEnd ; it != end; ++it)
> > and replacing the count of bits with the QBitArray method for doing that.
> > callgrind says:
> > before: 2984 calls to constBegin, 0,00%. 2960531 calls to constEnd, 2.33%
> > after: 2921 calls to constBegin, 0,00%. 2921 calls to constEnd, 0.00%
> > 
> > before: calcDiversity, 55.83%  (debug enabled)
> > after: calcDiversity, 14,46%  (debug enabled)
> > 
> > buildsycoca is still not faster than light, but it is only a four lines patch.
> > 
> > 
> > Diffs
> > -----
> > 
> >   kdecore/sycoca/ksycocadict.cpp 17ed9ac 
> > 
> > Diff: http://git.reviewboard.kde.org/r/102723/diff/diff
> > 
> > 
> > Testing
> > -------
> > 
> > 
> > Thanks,
> > 
> > Jaime Torres Amate
> > 
> >
> 
>