Bug 346644 - KF5/KMail Performance Bottleneck in Akonadi::StatisticsProxyModel::data(...)
Summary: KF5/KMail Performance Bottleneck in Akonadi::StatisticsProxyModel::data(...)
Status: RESOLVED FIXED
Alias: None
Product: kdepimlibs
Classification: Applications
Component: akonadi (show other bugs)
Version: GIT (master)
Platform: Other Linux
: NOR grave
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-25 15:45 UTC by Andreas Cord-Landwehr
Modified: 2015-06-04 12:39 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Cord-Landwehr 2015-04-25 15:45:33 UTC
About 50% of KMail's CPU time is spent via QIcon::fromTheme(QString &const, QIcon const&), called via Akonadi::StatisticsProxyModel::data(...) in statisticsproxymodel.cpp, line 427.
KF5/KMail is quite unresponsive due to this, even with a fast SSD on my machine. Performance stats where tested via attaching perf. Replacing QIcon::fromTheme( CollectionUtils::displayIconName( collection ) ) with a simple "QIcon()" fixes my performance problems.
Probably caching the icon is the way to go here...

Reproducible: Always

Steps to Reproduce:
1. start KMail
2. happens :)
Comment 1 Laurent Montel 2015-04-26 05:30:26 UTC
I will investigate it soon.
Comment 2 Andreas Cord-Landwehr 2015-04-26 07:37:53 UTC
Thanks, if you interested in the perf record, just ping me (though it is ~900MB large)
Comment 3 Laurent Montel 2015-04-26 08:27:55 UTC
When I read qt code I see that icon is caching
QIcon QIcon::fromTheme(const QString &name, const QIcon &fallback)
{
    QIcon icon;

    if (qtIconCache()->contains(name)) {
        icon = *qtIconCache()->object(name);
    } else {
        QPlatformTheme * const platformTheme = QGuiApplicationPrivate::platformTheme();
        QIconEngine * const engine = platformTheme ? platformTheme->createIconEngine(name)
                                                   : new QIconLoaderEngine(name);
        QIcon *cachedIcon  = new QIcon(engine);
        icon = *cachedIcon;
        qtIconCache()->insert(name, cachedIcon);
    }

    // Note the qapp check is to allow lazy loading of static icons
    // Supporting fallbacks will not work for this case.
    if (qApp && icon.availableSizes().isEmpty())
        return fallback;

    return icon;
}

for the moment I don't understand why it doesn't work. Perhaps CollectionUtils::displayIconName has a problem.
I will look at.
Comment 4 Andreas Cord-Landwehr 2015-04-26 08:47:09 UTC
Maybe this gives some more light on the issue, the paste of the first expanded entries of "perf report":
https://paste.kde.org/p6oxmvnmr
Comment 5 Laurent Montel 2015-04-26 10:26:12 UTC
Thanks for info.
Comment 6 Andreas Cord-Landwehr 2015-05-09 18:48:37 UTC
There actually happen quite interesting things behind the scenes. Here, my first analysis what happens (all assertions I do below are actually verified):

1) in kdepimlibs/akonadi/src/core/models/statisticsproxymodel.cpp
the model's data method returns
QIcon::fromTheme( CollectionUtils::displayIconName( collection ) )
this method is obviosly called a very big number of times

2) in qtbase/src/gui/image/qicon.cpp
the correctly cached icon from the internal cache is selected,
but then for a fallback check the following is tested:
    if (qApp && icon.availableSizes().isEmpty())
        return fallback;
and here the trouble starts with QIcon::availableSizes()
which triggers a call to KIconEngine::availableSizes()

3) kiconthemes/src/kiconengine.cpp
here we have a check that actually an icon exists (introduced in RR 122608, [2]), which calls the following very costly check:

   if (mIconLoader->iconPath(mIconName, KIconLoader::Desktop,
       KIconLoader::MatchBest).isEmpty()) {
       return QList<QSize>();
   }

So, the question is where we should fix this.
Comment 7 Daniel Vrátil 2015-05-14 12:41:47 UTC
Hi, this got too unbearable for me, so I pushed a workaround to CollectionModel in kdepimlibs.git/akonadi - basically we now cache the QIcons in the CollectionModel. There are only few icons, so the memory footprint should be minimal.

http://commits.kde.org/kdepimlibs/af21e9b58fbfb77fde15bc7b9e43580975e318f5

Proper fix in FrameworkIntegration plugin is the preferred way to go of course, but still...

Anyway, this and https://git.reviewboard.kde.org/r/123791/ make KMail from master not being sluggish anymore and I think it's comparable to 4.X again.
Comment 8 Daniel Vrátil 2015-05-14 12:49:01 UTC
...which turns our is not the proper place to fix the bottleneck...:)
Comment 9 Daniel Vrátil 2015-05-14 13:06:58 UTC
Git commit d18a3173144014ec0b19ad8466684d69534351a8 by Dan Vrátil.
Committed on 14/05/2015 at 13:06.
Pushed by dvratil into branch 'master'.

Add the QIcon::fromTheme() workaround to ETM

Workaroud the very slow QIcon::fromTheme() directly in ETM, which gives us the
cache for free for both Collections and Items. The cache is really small (not
more than 10 icons) so we can afford to have it all the way down in ETM.

Also adjusted StatisticsProxyModel to call ETM::data() for DecorationRole
instead of trying to resolve the icon on its own.

M  +5    -3    akonadi/src/core/models/entitytreemodel.cpp
M  +15   -0    akonadi/src/core/models/entitytreemodel_p.cpp
M  +6    -0    akonadi/src/core/models/entitytreemodel_p.h
M  +1    -5    akonadi/src/core/models/statisticsproxymodel.cpp

http://commits.kde.org/kdepimlibs/d18a3173144014ec0b19ad8466684d69534351a8
Comment 10 Andreas Cord-Landwehr 2015-05-31 23:16:31 UTC
Possible fix is currently discussed at reviewboard: https://git.reviewboard.kde.org/r/123924/
Comment 11 Andreas Cord-Landwehr 2015-06-04 12:39:25 UTC
This was fixed by commit 5fdfb9dd2abc1caf6cb63a6f73800b19f5d6357a to kiconthemes (see review link above).
Thanks and hugs to Aleix for this!