QIcon::fromTheme(xxx, someFallback)

[Volker Krause]

On Wednesday 09 September 2015 10:24:23 Olivier Goffart wrote:
> On Tuesday 8. September 2015 10:26:20 David Faure wrote:
> > On Monday 07 September 2015 15:53:31 Olivier Goffart wrote:
> > > 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.
> 
> > 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.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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 173 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150909/2dedd648/attachment.sig>