From 2a4c62a326c8213331c1775c6e2fc826d9dbb2ef Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Tue, 19 May 2020 13:52:49 +0530 Subject: [PATCH] update_to_dict_cache: Use bulk queries when preparing msgs for cache. During events such as stream / topic name edit for a topic, we were running queries to db in loop for each message for reactions, submessages and realm_id. This commit reduces the queries to be done only for realm_id, which is yet to be fixed. This is accomplished by building messages with empty reactions and submessages and then updating them in the messages using bulk queries. --- zerver/lib/actions.py | 10 +++--- zerver/lib/cache_helpers.py | 2 +- zerver/lib/message.py | 61 +++++++++++++++++++---------------- zerver/tests/test_messages.py | 12 +++---- 4 files changed, 46 insertions(+), 39 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index c127279112..e52e5451d8 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -4308,11 +4308,11 @@ def update_to_dict_cache(changed_messages: List[Message]) -> List[int]: messages).""" items_for_remote_cache = {} message_ids = [] - for changed_message in changed_messages: - message_ids.append(changed_message.id) - key = to_dict_cache_key_id(changed_message.id) - value = MessageDict.to_dict_uncached(changed_message) - items_for_remote_cache[key] = (value,) + changed_messages_to_dict = MessageDict.to_dict_uncached(changed_messages) + for msg_id, msg in changed_messages_to_dict.items(): + message_ids.append(msg_id) + key = to_dict_cache_key_id(msg_id) + items_for_remote_cache[key] = (msg,) cache_set_many(items_for_remote_cache) return message_ids diff --git a/zerver/lib/cache_helpers.py b/zerver/lib/cache_helpers.py index 5dd6cc8953..a064329a22 100644 --- a/zerver/lib/cache_helpers.py +++ b/zerver/lib/cache_helpers.py @@ -41,7 +41,7 @@ def message_cache_items(items_for_remote_cache: Dict[str, Tuple[bytes]], commented out for a while. ''' key = to_dict_cache_key_id(message.id) - value = MessageDict.to_dict_uncached(message) + value = MessageDict.to_dict_uncached([message])[message.id] items_for_remote_cache[key] = (value,) def user_cache_items(items_for_remote_cache: Dict[str, Tuple[UserProfile]], diff --git a/zerver/lib/message.py b/zerver/lib/message.py index 06df627629..c2abf934f1 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -171,7 +171,7 @@ def stringify_message_dict(message_dict: Dict[str, Any]) -> bytes: @cache_with_key(to_dict_cache_key, timeout=3600*24) def message_to_dict_json(message: Message) -> bytes: - return MessageDict.to_dict_uncached(message) + return MessageDict.to_dict_uncached([message])[message.id] def save_message_rendered_content(message: Message, content: str) -> str: rendered_content = render_markdown(message, content, realm=message.get_realm()) @@ -263,10 +263,6 @@ class MessageDict: del obj['sender_is_mirror_dummy'] @staticmethod - def to_dict_uncached(message: Message) -> bytes: - dct = MessageDict.to_dict_uncached_helper(message) - return stringify_message_dict(dct) - def sew_submessages_and_reactions_to_msgs(messages: List[Dict[str, Any]]) -> List[Dict[str, Any]]: msg_ids = [msg['id'] for msg in messages] submessages = SubMessage.get_raw_db_rows(msg_ids) @@ -276,26 +272,37 @@ class MessageDict: return sew_messages_and_reactions(messages, reactions) @staticmethod - def to_dict_uncached_helper(message: Message) -> Dict[str, Any]: - return MessageDict.build_message_dict( - message = message, - message_id = message.id, - last_edit_time = message.last_edit_time, - edit_history = message.edit_history, - content = message.content, - topic_name = message.topic_name(), - date_sent = message.date_sent, - rendered_content = message.rendered_content, - rendered_content_version = message.rendered_content_version, - sender_id = message.sender.id, - sender_realm_id = message.sender.realm_id, - sending_client_name = message.sending_client.name, - recipient_id = message.recipient.id, - recipient_type = message.recipient.type, - recipient_type_id = message.recipient.type_id, - reactions = Reaction.get_raw_db_rows([message.id]), - submessages = SubMessage.get_raw_db_rows([message.id]), - ) + def to_dict_uncached(messages: List[Message]) -> Dict[int, bytes]: + messages_dict = MessageDict.to_dict_uncached_helper(messages) + encoded_messages = {msg['id']: stringify_message_dict(msg) for msg in messages_dict} + return encoded_messages + + @staticmethod + def to_dict_uncached_helper(messages: List[Message]) -> List[Dict[str, Any]]: + message_dicts = [ + MessageDict.build_message_dict( + message = message, + message_id = message.id, + last_edit_time = message.last_edit_time, + edit_history = message.edit_history, + content = message.content, + topic_name = message.topic_name(), + date_sent = message.date_sent, + rendered_content = message.rendered_content, + rendered_content_version = message.rendered_content_version, + sender_id = message.sender.id, + sender_realm_id = message.sender.realm_id, + sending_client_name = message.sending_client.name, + recipient_id = message.recipient.id, + recipient_type = message.recipient.type, + recipient_type_id = message.recipient.type_id, + ) for message in messages + ] + + # We do bulk query for reactions and submessages to avoid + # running queries for them in a loop. + MessageDict.sew_submessages_and_reactions_to_msgs(message_dicts) + return message_dicts @staticmethod def get_raw_db_rows(needed_ids: List[int]) -> List[Dict[str, Any]]: @@ -363,8 +370,8 @@ class MessageDict: recipient_id: int, recipient_type: int, recipient_type_id: int, - reactions: List[Dict[str, Any]], - submessages: List[Dict[str, Any]] + reactions: List[Dict[str, Any]]=[], + submessages: List[Dict[str, Any]]=[] ) -> Dict[str, Any]: obj = dict( diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index 9e840c4ea5..f71c2d2de4 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -1313,7 +1313,7 @@ class MessageDictTest(ZulipTestCase): apply_markdown: bool, client_gravatar: bool) -> Dict[str, Any]: msg = reload_message(msg_id) - unhydrated_dict = MessageDict.to_dict_uncached_helper(msg) + unhydrated_dict = MessageDict.to_dict_uncached_helper([msg])[0] # The next step mutates the dict in place # for performance reasons. MessageDict.post_process_dicts( @@ -1492,7 +1492,7 @@ class MessageDictTest(ZulipTestCase): return Message.objects.get(id=msg_id) def assert_topic_links(links: List[str], msg: Message) -> None: - dct = MessageDict.to_dict_uncached_helper(msg) + dct = MessageDict.to_dict_uncached_helper([msg])[0] self.assertEqual(dct[TOPIC_LINKS], links) # Send messages before and after saving the realm filter from each user. @@ -2687,10 +2687,10 @@ class EditMessageTest(ZulipTestCase): # Check number of queries performed with queries_captured() as queries: - for msg in messages: - MessageDict.to_dict_uncached(msg) - # 3 query for realm_id, reactions & submessage per message = 6 - self.assertEqual(len(queries), 6) + MessageDict.to_dict_uncached(messages) + # 1 query for realm_id per message = 2 + # 1 query each for reactions & submessage for all messages = 2 + self.assertEqual(len(queries), 4) def test_save_message(self) -> None: """This is also tested by a client test, but here we can verify