{"action":"created","issue":{"url":"https://api.github.com/repos/zulip/zulip/issues/375","labels_url":"https://api.github.com/repos/zulip/zulip/issues/375/labels{/name}","comments_url":"https://api.github.com/repos/zulip/zulip/issues/375/comments","events_url":"https://api.github.com/repos/zulip/zulip/issues/375/events","html_url":"https://github.com/zulip/zulip/issues/375","id":122170379,"number":375,"title":"Performance issues & some fixes","user":{"login":"dbiollo","id":1099813,"avatar_url":"https://avatars.githubusercontent.com/u/1099813?v=3","gravatar_id":"","url":"https://api.github.com/users/dbiollo","html_url":"https://github.com/dbiollo","followers_url":"https://api.github.com/users/dbiollo/followers","following_url":"https://api.github.com/users/dbiollo/following{/other_user}","gists_url":"https://api.github.com/users/dbiollo/gists{/gist_id}","starred_url":"https://api.github.com/users/dbiollo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dbiollo/subscriptions","organizations_url":"https://api.github.com/users/dbiollo/orgs","repos_url":"https://api.github.com/users/dbiollo/repos","events_url":"https://api.github.com/users/dbiollo/events{/privacy}","received_events_url":"https://api.github.com/users/dbiollo/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-12-15T01:21:15Z","updated_at":"2015-12-15T02:03:34Z","closed_at":null,"body":"I've been testing out zulip with large data sets (1000's of users, streams, etc). While some of the performance problems require UI design changes, there are a few optimizations that can be made directly on the backend. Here the small ones I've found so far:\r\n\r\n### 1. Case-insensitive index on UserProfile.email. \r\n\r\nCode like the following does a O(n) lookup on the UserProfile table:\r\n```\r\ndef get_user_profile_by_email(email):\r\n    return UserProfile.objects.select_related().get(email__iexact=email.strip())\r\n```\r\n\r\nAll it needs for indexed-based DB look up is an index like what is in 0003_custom_indexes.py:\r\n```sql\r\n        migrations.RunSQL(\"CREATE INDEX upper_user_email_idx ON zerver_userprofile ((upper(email)));\",\r\n                          reverse_sql=\"DROP INDEX upper_user_email_idx;\"),\r\n```\r\nThere might be other tables (prereg user?) that need this index. Alternatively, Postgres has a CITEXT column type for Case-Insensitive text.\r\n\r\n### 2. Realm.get_admin_users*\r\nTurns out that this authorization check is rather expensive when you have a lot of users. Primarily slow due to an OR with the group_permissions table. I could be wrong but it appears that Zulip doesn't use the group permissions of guardian. Best to check, but adding the \"with_group_users=False\" made this an order of magnitude faster:\r\n```diff\r\n     def get_admin_users(self):\r\n         # This method is kind of expensive, due to our complex permissions model.\r\n-        candidates = get_users_with_perms(self, only_with_perms=['administer'])\r\n+        candidates = get_users_with_perms(self, only_with_perms=['administer'], with_group_users=False)\r\n         return candidates\r\n```\r\n\r\n### 3. Filtering UserProfile on is_action, is_bot\r\nIndexing these columns helps performance in a few cases, as it is a common filter. Needs corresponding migration file.\r\n```diff\r\n class UserProfile(AbstractBaseUser, PermissionsMixin):\r\n     # Fields from models.AbstractUser minus last_name and first_name,\r\n     # which we don't use; email is modified to make it indexed and unique.\r\n     email = models.EmailField(blank=False, db_index=True, unique=True)\r\n     is_staff = models.BooleanField(default=False)\r\n-    is_active = models.BooleanField(default=True)\r\n-    is_bot = models.BooleanField(default=False)\r\n+    is_active = models.BooleanField(default=True, db_index=True)\r\n+    is_bot = models.BooleanField(default=False, db_index=True)\r\n     date_joined = models.DateTimeField(default=timezone.now)\r\n     is_mirror_dummy = models.BooleanField(default=False)\r\n```\r\n\r\n### 4. Caching entire company directory can break memcache\r\nMemcache has a reasonable 1MB size limit per cached object. When you have a company with 1000's of users, the following caching code breaks, getting error 37 from Memcache. It's more scalable to refactor the code to not fetch all company users in one go, but at least to fix the broken memcache, the cache on this function could simply be disabled:\r\n```.diff\r\n-@cache_with_key(active_user_dicts_in_realm_cache_key, timeout=3600*24*7)\r\n+# @cache_with_key(active_user_dicts_in_realm_cache_key, timeout=3600*24*7)\r\n def get_active_user_dicts_in_realm(realm):\r\n      return UserProfile.objects.filter(realm=realm, is_active=True) \\\r\n                                .values('id', 'full_name', 'short_name', 'email', 'is_bot')\r\n```\r\n\r\n### 5. alert_words_in_realm fetches too much data\r\nThe default behaviour of this function is to fetch all records for the realm, which is a lot of data for large companies. Assuming that usage of Alert Words is sparse, then filtering to exclude users who don't have any results in a much faster query.\r\n```diff\r\n import ujson\r\n+from django.db.models import Q\r\n \r\n @cache_with_key(realm_alert_words_cache_key, timeout=3600*24)\r\n def alert_words_in_realm(realm):\r\n-    users = zerver.models.UserProfile.objects.filter(realm=realm, is_active=True).values('id', 'alert_words')\r\n+    users = zerver.models.UserProfile.objects.filter(~Q(alert_words=ujson.dumps([]))).filter(realm=realm, is_active=True).values('id', 'alert_words')\r\n     all_user_words = dict((user['id'], ujson.loads(user['alert_words'])) for user in users)\r\n```\r\n\r\n### 6. DevAuth feature makes Login page too slow\r\nDevAuthBackend is pretty handy, but the login page will be pretty much unusable once you get a few thousand UserProfiles in the DB. Makes sense to cap the query at 100? Also sorting by email can be done on the DB as a minor improvement.\r\n```diff\r\n def login_page(request, **kwargs):\r\n     extra_context = kwargs.pop('extra_context', {})\r\n     if dev_auth_enabled():\r\n-        users = UserProfile.objects.filter(is_bot=False, is_active=True)\r\n-        extra_context['direct_admins'] = sorted([u.email for u in users if u.is_admin()])\r\n-        extra_context['direct_users'] = sorted([u.email for u in users if not u.is_admin()])\r\n+        MAX_DEV_BACKEND_USERS = 100\r\n+        users = UserProfile.objects.select_related('realm').filter(is_bot=False, is_active=True).order_by('email')[:MAX_DEV_BACKEND_USERS]\r\n+        extra_context['direct_admins'] = [u.email for u in users if u.is_admin()]\r\n+        extra_context['direct_users'] = [u.email for u in users if not u.is_admin()]\r\n```\r\n\r\n### 7. json_events_register - unused?\r\nThis function might not be used? At least that's what my old comment says (in my local copy).\r\nhttps://github.com/zulip/zulip/blob/e95739961f28916011d36fe58cc4eb45a20e20a3/zerver/views/__init__.py#L1157\r\n\r\n### 8. realm_user_count - replace python object count with DB count query\r\nThis will be much faster for large realms.\r\n```diff\r\n def realm_user_count(realm):\r\n-    user_dicts = get_active_user_dicts_in_realm(realm)\r\n-    return len([user_dict for user_dict in user_dicts if not user_dict[\"is_bot\"]])\r\n+    return UserProfile.objects.filter(realm=realm, is_active=True, is_bot=False).count()\r\n```"},"comment":{"url":"https://api.github.com/repos/zulip/zulip/issues/comments/164619323","html_url":"https://github.com/zulip/zulip/issues/375#issuecomment-164619323","issue_url":"https://api.github.com/repos/zulip/zulip/issues/375","id":164619323,"user":{"login":"timabbott","id":2746074,"avatar_url":"https://avatars.githubusercontent.com/u/2746074?v=3","gravatar_id":"","url":"https://api.github.com/users/timabbott","html_url":"https://github.com/timabbott","followers_url":"https://api.github.com/users/timabbott/followers","following_url":"https://api.github.com/users/timabbott/following{/other_user}","gists_url":"https://api.github.com/users/timabbott/gists{/gist_id}","starred_url":"https://api.github.com/users/timabbott/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/timabbott/subscriptions","organizations_url":"https://api.github.com/users/timabbott/orgs","repos_url":"https://api.github.com/users/timabbott/repos","events_url":"https://api.github.com/users/timabbott/events{/privacy}","received_events_url":"https://api.github.com/users/timabbott/received_events","type":"User","site_admin":false},"created_at":"2015-12-15T02:03:34Z","updated_at":"2015-12-15T02:03:34Z","body":"Also, @dbiollo if the way you're testing for these issues is something we could script and include in the automated tests that we run on every Zulip change, that would be awesome -- I always find it's best with performance work to have tools making sure issues don't regress in the future :)"}}