Merge lp:~marcustomlinson/unity-scopes-shell/lp-1627795 into lp:unity-scopes-shell

[Marcus Tomlinson]

Don't make changes to the settings model from a separate thread.

[Paweł Stołowski (stolowski)]

Hmmm... AFAIR the original change with QtConcurrent was introduced because of the measured (and actually noticable) delay related to child scopes update & resulting settings model update? Aren't we returning to this problem with such change?

How about instead of calling m_settingsModel->update_child_scopes(...) directly from the QtConcurrent's thread, we do this via a signal-slot or a custom QEvent?

review: Needs Information

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:346
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-shell-ci/2/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/832/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/839
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/646/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/646/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/646/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/646
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/646/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-shell-ci/2/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

> Hmmm... AFAIR the original change with QtConcurrent was introduced because of
> the measured (and actually noticable) delay related to child scopes update &
> resulting settings model update? Aren't we returning to this problem with such
> change?
>
> How about instead of calling m_settingsModel->update_child_scopes(...)
> directly from the QtConcurrent's thread, we do this via a signal-slot or a
> custom QEvent?

No, we still retain most of the optimisation. See line 43 of the diff here: https://code.launchpad.net/~marcustomlinson/unity-scopes-shell/lp-1567429/+merge/291480

The main improvement that was introduced was a m_childScopesDirty flag that avoided us re-loading child scopes each time settings() was called.

[Marcus Tomlinson (marcustomlinson)]

Got it building here: https://bileto.ubuntu.com/#/ticket/1785

I'll see if there is any noticeable performance hit.

[Marcus Tomlinson (marcustomlinson)]

No noticeable loss in performance.

+ This still gives us the 0ms lag on settings() (which is what we fixed in the previous MP: Bug #1567429)

+ I still see that cool behaviour where the scope starts up quickly, then the settings cog appears as soon as child scopes are loaded.

[Paweł Stołowski (stolowski)]

Cool. This looks good to me, thanks! +1

review: Approve

=== modified file 'src/Unity/scope.cpp'
--- src/Unity/scope.cpp	2016-06-24 14:03:59 +0000
+++ src/Unity/scope.cpp	2016-10-04 08:08:33 +0000
@@ -133,7 +133,6 @@
 
 Scope::~Scope()
 {
-    m_childScopesFuture.waitForFinished();
 }
 
 void Scope::processSearchChunk(PushEvent* pushEvent)
@@ -964,14 +963,7 @@
         // reset the flag so that re-entering this method won't restart the update unnecessarily
         m_childScopesDirty = false;
 
-        // just in case we have another update still running, wait here for it to complete
-        m_childScopesFuture.waitForFinished();
-
-        // run the update in a seperate thread
-        m_childScopesFuture = QtConcurrent::run([this]
-        {
-            m_settingsModel->update_child_scopes(m_scopesInstance->getAllMetadata());
-        });
+        m_settingsModel->update_child_scopes(m_scopesInstance->getAllMetadata());
     }
 }
 

=== modified file 'src/Unity/scope.h'
--- src/Unity/scope.h	2016-06-23 09:27:45 +0000
+++ src/Unity/scope.h	2016-10-04 08:08:33 +0000
@@ -21,7 +21,6 @@
 #define NG_SCOPE_H
 
 // Qt
-#include <QFuture>
 #include <QObject>
 #include <QString>
 #include <QTimer>
@@ -256,7 +255,6 @@
     int m_cardinality;
 
     bool m_childScopesDirty;
-    QFuture<void> m_childScopesFuture;
 
     QMap<std::string, QList<std::shared_ptr<unity::scopes::CategorisedResult>>> m_category_results;
     std::unique_ptr<CollectionController> m_searchController;

=== modified file 'src/Unity/settingsmodel.cpp'
--- src/Unity/settingsmodel.cpp	2016-05-30 07:08:49 +0000
+++ src/Unity/settingsmodel.cpp	2016-10-04 08:08:33 +0000
@@ -158,8 +158,6 @@
 
 QVariant SettingsModel::data(const QModelIndex& index, int role) const
 {
-    QMutexLocker locker(&m_mutex);
-
     int row = index.row();
     QVariant result;
 
@@ -255,8 +253,6 @@
 
 QVariant SettingsModel::value(const QString& id) const
 {
-    QMutexLocker locker(&m_mutex);
-
     // Check for the setting id in the child scopes list first, in case the
     // aggregator is incorrectly using a scope id as a settings as well.
     if (m_child_scopes_data_by_id.contains(id))
@@ -313,8 +309,6 @@
 
 void SettingsModel::update_child_scopes(QMap<QString, sc::ScopeMetadata::SPtr> const& scopes_metadata)
 {
-    QMutexLocker locker(&m_mutex);
-
     if (!scopes_metadata.contains(m_scopeId) ||
         !scopes_metadata[m_scopeId]->is_aggregator())
     {
@@ -369,15 +363,12 @@
         endResetModel();
     }
 
-    locker.unlock();
     Q_EMIT countChanged();
 }
 
 bool SettingsModel::setData(const QModelIndex &index, const QVariant &value,
         int role)
 {
-    QMutexLocker locker(&m_mutex);
-
     int row = index.row();
     QVariant result;
 
@@ -441,20 +432,16 @@
 
 int SettingsModel::count() const
 {
-    QMutexLocker locker(&m_mutex);
     return m_data.size() + m_child_scopes_data.size();
 }
 
 bool SettingsModel::require_child_scopes_refresh() const
 {
-    QMutexLocker locker(&m_mutex);
     return m_requireChildScopesRefresh;
 }
 
 void SettingsModel::settings_timeout()
 {
-    QMutexLocker locker(&m_mutex);
-
     QObject *timer = sender();
     if (!timer)
     {
@@ -478,7 +465,6 @@
             {
                 m_scopeProxy->set_child_scopes(m_child_scopes);
 
-                locker.unlock();
                 Q_EMIT settingsChanged();
             }
             catch (std::exception const& e)
@@ -514,7 +500,6 @@
             FileLock lock = unixLock(m_settings_path, true);
             m_settings->sync(); // make sure the change to setting value is synced to fs
 
-            locker.unlock();
             Q_EMIT settingsChanged();
         }
         catch(const unity::FileException& e)

=== modified file 'src/Unity/settingsmodel.h'
--- src/Unity/settingsmodel.h	2016-05-30 07:08:49 +0000
+++ src/Unity/settingsmodel.h	2016-10-04 08:08:33 +0000
@@ -29,7 +29,6 @@
 
 #include <QAbstractListModel>
 #include <QList>
-#include <QMutex>
 #include <QSharedPointer>
 
 QT_BEGIN_NAMESPACE
@@ -95,7 +94,6 @@
     void tryLoadSettings(bool read_only) const;
 
 protected:
-    mutable QMutex m_mutex;
     QString m_scopeId;
     unity::scopes::ScopeProxy m_scopeProxy;
     int m_settingsTimeout;