[Kde-pim] KDE/kdepimlibs/akonadi

[Marc Mutz]

On Friday 11 April 2008 19:29, Volker Krause wrote:
> On Friday 11 April 2008 12:42:30 Marc Mutz wrote:
> > On Friday April 11 2008 09:46, Volker Krause wrote:
> > > On Friday 11 April 2008 08:55:45 Marc Mutz wrote:
> > > > On Thursday April 10 2008 17:35, Tobias Koenig wrote:
> > > > > SVN commit 795521 by tokoe:
> > > > >
> > > > > Change all QList<QByteArray> to QSet<QByteArray> for itemParts
> > > >
> > > > This is a bad idea. A set doesn't pay it's weight for small numbers
> > > > of items. In the vast majority of cases, it's faster to use a sorted
> > > > vector/qlist instread of a set. A set is node-based, while a (q)list
> > > > is simply an array. Locality of reference and malloc-free appends
> > > > ruin set's day.
> > >
> > > Right, performance-wise it's probably not the best choice, but it
> > > provides exactly the semantics we want here: no duplicates, order
> > > doesn't matter. That's why we decided to use it instead of a list. The
> > > performance impact should be limited since these sets are not created
> > > very frequently (which is the expensive part AFAIU), but mostly passed
> > > around and checked to contain a specific value. So, I would rather
> > > enforce correctness here and keep the set, especially since this is not
> > > an implementation detail but public API.
> >
> > <snip>
> >
> > Right, the passing around is not a problem. Neither is the creation, if
> > it's done infrequently. It's the checking that counts here. Checking is
> > much slower in sets than in sorted vectors. Locality of reference. It
> > might be a good idea to not use a naked container here, but wrap it in a
> > class:
>
> Tobias actually meassured it, running contains() 10^5 times on a list and a
> set containing 5 elements each (which are typical numbers for our use
> case). The result was 7-9ms vs 9-11ms. So, the list is indeed faster, but
> the absolute times are so small that it's not worth the effort to optimize
> anything here.
<snip>

If there's a factor of 25% slowdown already with _warm_ L1 cache, there's 
going to be much more with cold cache. However, that's not the point. There 
are two points:

The first point is that this is a premature pessimisation. There is no effort 
to de-pessimize it, it's just an svnrevertlast away. Premature pessimisations 
typically don't add up to much (try measuring the difference between 
pass-by-value and pass-by-const-&, eg., or ++it vs. it++), but they're easy 
to avoid.

The second, and arguably more important one, is that a naked set (or list) is 
a very bad abstraction for what you're trying to do. Are the QByteArrays 
case-insensitive? What _is_ the QByteArray, actually? Can I add new ones?

The class fragment that you snipped is much more straightforward to use, and 
shields the users from details like "do I need to lowercase the argument to 
contains()?"

In short: you're trying to use a container as something _more_ than a 
container. Simple OOP common sense tell us to make it a class with a 
well-defined and targeted interface instead, _especially_ since it's public 
API.

Thanks,
Marc

-- 
Marc Mutz -- marc at klaralvdalens-datakonsult.se, mutz at kde.org
Klarälvdalens Datakonsult AB, Platform-independent software solutions
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/