QIcon::fromTheme(xxx, someFallback)

[Olivier Goffart]

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 would also approve a change to Qt to add an overload of QIcon::fromTheme 
with only one argument without a fallback. (this would be binary compatible,  
and source compatible unless someone took the address of QIcon::fromTheme)

Otherwise, the only solution I come with is indeed to improve KIconEngine's 
cache to query quickly if the file is there or not.

-- 
Olivier