messages: Optimize get_recent_private_conversations.

Previously, get_recent_private_messages could take 100ms-1s to run,
contributing a substantial portion of the total runtime of `/`.

We fix this by taking advantage of the recent denormalization of
personal_recipient into the UserProfile model, allowing us to avoid
the complex join with Recipient that was previously required.

The change that requires additional commentary is the change to the
main, big SQL query:
1. We eliminate UserMessage table from the query, because the condition
m.recipient_id=%(my_recipient_id)d
implies m is a personal message to the user being processed - so joining
with usermessage to check for user_profile_id and flags&2048 (which
checks the message is private) is redundant.
2. We only need to join the Message table with UserProfile
(on sender_id) and get the sender's personal_recipient_id from their
UserProfile row.

Fixes #13437.
This commit is contained in:
Mateusz Mandera
2019-11-28 20:31:18 +01:00
committed by Tim Abbott
parent 8acfa17fe6
commit dda3ff41e1
3 changed files with 12 additions and 22 deletions

View File

@@ -1033,11 +1033,9 @@ def get_recent_conversations_recipient_id(user_profile: UserProfile,
get_recent_private_conversations would have used to record that get_recent_private_conversations would have used to record that
message in its data structure. message in its data structure.
""" """
my_recipient_id = Recipient.objects.get(type=Recipient.PERSONAL, my_recipient_id = user_profile.id
type_id=user_profile.id).id
if recipient_id == my_recipient_id: if recipient_id == my_recipient_id:
return Recipient.objects.get(type=Recipient.PERSONAL, return UserProfile.objects.values_list('recipient_id', flat=True).get(id=sender_id)
type_id=sender_id).id
return recipient_id return recipient_id
def get_recent_private_conversations(user_profile: UserProfile) -> Dict[int, Dict[str, Any]]: def get_recent_private_conversations(user_profile: UserProfile) -> Dict[int, Dict[str, Any]]:
@@ -1071,8 +1069,7 @@ def get_recent_private_conversations(user_profile: UserProfile) -> Dict[int, Dic
RECENT_CONVERSATIONS_LIMIT = 1000 RECENT_CONVERSATIONS_LIMIT = 1000
recipient_map = {} recipient_map = {}
my_recipient_id = Recipient.objects.get(type=Recipient.PERSONAL, my_recipient_id = user_profile.recipient_id
type_id=user_profile.id).id
query = ''' query = '''
SELECT SELECT
@@ -1095,22 +1092,15 @@ def get_recent_private_conversations(user_profile: UserProfile) -> Dict[int, Dic
LIMIT %(conversation_limit)d) LIMIT %(conversation_limit)d)
UNION ALL UNION ALL
(SELECT (SELECT
um.message_id AS message_id, m.id AS message_id,
r.id AS recipient_id sender_profile.recipient_id AS recipient_id
FROM FROM
zerver_usermessage um
JOIN
zerver_message m zerver_message m
ON
um.message_id = m.id
JOIN JOIN
zerver_recipient r zerver_userprofile sender_profile
ON ON
r.type = 1 AND m.sender_id = sender_profile.id
r.type_id = m.sender_id
WHERE WHERE
um.user_profile_id=%(user_profile_id)d AND
um.flags & 2048 <> 0 AND
m.recipient_id=%(my_recipient_id)d m.recipient_id=%(my_recipient_id)d
ORDER BY message_id DESC ORDER BY message_id DESC
LIMIT %(conversation_limit)d) LIMIT %(conversation_limit)d)

View File

@@ -3505,7 +3505,7 @@ class FetchQueriesTest(ZulipTestCase):
client_gravatar=False, client_gravatar=False,
) )
self.assert_length(queries, 33) self.assert_length(queries, 32)
expected_counts = dict( expected_counts = dict(
alert_words=0, alert_words=0,
@@ -3526,7 +3526,7 @@ class FetchQueriesTest(ZulipTestCase):
realm_filters=1, realm_filters=1,
realm_user=3, realm_user=3,
realm_user_groups=2, realm_user_groups=2,
recent_private_conversations=2, recent_private_conversations=1,
starred_messages=1, starred_messages=1,
stream=2, stream=2,
stop_words=0, stop_words=0,

View File

@@ -245,7 +245,7 @@ class HomeTest(ZulipTestCase):
self.assertEqual(set(result["Cache-Control"].split(", ")), self.assertEqual(set(result["Cache-Control"].split(", ")),
{"must-revalidate", "no-store", "no-cache"}) {"must-revalidate", "no-store", "no-cache"})
self.assert_length(queries, 45) self.assert_length(queries, 44)
self.assert_length(cache_mock.call_args_list, 7) self.assert_length(cache_mock.call_args_list, 7)
html = result.content.decode('utf-8') html = result.content.decode('utf-8')
@@ -311,7 +311,7 @@ class HomeTest(ZulipTestCase):
result = self._get_home_page() result = self._get_home_page()
self.assertEqual(result.status_code, 200) self.assertEqual(result.status_code, 200)
self.assert_length(cache_mock.call_args_list, 6) self.assert_length(cache_mock.call_args_list, 6)
self.assert_length(queries, 42) self.assert_length(queries, 41)
@slow("Creates and subscribes 10 users in a loop. Should use bulk queries.") @slow("Creates and subscribes 10 users in a loop. Should use bulk queries.")
def test_num_queries_with_streams(self) -> None: def test_num_queries_with_streams(self) -> None:
@@ -343,7 +343,7 @@ class HomeTest(ZulipTestCase):
with queries_captured() as queries2: with queries_captured() as queries2:
result = self._get_home_page() result = self._get_home_page()
self.assert_length(queries2, 39) self.assert_length(queries2, 38)
# Do a sanity check that our new streams were in the payload. # Do a sanity check that our new streams were in the payload.
html = result.content.decode('utf-8') html = result.content.decode('utf-8')