QIcon::fromTheme(xxx, someFallback)

[David Faure]

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.

Yes, I'm sure. But the problem is: we're paying the price of the optional fallback
feature even when that feature is not used.

QIcon::fromTheme(x) (null icon as fallback) cannot know if the application wants
a null icon when x is not found, or if it's ok that isNull() doesn't return false, and that
some "unknown" icon is being displayed instead (which is KIconLoader's behavior).

Even if nothing was displayed instead, the performance gain comes from not having
to look up the icon on disk so that the returned QIcon can be null if not found.

This is why I'm thinking the only good solution would be a QIcon::fromTheme
overload (or a method with a different name) that doesn't have this semantic of
"null qicon if not found". 99% of the users of QIcon::fromTheme don't need this
semantic, and don't need the slowness that it creates.

> 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).

KIconLoader uses a persistent cache, a file on disk. QIconLoader doesn't have
that, so every startup is slow. But that's not even the point here, they both pay
the price of the QIcon::fromTheme return value constraint, so this discussion
is not even about KIconLoader vs QIconLoader. The problem applies to both.
 
> 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)

I thought about that, but that would change behavior of existing code, no?

Currently: QIcon::fromTheme("foo").isNull() is true if "foo" isn't found.
If we add a one-arg overload: QIcon::fromTheme("foo").isNull() would be false,
and QIcon::fromTheme("foo", QIcon()).isNull() would be true. The old behavior
(for these rare apps that care) would require adding an explicit null-icon second arg.
I wish this would have been done this way from the start, but we can't make
that change now.

What I need is QIcon::fromThemeFast("foo") :)
QIcon::fromThemeNoFallback()
QIcon::fromThemeV2()
Yeah I know, there's no good name for "work as it should have".

Well, there is one solution: QIcon("foo"), the existing fileName ctor,
if we add the fact that "if not found in the current dir then the icon
will be looked up in the theme". That means one stat() per QIcon,
but that's nothing compare to the current QIcon::fromTheme().

And from the documentation, it looks like QIcon(QString) already
behaves the optimal way, i.e. you don't get isNull() if the file doesn't
exist, because "The file will be loaded on demand." right?
That's exactly what I want for icons loaded from a theme, and
which QIcon::fromTheme() doesn't allow implementing, due
to its requirement to immediately find out if a null icon
(or a specific fallback) should be returned instead.

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5