Add cache for KisIconUtils::loadIcon
ClosedPublic

Authored by poke1024 on Oct 22 2017, 12:42 PM.

Details

Summary

I found this while profiling and KisPaintLayer::icon with a small but astonishing 0.1%:

It turns out, about once to twice per second, Krita really does re-render the SVG for its layer paint icons:

This seems to be the only icon that is reloaded periodically in this way, so first I thought the best fix might be to preload the QIcon into the paintlayer's private struct.

On further thinking though, a more general caching seems nicer as more code might be affected in other places. Also, caching inside KisIconUtils also works when switching themes, as the library seems to get reloaded when themes are switched (and thus the static vars cleared, at least that's what I see); this would add ugly code if caching the QIcon inside the paint layer. So I vote for having a general caching here and solving all icon caching issues there.

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
poke1024 created this revision.Oct 22 2017, 12:42 PM
dkazakov accepted this revision.Oct 23 2017, 8:39 AM
dkazakov added subscribers: rempt, dkazakov.

The patch looks fine. Some time ago we used KDE's icons engine, which had its own caching, but after porting to Qt5 it is gone, so now, yes, we need some caching.

Please ask @rempt if we also need to push this patch or not (I'm not sure myself), it looks as if the line was lost/forgotten somehow:

1diff --git a/libs/widgetutils/kis_icon_utils.cpp b/libs/widgetutils/kis_icon_utils.cpp
2index 3df5222..fb6dfb0 100644
3--- a/libs/widgetutils/kis_icon_utils.cpp
4+++ b/libs/widgetutils/kis_icon_utils.cpp
5@@ -114,6 +114,7 @@ QIcon loadIcon(const QString &name)
6
7 QIcon icon = QIcon::fromTheme(name);
8 qWarning() << "\tfalling back on QIcon::FromTheme:" << name;
9+ s_icons.insert(icon.cacheKey(), name);
10 s_cache.insert(name, icon);
11 return icon;
12 }

This revision is now accepted and ready to land.Oct 23 2017, 8:39 AM
rempt accepted this revision.Oct 23 2017, 8:48 AM

I would think that QIcon::fromTheme already does caching for us, so that wouldn't be needed.

I guess P116 can just be added to the patch and pushed :)

This revision was automatically updated to reflect the committed changes.
rempt reopened this revision.Nov 2 2017, 9:28 AM

Meh... I backported this to 3.3 today and started building a release, but this seems to cause a crash:

Program received signal SIGSEGV, Segmentation fault.
0x00007fdfbdb389b3 in QIcon::~QIcon() () from /krita.appdir/usr/lib64/libQt5Gui.so.5
(gdb) bt
#0 0x00007fdfbdb389b3 in QIcon::~QIcon() () from /krita.appdir/usr/lib64/libQt5Gui.so.5
#1 0x00007fdfc019a471 in callDestructorIfNecessary<QIcon> (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:101
#2 QMapNode<QString, QIcon>::destroySubTree (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:126
#3 0x00007fdfc019b21e in destroy (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:244
#4 QMap<QString, QIcon>::~QMap (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:335
#5 0x000000357c235e7d in cxa_finalize () from /lib64/libc.so.6
#6 0x00007fdfc0194b33 in
do_global_dtors_aux () from /krita.appdir/usr/lib64/libkritawidgetutils.so.18
#7 0x00007fffef01aed0 in ?? ()
#8 0x000000357ba0ed3c in _dl_fini () from /lib64/ld-linux-x86-64.so.2
Backtrace stopped: frame did not save the PC

This revision is now accepted and ready to land.Nov 2 2017, 9:28 AM
rempt added a comment.Nov 2 2017, 9:45 AM

Hm... No crash on my desktop -- I guess it could be to do with the version of Qt... It's 5.9 on the desktop, 5.6 for the appimage builder and 5.7 for OSX.

In D8422#163337, @rempt wrote:

Meh... I backported this to 3.3 today and started building a release, but this seems to cause a crash:

Program received signal SIGSEGV, Segmentation fault.
0x00007fdfbdb389b3 in QIcon::~QIcon() () from /krita.appdir/usr/lib64/libQt5Gui.so.5
(gdb) bt
#0 0x00007fdfbdb389b3 in QIcon::~QIcon() () from /krita.appdir/usr/lib64/libQt5Gui.so.5
#1 0x00007fdfc019a471 in callDestructorIfNecessary<QIcon> (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:101
#2 QMapNode<QString, QIcon>::destroySubTree (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:126
#3 0x00007fdfc019b21e in destroy (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:244
#4 QMap<QString, QIcon>::~QMap (this=Unhandled dwarf expression opcode 0xf3
) at /usr/include/QtCore/qmap.h:335
#5 0x000000357c235e7d in cxa_finalize () from /lib64/libc.so.6
#6 0x00007fdfc0194b33 in
do_global_dtors_aux () from /krita.appdir/usr/lib64/libkritawidgetutils.so.18
#7 0x00007fffef01aed0 in ?? ()
#8 0x000000357ba0ed3c in _dl_fini () from /lib64/ld-linux-x86-64.so.2
Backtrace stopped: frame did not save the PC

This would seem to be when destructing KisIconUtils::s_cache. QIcon needs a QGuiApplication instance, so I would think the map should be emptied when KisApplication's destructor is called...

Looks like that really is an upstream bug: https://bugreports.qt.io/browse/QTBUG-50829

rempt added a comment.Nov 2 2017, 2:05 PM

I worked around that qt bug by only using the cache with 5.9, where at least on osx and linux it is not a problem.

alvinhochun closed this revision.Nov 7 2017, 1:35 PM