QIcon::fromTheme(xxx, someFallback)

[David Faure]

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.

On Wednesday 18 February 2015 09:38:54 Olivier Goffart wrote:
> Git commit b0a6df6fbd9117b41a7f4e3bc861e20fbadb1956 by Olivier Goffart.
> Committed on 17/02/2015 at 14:24.
> Pushed by ogoffart into branch 'master'.
> 
> Fix QIcon::fromTheme(xxx, someFallback) would not return the fallback
> 
> because KIconEngine::availableSize always return something even if the icon
> is not there
> 
> BUG: 342906
> REVIEW: 122608
> 
> M  +9    -0    src/kiconengine.cpp
> 
> http://commits.kde.org/kiconthemes/b0a6df6fbd9117b41a7f4e3bc861e20fbadb1956
> 
> diff --git a/src/kiconengine.cpp b/src/kiconengine.cpp
> index 530403e..6dff533 100644
> --- a/src/kiconengine.cpp
> +++ b/src/kiconengine.cpp
> @@ -111,6 +111,15 @@ QList<QSize> KIconEngine::availableSizes(QIcon::Mode mode, QIcon::State state) c
>  {
>      Q_UNUSED(mode);
>      Q_UNUSED(state);
> +
> +    if (!mIconLoader) {
> +        return QList<QSize>();
> +    }
> +
> +    if (mIconLoader->iconPath(mIconName, KIconLoader::Desktop, KIconLoader::MatchBest).isEmpty()) {
> +        return QList<QSize>();
> +    }
> +
>      return QList<QSize>() << QSize(16, 16)
>             << QSize(22, 22)
>             << QSize(32, 32)

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