Bug 103488 - RHEL customers are hitting some scalability problems with accountsservice
Summary: RHEL customers are hitting some scalability problems with accountsservice
Status: RESOLVED FIXED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-27 17:24 UTC by Ray Strode [halfline]
Modified: 2017-10-30 14:02 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
lib: move g_object_ref call for clarity (3.56 KB, patch)
2017-10-27 17:24 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: leak fix (3.44 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: another leak fix (3.68 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: don't send spurious change signals when wtmp changes (4.62 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: consolidate change notification (6.46 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
daemon: add new HasMultipleUsers and HasNoUsers properties (9.32 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: use new has-multiple-users property from accountsservice (14.54 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: factor user loading functions into helpers (8.09 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: move accounts proxy creation to helper (10.97 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: retry connecting to accountsservice when loading users (2.97 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: simplify code dramatically (46.83 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: don't track user-added/user-removed until we get initial list (8.67 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review
lib: only track users after act_user_manager_list_users (26.70 KB, patch)
2017-10-27 17:25 UTC, Ray Strode [halfline]
Details | Splinter Review

Description Ray Strode [halfline] 2017-10-27 17:24:34 UTC
This patchset attempts to address those problems.
Comment 1 Ray Strode [halfline] 2017-10-27 17:24:58 UTC
Created attachment 135116 [details] [review]
lib: move g_object_ref call for clarity

create_new_user creates an ActUser object and returns it to
the caller.  Before returning the object, it also adds the
object to a list of "new users" to track when it fully loads.

It's idiomatic to return the original ref to the caller of
a constructor, but create_new_user instead gives the original
reference to the new_users list.  Their's no practical difference,
but this commit moves the ref around for clarity.
Comment 2 Ray Strode [halfline] 2017-10-27 17:25:00 UTC
Created attachment 135117 [details] [review]
lib: leak fix

If a new user is added by accounts service, we currently leak
a reference to it. That leads to transient user objects living
longer than they're supposed which uses memory and produces
excess bus traffic.
Comment 3 Ray Strode [halfline] 2017-10-27 17:25:03 UTC
Created attachment 135118 [details] [review]
lib: another leak fix

If a user gets removed from account service before it's fully loaded,
then we neglect to drop its reference on the new_users lists.

This commit fixes that.
Comment 4 Ray Strode [halfline] 2017-10-27 17:25:05 UTC
Created attachment 135119 [details] [review]
daemon: don't send spurious change signals when wtmp changes

Right now, we unintentionally send out a changed signal for
every tracked user anytime wtmp changes.

This commit fixes that.
Comment 5 Ray Strode [halfline] 2017-10-27 17:25:07 UTC
Created attachment 135120 [details] [review]
lib: consolidate change notification

if the daemon sends out multiple change notifications for a user
because several properties changed in quick succession, try to
batch them up, and only update info at the end.
Comment 6 Ray Strode [halfline] 2017-10-27 17:25:09 UTC
Created attachment 135121 [details] [review]
daemon: add new HasMultipleUsers and HasNoUsers properties

Every gnome-shell instance wants to know if the system has multiple
users or not, in order to know whether or not to show the
'Switch User' feature in the menu.

accountsservice doesn't provide this information directly, though,
so libaccountsservice instead requests a list of all users on the
system and counts the provided list, filtering out system users.

This is a lot of work for every gnome-shell instance to do, when
it doesn't actually need the list of users at all.

This adds a new property HasMultipleUsers which libaccountsservice
can watch for instead.  For good measure, this commit also adds a
HasNoUsers boolean which can be used to know whether or not to start
gnome-initial-setup.
Comment 7 Ray Strode [halfline] 2017-10-27 17:25:11 UTC
Created attachment 135122 [details] [review]
lib: use new has-multiple-users property from accountsservice

This commit changes accountsservice to use the new has-multiple-users
property provided by the daemon.
Comment 8 Ray Strode [halfline] 2017-10-27 17:25:13 UTC
Created attachment 135123 [details] [review]
lib: factor user loading functions into helpers

Right now, we process ListCachedUser results open coded.

This commit moves the processing to helper functions.
Comment 9 Ray Strode [halfline] 2017-10-27 17:25:15 UTC
Created attachment 135124 [details] [review]
lib: move accounts proxy creation to helper

This commit factors out hte accounts proxy creation to a
helper function.
Comment 10 Ray Strode [halfline] 2017-10-27 17:25:17 UTC
Created attachment 135125 [details] [review]
lib: retry connecting to accountsservice when loading users

If we were unable to connect to accountsservice and we need to
load users again, then try again.
Comment 11 Ray Strode [halfline] 2017-10-27 17:25:20 UTC
Created attachment 135126 [details] [review]
lib: simplify code dramatically

ActUser is a wrapper over the accountsservice daemon's
managed user objects.  There's a nearly 1-to-1 correspondence
between properties on the proxy to the daemon and properties
on the ActUser object.

This commit dramatically reduces the code, by leveraging the
proxies properties directly, rather than duplicating the values
on the ActUser object.

At the same time, it drops manual GetAll() calls for synchronizing
the proxies properties, since it's completely redundant with the
work the proxy is doing under the hood anyway.
Comment 12 Ray Strode [halfline] 2017-10-27 17:25:22 UTC
Created attachment 135127 [details] [review]
lib: don't track user-added/user-removed until we get initial list

There's no reason to process user-added and user-removed signals
until we have our starting list.  Those signals are supposed to
be a delta off that list anyway.
Comment 13 Ray Strode [halfline] 2017-10-27 17:25:24 UTC
Created attachment 135128 [details] [review]
lib: only track users after act_user_manager_list_users

At the moment, as soon as someone calls
act_user_manager_get_default() we end up firing of an asynchronous
call to get a list of all users. This is problematic since not all
programs using accountsservice want a list of users.

This commit changes the code to only get a list of users when the
caller invokes act_user_manager_list_users.  This does mean some calls
that were async before are synchronous now, but user proxies were
always obtained synchronously, and they're by far the slowest part
of listing users, so I don't expect this introduce any noticeable
blocking.

Longer term, to fix the sync i/o bits, I think we should
probably ditch libaccountsservice and just make accountsservice use
ObjectManager interfaces over d-bus.
Comment 14 Ray Strode [halfline] 2017-10-30 14:02:55 UTC
Attachment 135116 [details] pushed as 53b7b2a - lib: move g_object_ref call for clarity
Attachment 135117 [details] pushed as c8b7201 - lib: leak fix
Attachment 135118 [details] pushed as 53568fa - lib: another leak fix
Attachment 135119 [details] pushed as ce3f71c - daemon: don't send spurious change signals when wtmp changes
Attachment 135120 [details] pushed as 3c86a80 - lib: consolidate change notification
Attachment 135121 [details] pushed as c323b23 - daemon: add new HasMultipleUsers and HasNoUsers properties
Attachment 135122 [details] pushed as 074b802 - lib: use new has-multiple-users property from accountsservice
Attachment 135123 [details] pushed as 3727f77 - lib: factor user loading functions into helpers
Attachment 135124 [details] pushed as 24ed1f2 - lib: move accounts proxy creation to helper
Attachment 135125 [details] pushed as 80588c7 - lib: retry connecting to accountsservice when loading users
Attachment 135126 [details] pushed as 9b41087 - lib: simplify code dramatically
Attachment 135127 [details] pushed as 9b8727c - lib: don't track user-added/user-removed until we get initial list
Attachment 135128 [details] pushed as cd83d3d - lib: only track users after act_user_manager_list_users


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.