QIcon::fromTheme(xxx, someFallback)

[Volker Krause]

On Friday 21 August 2015 10:53:29 Olivier Goffart wrote:
> On Friday 21. August 2015 10:05:09 David Faure wrote:
> > Hi Olivier,
> > 
> > I've been trying to fix performance problems with QIcon::fromTheme("foo")
> > when using KIconEngine, and I'm hitting a wall due to the QIcon API - more
> > precisely, that fallback argument.
> > 
> > KMail (and all similar large apps) calls QIcon::fromTheme() for many
> > actions on startup, most of which are not visible to the user (until
> > opening a menu, for instance). KIcon("foo") was fast because it did
> > nothing until the icon had to be rendered on the screen. This would
> > almost be true for QIcon::fromTheme(), if not for the commit below (which
> > has been improved since, but still), due to that "fallback" argument.
> > 
> > The existence of that fallback argument in QIcon::fromTheme kills any
> > hopes
> > of completely delaying the looking up of the icon, since we have to find
> > out right away (in availableSizes()) whether the icon exists on the
> > filesystem or not. And we're talking about app startup, so any in-memory
> > cache doesn't help here (we do have an on-disk cache as well, but it's not
> > filled in for icons that aren't getting rendered (see
> > https://git.reviewboard.kde.org/r/124811/) ).
> > 
> > I think the fallback argument is a major flaw in the QIcon::fromTheme API.
> > We almost never need it (except in that bug report below, apparently), but
> > we pay the price for it for every single icon.
> > And of course a fast path when the fallback is null does not help (it was
> > my first try) because we still need to find out whether to return null or
> > not.
> > 
> > Do you see a way to have a QIcon::fromTheme equivalent which allows
> > fully delayed loading? I don't have a good solution right now, but I think
> > the only good solution would be something around another Qt method,
> > despite that being possibly confusing...
> > 
> > Do you agree? Any better idea?
> > 
> > Thanks.
> > 
> > PS: when I make KIconEngine::availableSizes skip checking (i.e. basically
> > reverting your commit below), KMail uses 30% CPU less and starts up almost
> > instantaneously, reports Volker. So this is a real optimization worth
> > doing.
> > 
> > PS2: if all else fails, we could add a separate persistent cache for
> > hasIcon() (names only, no pixmaps).
> > But it would still be much faster to make it all really delayed, i.e.
> > without fallback at creation time.
> 
> Hi David,
> 
> Tahnks for looking into this issue.
> 
> Yes, QIcon::fromTheme is used with a fallback by some application. Several
> applications were affected by the problem. There are many bug reports for
> several applications.  Qt bug: QTBUG-43021
> And also the reason i came to fix this bug is because another application i
> work with had broken icons.
> 
> 
> Another question is how usefull is KIconEngine? What adventages does it
> provide over the normal QIconLoader?  I only know about the shared cache.
> Is the shared cache really that usefull? QIcon itself already hase a cache,
> so the icons are in two caches.  And maybe the lookup could indeed be
> improved in QIconLoader itself by using a cache there (or a shared cache
> there).
> 
> If that's the only reason we could disable the KIconEngine for the time
> being.

I measured KMail with the KDE platform plugin deleted, the result is similar 
to the KIconLoader case, 50k QDir::exists and 58k QFile::exists calls.

KIconLoader gets to 188k QFile::exists calls. The main reason for the 
difference seems to be that KMail adds five extra search paths to KIconLoader 
(down from 10 yesterday, that's why the total numbers are lower by now than 
mentioned initially).

So, to me it looks like they don't differ much, cost-wise. Even if QIconLoader 
would not get more expensive with extra search paths added we are still 
looking at 15% CPU cost in total there, for starting KMail, opening a medium 
sized folder and closing it again.

regards,
Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 173 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150821/c1680129/attachment.sig>