Discussion:
QIcon::fromTheme(xxx, someFallback)
(too old to reply)
David Faure
2015-08-21 08:05:09 UTC
Permalink
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.
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, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Volker Krause
2015-08-21 18:25:41 UTC
Permalink
Post by 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.
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
David Faure
2015-08-21 20:30:37 UTC
Permalink
Post by 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.
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, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
David Faure
2015-08-23 10:01:38 UTC
Permalink
Post by David Faure
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.
Ah ok, i first tought that the problem was the call to availableSizes.
But other problem is also the fact that we want to return a null icon if not
found.
It's the same problem, the QIcon code calls availableSizes in order to determine
if it should return the (null by default) fallback ;)
Fo this mean we could change QIcon::isNull fo be something like
if (!d) return true;
if (d->notNull) return false;
bool null;
d->virtualHook(CheckNull, &null);
if (null) {
const_cast(this)->operator=(QIcon())
} else {
d->notNull = true;
}
return null;
So than isNull is also lazy, and the problem is solved... is it not?
Ah!! Yes, that would work. Can you make the change in Qt, since you know that code best?
Post by David Faure
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().
That could be a solution.
Post by David Faure
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.
And I think this behaviour is also confusing.
Not sure what you meant here. QIcon("file") should be null if
./file doesn't exist?
--
David Faure, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Olivier Goffart
2015-09-07 13:53:31 UTC
Permalink
Post by David Faure
Post by David Faure
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.
Ah ok, i first tought that the problem was the call to availableSizes.
But other problem is also the fact that we want to return a null icon if
not found.
It's the same problem, the QIcon code calls availableSizes in order to
determine if it should return the (null by default) fallback ;)
Fo this mean we could change QIcon::isNull fo be something like
if (!d) return true;
if (d->notNull) return false;
bool null;
d->virtualHook(CheckNull, &null);
if (null) {
const_cast(this)->operator=(QIcon())
} else {
d->notNull = true;
}
return null;
So than isNull is also lazy, and the problem is solved... is it not?
Ah!! Yes, that would work. Can you make the change in Qt, since you know that code best?
Yes,

Here are the patches:
https://codereview.qt-project.org/125244/
https://codereview.qt-project.org/125245/

So these patch will ensure that the icons are loaded lazily and that only
icons that are meant to be shown will query the file system.

But the problem is that QIcon::isNull is likely to be called anyway.
And this will again do all the look ups in the file system.

The problem is that the current persistent cache is at the wrong level as it
caches the image data. It would be better i think if we had a cache that
cached icon name -> list of files for each sizes.
That's what the gtk icon cache does (well, it does both I think).

We could try to use the gtk cache, But i don't think it's good to depend on
their format. Or should augment the current cache with a mapping to the
availables icons.

And should we do that in Qt or in the KIconLoader?
Post by David Faure
Post by David Faure
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().
That could be a solution.
Post by David Faure
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.
And I think this behaviour is also confusing.
Not sure what you meant here. QIcon("file") should be null if
./file doesn't exist?
Yes. Don't you think?
--
Olivier
David Faure
2015-09-08 08:26:20 UTC
Permalink
Post by Olivier Goffart
But the problem is that QIcon::isNull is likely to be called anyway.
And this will again do all the look ups in the file system.
I don't think so. That's the whole reasoning behind this change.

I added debug output in QIcon and ran konqueror-kf5 (details below).

Result: QIcon::fromTheme is called 84 times, for 66 different icon names.
Among those, 56 do NOT lead to an isNull call.

I think for icons for QActions in menus aren't loaded (or even checked with
isNull) until opening the menu. So this really helps speeding up application startup.

On the other hand, isNull() can obviously be called multiple times for
a given icon, so of course we shouldn't make it slower, it should have the
answer at hand immediately.

==== Tracing details ====

Qt patch http://www.davidfaure.fr/2015/qicon.cpp.diff

$ konqueror 2>&1 | tee /k/konq5-icon.txt

sample output:
konqueror(28364)/default QIcon::fromTheme: QIcon::fromTheme "konqueror" 0x117dc80
konqueror(28364)/default QIcon::fromTheme: QIcon::fromTheme "edit-clear-locationbar-rtl" 0x1372830
konqueror(28364)/default QIcon::fromTheme: QIcon::fromTheme "document-print-frame" 0x136af50
konqueror(28364)/default QIcon::isNull: QIcon::isNull "" 0x0
konqueror(28364)/default QIcon::isNull: QIcon::isNull "konqueror" 0x117dc80
(some lines being repeated more than once)

$ grep fromTheme /k/konq5-icon.txt | sort | uniq | wc -l
66

$ grep fromTheme /k/konq5-icon.txt | sed -e 's/.*0x//' | sort | uniq | while read a; do grep $a /k/konq5-icon.txt | grep -q isNull || echo $a ; done | wc -l
56
--
David Faure, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Olivier Goffart
2015-09-09 08:24:23 UTC
Permalink
Post by David Faure
Post by Olivier Goffart
But the problem is that QIcon::isNull is likely to be called anyway.
And this will again do all the look ups in the file system.
I don't think so. That's the whole reasoning behind this change.
I added debug output in QIcon and ran konqueror-kf5 (details below).
Result: QIcon::fromTheme is called 84 times, for 66 different icon names.
Among those, 56 do NOT lead to an isNull call.
I think for icons for QActions in menus aren't loaded (or even checked with
isNull) until opening the menu. So this really helps speeding up application startup.
Right, but we still need to do a lot of stats for the 10 icons left.

Hence the idea to use the GTK+ cache from within Qt.
https://codereview.qt-project.org/125379/

I'll let Volker do the benchmark.
Post by David Faure
On the other hand, isNull() can obviously be called multiple times for
a given icon, so of course we shouldn't make it slower, it should have the
answer at hand immediately.
I tried to optimize it by 'caching' the isNull value in QIconPrivate.

But then the test failed:
http://code.woboq.org/qt5/qtbase/tests/auto/gui/image/qicon/tst_qicon.cpp.html#633
In that test, the "address-book-new" was looked before and cached, and then we
expect that looking it up again after changing the theme name changes.
So cahcing the value of isNull would make the test fail, because it would be
cached as not null. Yes, the behaviour is that when changing the themeName,
existing QIcon will be re-looked-up next time we try to render them.


Anyway, I hope that calling a virtual function is not too much overhead and
that the fact that QIcon::isNull is now slightly slower is not a problem
compared to the benefit we have to lookup icons lazily.
--
Olivier
Volker Krause
2015-09-09 21:30:49 UTC
Permalink
Post by Olivier Goffart
Post by David Faure
Post by Olivier Goffart
But the problem is that QIcon::isNull is likely to be called anyway.
And this will again do all the look ups in the file system.
I don't think so. That's the whole reasoning behind this change.
I added debug output in QIcon and ran konqueror-kf5 (details below).
Result: QIcon::fromTheme is called 84 times, for 66 different icon names.
Among those, 56 do NOT lead to an isNull call.
I think for icons for QActions in menus aren't loaded (or even checked with
isNull) until opening the menu. So this really helps speeding up application startup.
Right, but we still need to do a lot of stats for the 10 icons left.
Hence the idea to use the GTK+ cache from within Qt.
https://codereview.qt-project.org/125379/
I'll let Volker do the benchmark.
Post by David Faure
On the other hand, isNull() can obviously be called multiple times for
a given icon, so of course we shouldn't make it slower, it should have the
answer at hand immediately.
I tried to optimize it by 'caching' the isNull value in QIconPrivate.
http://code.woboq.org/qt5/qtbase/tests/auto/gui/image/qicon/tst_qicon.cpp.ht
ml#633 In that test, the "address-book-new" was looked before and cached,
and then we expect that looking it up again after changing the theme name
changes. So cahcing the value of isNull would make the test fail, because
it would be cached as not null. Yes, the behaviour is that when changing
the themeName, existing QIcon will be re-looked-up next time we try to
render them.
Anyway, I hope that calling a virtual function is not too much overhead and
that the fact that QIcon::isNull is now slightly slower is not a problem
compared to the benefit we have to lookup icons lazily.
Olivier and me just did some profiling here, in all possible combination.

The results are that both the GTK icon index and the lazy null patch bring
significant improvements (~20% on KMail). With both of them pure Qt icon
loading is as fast as KIconEngine with its content caching, for Oxygen with
hot caches, ie. with PNG icons. When using Breeze (all SVG), the KF5 icon
content caching still brings significant improvements (~10% on KMail).

So, the conclusions are:
- David, please +2 Olivier's Qt patches :)
- If we could have a PNG version of Breeze, we could entirely deprecate
KIconEngine & friends.
- If we can't do that, we also need to use the GTK icon index in KIconEngine,
to implement a fast isNull() (we have been cheating there for the
measurements, by basically assuming "return false").

regards,
Volker
Martin Klapetek
2015-09-09 22:25:09 UTC
Permalink
Post by Volker Krause
Olivier and me just did some profiling here, in all possible combination.
The results are that both the GTK icon index and the lazy null patch bring
significant improvements (~20% on KMail). With both of them pure Qt icon
loading is as fast as KIconEngine with its content caching, for Oxygen with
hot caches, ie. with PNG icons. When using Breeze (all SVG), the KF5 icon
content caching still brings significant improvements (~10% on KMail).
Would maybe this help?

https://git.reviewboard.kde.org/r/125104/

Cheers
--
Martin Klapetek | KDE Developer
David Faure
2015-09-10 07:16:11 UTC
Permalink
Post by Olivier Goffart
I tried to optimize it by 'caching' the isNull value in QIconPrivate.
http://code.woboq.org/qt5/qtbase/tests/auto/gui/image/qicon/tst_qicon.cpp.html#633
In that test, the "address-book-new" was looked before and cached, and then we
expect that looking it up again after changing the theme name changes.
So cahcing the value of isNull would make the test fail, because it would be
cached as not null. Yes, the behaviour is that when changing the themeName,
existing QIcon will be re-looked-up next time we try to render them.
One solution would be a "change counter" in the icon engine, incremented
when changing themes. In QIcon, comparing a local number with that change counter,
and skipping the cache if they don't match. But of course this means one more int
per icon, so I don't know if it's a good idea.

But yeah your isNull implementation looks fast enough; I was afraid it would be a call
to availableSizes() like fromTheme was doing.
--
David Faure, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Michael Pyne
2015-09-10 23:29:23 UTC
Permalink
Post by David Faure
Post by Olivier Goffart
I tried to optimize it by 'caching' the isNull value in QIconPrivate.
http://code.woboq.org/qt5/qtbase/tests/auto/gui/image/qicon/tst_qicon.cpp.
html#633 In that test, the "address-book-new" was looked before and
cached, and then we expect that looking it up again after changing the
theme name changes. So cahcing the value of isNull would make the test
fail, because it would be cached as not null. Yes, the behaviour is that
when changing the themeName, existing QIcon will be re-looked-up next
time we try to render them.
One solution would be a "change counter" in the icon engine, incremented
when changing themes. In QIcon, comparing a local number with that change
counter, and skipping the cache if they don't match. But of course this
means one more int per icon, so I don't know if it's a good idea.
It would only need a static int for QIcon's theme engine as a whole, no? In
this case, for the "is null" cache.

KSharedDataCache supports timestamping (in fact the timestamp is the only
thing you're guaranteed to be able to always retrieve...), and it shouldn't be
hard to track a change counter separately at a process-global perspective if
needed for other aux caches.

Regards,
- Michael Pyne
David Faure
2015-09-11 07:28:51 UTC
Permalink
Post by Michael Pyne
Post by David Faure
Post by Olivier Goffart
I tried to optimize it by 'caching' the isNull value in QIconPrivate.
http://code.woboq.org/qt5/qtbase/tests/auto/gui/image/qicon/tst_qicon.cpp.
html#633 In that test, the "address-book-new" was looked before and
cached, and then we expect that looking it up again after changing the
theme name changes. So cahcing the value of isNull would make the test
fail, because it would be cached as not null. Yes, the behaviour is that
when changing the themeName, existing QIcon will be re-looked-up next
time we try to render them.
One solution would be a "change counter" in the icon engine, incremented
when changing themes. In QIcon, comparing a local number with that change
counter, and skipping the cache if they don't match. But of course this
means one more int per icon, so I don't know if it's a good idea.
It would only need a static int for QIcon's theme engine as a whole, no? In
this case, for the "is null" cache.
You're assuming a cache outside QIcon, while Olivier said he was caching the isNull
value in QIconPrivate. So to know if that cache is valid at a given point in time, you also
need an int in QIconPrivate, to compare that with the int in the icon engine.

I think you're assuming a separate "isnull cache" while in his case, it's inside the icon.

Separate is a solution of course... QHash<QString, bool> ?
hash[key] == QIcon::fromTheme(key).isNull()
That's of course easier to wipe out when switching icon themes.
--
David Faure, ***@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5
Loading...