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

Proposed by Michael Zanetti
Status: Merged
Approved by: Michael Terry
Approved revision: 1644
Merged at revision: 1678
Proposed branch: lp:~mzanetti/unity8/fix-laggy-launcher-dnd
Merge into: lp:unity8
Diff against target: 97 lines (+23/-4)
5 files modified
plugins/AccountsService/AccountsService.cpp (+1/-1)
plugins/AccountsService/AccountsServiceDBusAdaptor.cpp (+11/-2)
plugins/AccountsService/AccountsServiceDBusAdaptor.h (+2/-0)
plugins/Unity/Launcher/gsettings.cpp (+8/-1)
plugins/Unity/Launcher/gsettings.h (+1/-0)
To merge this branch: bzr merge lp:~mzanetti/unity8/fix-laggy-launcher-dnd
Reviewer Review Type Date Requested Status
Michael Terry Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+251567@code.launchpad.net

Commit message

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

Description of the change

 * 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

To post a comment you must log in.
1642. By Michael Zanetti

fix typo in comment

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

Inline comments

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Added my two cents inline

1643. By Michael Zanetti

be more clever with caching

Revision history for this message
Michael Zanetti (mzanetti) wrote :

inline replies

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1644. By Michael Zanetti

always reset the flag

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

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
Revision history for this message
MichaƂ Sawicz (saviq) wrote :

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.

Revision history for this message
Michael Zanetti (mzanetti) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/AccountsService/AccountsService.cpp'
2--- plugins/AccountsService/AccountsService.cpp 2014-11-28 15:55:08 +0000
3+++ plugins/AccountsService/AccountsService.cpp 2015-03-04 15:58:00 +0000
4@@ -152,7 +152,7 @@
5
6 void AccountsService::updateBackgroundFile()
7 {
8- auto backgroundFile = m_service->getUserProperty(m_user, "org.freedesktop.Accounts.User", "BackgroundFile").toString();
9+ QString backgroundFile = m_service->getUserProperty(m_user, "org.freedesktop.Accounts.User", "BackgroundFile").toString();
10 if (m_backgroundFile != backgroundFile) {
11 m_backgroundFile = backgroundFile;
12 Q_EMIT backgroundFileChanged();
13
14=== modified file 'plugins/AccountsService/AccountsServiceDBusAdaptor.cpp'
15--- plugins/AccountsService/AccountsServiceDBusAdaptor.cpp 2014-12-12 10:00:31 +0000
16+++ plugins/AccountsService/AccountsServiceDBusAdaptor.cpp 2015-03-04 15:58:00 +0000
17@@ -25,7 +25,8 @@
18 AccountsServiceDBusAdaptor::AccountsServiceDBusAdaptor(QObject* parent)
19 : QObject(parent),
20 m_accountsManager(nullptr),
21- m_users()
22+ m_users(),
23+ m_ignoreNextChanged(false)
24 {
25 QDBusConnection connection = QDBusConnection::SM_BUSNAME();
26 QDBusConnectionInterface *interface = connection.interface();
27@@ -75,11 +76,19 @@
28 combined.removeDuplicates();
29
30 Q_EMIT propertiesChanged(getUserForPath(message().path()), interface, combined);
31+
32+ // In case a non-builtin property changes, we're getting propertiesChanged *and* changed
33+ // As the generic changed requires asking back over DBus, it's quite slow to process.
34+ // We don't want to trigger that when we know it's not a built-in property change.
35+ m_ignoreNextChanged = true;
36 }
37
38 void AccountsServiceDBusAdaptor::maybeChangedSlot()
39 {
40- Q_EMIT maybeChanged(getUserForPath(message().path()));
41+ if (!m_ignoreNextChanged) {
42+ Q_EMIT maybeChanged(getUserForPath(message().path()));
43+ }
44+ m_ignoreNextChanged = false;
45 }
46
47 QString AccountsServiceDBusAdaptor::getUserForPath(const QString &path)
48
49=== modified file 'plugins/AccountsService/AccountsServiceDBusAdaptor.h'
50--- plugins/AccountsService/AccountsServiceDBusAdaptor.h 2015-02-04 09:41:40 +0000
51+++ plugins/AccountsService/AccountsServiceDBusAdaptor.h 2015-03-04 15:58:00 +0000
52@@ -61,6 +61,8 @@
53
54 QDBusInterface *m_accountsManager;
55 QMap<QString, QDBusInterface *> m_users;
56+
57+ bool m_ignoreNextChanged;
58 };
59
60 #endif
61
62=== modified file 'plugins/Unity/Launcher/gsettings.cpp'
63--- plugins/Unity/Launcher/gsettings.cpp 2014-10-02 13:34:11 +0000
64+++ plugins/Unity/Launcher/gsettings.cpp 2015-03-04 15:58:00 +0000
65@@ -59,12 +59,19 @@
66 Q_FOREACH(const QString &entry, storedApplications) {
67 gSettingsList << QString("appid://%1").arg(entry);
68 }
69+ // GSettings will emit a changed signal to ourselves. Let's cache the items
70+ // and only forward the changed signal when the list did actually change.
71+ m_cachedItems = gSettingsList;
72 m_gSettings->set("items", gSettingsList);
73 }
74
75 void GSettings::onSettingsChanged(const QString &key)
76 {
77 if (key == "items") {
78- Q_EMIT changed();
79+ QStringList cachedItems = m_gSettings->get("items").toStringList();
80+ if (m_cachedItems != cachedItems) {
81+ m_cachedItems = cachedItems;
82+ Q_EMIT changed();
83+ }
84 }
85 }
86
87=== modified file 'plugins/Unity/Launcher/gsettings.h'
88--- plugins/Unity/Launcher/gsettings.h 2014-10-02 13:34:11 +0000
89+++ plugins/Unity/Launcher/gsettings.h 2015-03-04 15:58:00 +0000
90@@ -41,6 +41,7 @@
91
92 private:
93 QGSettings *m_gSettings;
94+ QStringList m_cachedItems;
95 };
96
97 #endif

Subscribers

People subscribed via source and target branches