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 _______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel