Merge lp:~mzanetti/unity8/fix-laggy-launcher-dnd into lp:unity8

[Michael Zanetti]

performance improvements

* Prevent processing of built-in AccountsService properties if we know it's not a built-in that changed
* don't reload the launcher settings after we just set them ourselves

 * Are there any related MPs required for this MP to build/function as expected? Please list.

no

 * Did you perform an exploratory manual test run of your code change and any related functionality?

yes

 * Did you make sure that your branch does not contain spurious tags?

yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

n/a

 * If you changed the UI, has there been a design review?

n/a

[lp:~mzanetti/unity8/fix-laggy-launcher-dnd]

1642. By Michael Zanetti on 2015-03-03

fix typo in comment

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1642
http://jenkins.qa.ubuntu.com/job/unity8-ci/5381/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1617
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/545
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/546
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/546
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1435
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1615
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1615/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18501

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5381/rebuild

review: Needs Fixing (continuous-integration)

[Michael Terry (mterry)]

Inline comments

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Added my two cents inline

[lp:~mzanetti/unity8/fix-laggy-launcher-dnd]

1643. By Michael Zanetti on 2015-03-04

be more clever with caching

[Michael Zanetti (mzanetti)]

inline replies

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1643
http://jenkins.qa.ubuntu.com/job/unity8-ci/5386/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1639
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/550
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/551
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/551
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1453
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1637
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1637/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18530

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5386/rebuild

review: Needs Fixing (continuous-integration)

[lp:~mzanetti/unity8/fix-laggy-launcher-dnd]

1644. By Michael Zanetti on 2015-03-04

always reset the flag

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1644
http://jenkins.qa.ubuntu.com/job/unity8-ci/5389/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1646
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/553
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/554
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/554
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1460
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1644
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1644/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18549

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5389/rebuild

review: Needs Fixing (continuous-integration)

[Michael Terry (mterry)]

Looks great! Tested and it's fine.

 * Did you perform an exploratory manual test run of the code change and any related functionality?
 Yes

 * Did CI run pass? If not, please explain why.
 Eh, for other reasons.

 * Did you make sure that the branch does not contain spurious tags?
 Yes

review: Approve

[Michał Sawicz (saviq)]

Is there a way to test this? The symptom I was fighting with this morning was starting or closing an unpinned app, this resulted in 1-2s UI lock every time.

[Michael Zanetti (mzanetti)]

> Is there a way to test this? The symptom I was fighting with this morning was
> starting or closing an unpinned app, this resulted in 1-2s UI lock every time.

So the actual issue is that AccountService does only emit a "Changed" signal for all built-in properties, without having a parameter that actually telly you which built-in property it is. That in turn causes all connected things to look up their properties with sync dbus calls.

This branch at least decouples built-in properties from not built-in ones (the launcher stuff isn't built-in) so a change in the launcher doesn't cause things like "backgroundimage" to refresh at least.

I guess it'd be possible to test if a change in a not built-in property does not emit the generic changed property. Whether that's really a useful test or not, I'm not exactly sure about.

=== modified file 'plugins/AccountsService/AccountsService.cpp'
--- plugins/AccountsService/AccountsService.cpp	2014-11-28 15:55:08 +0000
+++ plugins/AccountsService/AccountsService.cpp	2015-03-04 15:58:00 +0000
@@ -152,7 +152,7 @@
 
 void AccountsService::updateBackgroundFile()
 {
-    auto backgroundFile = m_service->getUserProperty(m_user, "org.freedesktop.Accounts.User", "BackgroundFile").toString();
+    QString backgroundFile = m_service->getUserProperty(m_user, "org.freedesktop.Accounts.User", "BackgroundFile").toString();
     if (m_backgroundFile != backgroundFile) {
         m_backgroundFile = backgroundFile;
         Q_EMIT backgroundFileChanged();

=== modified file 'plugins/AccountsService/AccountsServiceDBusAdaptor.cpp'
--- plugins/AccountsService/AccountsServiceDBusAdaptor.cpp	2014-12-12 10:00:31 +0000
+++ plugins/AccountsService/AccountsServiceDBusAdaptor.cpp	2015-03-04 15:58:00 +0000
@@ -25,7 +25,8 @@
 AccountsServiceDBusAdaptor::AccountsServiceDBusAdaptor(QObject* parent)
   : QObject(parent),
     m_accountsManager(nullptr),
-    m_users()
+    m_users(),
+    m_ignoreNextChanged(false)
 {
     QDBusConnection connection = QDBusConnection::SM_BUSNAME();
     QDBusConnectionInterface *interface = connection.interface();
@@ -75,11 +76,19 @@
     combined.removeDuplicates();
 
     Q_EMIT propertiesChanged(getUserForPath(message().path()), interface, combined);
+
+    // In case a non-builtin property changes, we're getting propertiesChanged *and* changed
+    // As the generic changed requires asking back over DBus, it's quite slow to process.
+    // We don't want to trigger that when we know it's not a built-in property change.
+    m_ignoreNextChanged = true;
 }
 
 void AccountsServiceDBusAdaptor::maybeChangedSlot()
 {
-    Q_EMIT maybeChanged(getUserForPath(message().path()));
+    if (!m_ignoreNextChanged) {
+        Q_EMIT maybeChanged(getUserForPath(message().path()));
+    }
+    m_ignoreNextChanged = false;
 }
 
 QString AccountsServiceDBusAdaptor::getUserForPath(const QString &path)

=== modified file 'plugins/AccountsService/AccountsServiceDBusAdaptor.h'
--- plugins/AccountsService/AccountsServiceDBusAdaptor.h	2015-02-04 09:41:40 +0000
+++ plugins/AccountsService/AccountsServiceDBusAdaptor.h	2015-03-04 15:58:00 +0000
@@ -61,6 +61,8 @@
 
     QDBusInterface *m_accountsManager;
     QMap<QString, QDBusInterface *> m_users;
+
+    bool m_ignoreNextChanged;
 };
 
 #endif

=== modified file 'plugins/Unity/Launcher/gsettings.cpp'
--- plugins/Unity/Launcher/gsettings.cpp	2014-10-02 13:34:11 +0000
+++ plugins/Unity/Launcher/gsettings.cpp	2015-03-04 15:58:00 +0000
@@ -59,12 +59,19 @@
     Q_FOREACH(const QString &entry, storedApplications) {
         gSettingsList << QString("appid://%1").arg(entry);
     }
+    // GSettings will emit a changed signal to ourselves. Let's cache the items
+    // and only forward the changed signal when the list did actually change.
+    m_cachedItems = gSettingsList;
     m_gSettings->set("items", gSettingsList);
 }
 
 void GSettings::onSettingsChanged(const QString &key)
 {
     if (key == "items") {
-        Q_EMIT changed();
+        QStringList cachedItems = m_gSettings->get("items").toStringList();
+        if (m_cachedItems != cachedItems) {
+            m_cachedItems = cachedItems;
+            Q_EMIT changed();
+        }
     }
 }

=== modified file 'plugins/Unity/Launcher/gsettings.h'
--- plugins/Unity/Launcher/gsettings.h	2014-10-02 13:34:11 +0000
+++ plugins/Unity/Launcher/gsettings.h	2015-03-04 15:58:00 +0000
@@ -41,6 +41,7 @@
 
 private:
     QGSettings *m_gSettings;
+    QStringList m_cachedItems;
 };
 
 #endif