From kde-pim Fri Apr 11 22:30:38 2008 From: Marc Mutz Date: Fri, 11 Apr 2008 22:30:38 +0000 To: kde-pim Subject: Re: [Kde-pim] KDE/kdepimlibs/akonadi Message-Id: <200804120030.43769.marc () klaralvdalens-datakonsult ! se> X-MARC-Message: https://marc.info/?l=kde-pim&m=120795298811210 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 to QSet 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. > > > > > > > > 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. 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@klaralvdalens-datakonsult.se, mutz@kde.org Klarälvdalens Datakonsult AB, Platform-independent software solutions _______________________________________________ KDE PIM mailing list kde-pim@kde.org https://mail.kde.org/mailman/listinfo/kde-pim KDE PIM home page at http://pim.kde.org/