From 3ea8df4b06e2fa57cbb72d5ddf99cc10ba2b2148 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 28 Mar 2023 14:45:45 +1000 Subject: [PATCH] DEV: Fix excessive MessageBus#last_id calls in chat (#20855) We noticed via profiling that chat was doing N redis calls per channel. Part of this was from the kick_message_bus_last_id from 520d4f504b5d0e8b76343b179f72f8a137423d52 being incorrectly passed down for DM channels rather that public channels, and the other part was from the root MessageBus channel last_id being fetched in ChannelSerializer for every single channel. This commit fixes both issues, for me going from 134 redis calls on page load to 20 locally. Also deletes an old file missed in 12a18d4d553b6c11a7f45f686ab699aa72c3312f --- .../serializers/chat/channel_serializer.rb | 18 ++- .../chat/structured_channel_serializer.rb | 10 +- .../app/serializers/chat/view_serializer.rb | 3 +- .../serializers/chat_channel_serializer.rb | 143 ------------------ plugins/chat/app/services/chat/publisher.rb | 40 ++++- 5 files changed, 53 insertions(+), 161 deletions(-) delete mode 100644 plugins/chat/app/serializers/chat_channel_serializer.rb diff --git a/plugins/chat/app/serializers/chat/channel_serializer.rb b/plugins/chat/app/serializers/chat/channel_serializer.rb index 01702b8fb48..db29fb713ad 100644 --- a/plugins/chat/app/serializers/chat/channel_serializer.rb +++ b/plugins/chat/app/serializers/chat/channel_serializer.rb @@ -109,14 +109,14 @@ module Chat end def meta - { - message_bus_last_ids: { - channel_message_bus_last_id: MessageBus.last_id("/chat/#{object.id}"), - new_messages: new_messages_message_bus_id, - new_mentions: new_mentions_message_bus_id, - kick: kick_message_bus_id, - }, + ids = { + channel_message_bus_last_id: channel_message_bus_last_id, + new_messages: new_messages_message_bus_id, + new_mentions: new_mentions_message_bus_id, } + + ids[:kick] = kick_message_bus_id if !object.direct_message_channel? + { message_bus_last_ids: ids } end alias_method :include_archive_topic_id?, :include_archive_status? @@ -127,6 +127,10 @@ module Chat private + def channel_message_bus_last_id + @opts[:channel_message_bus_last_id] || Chat::Publisher.root_message_bus_channel(object.id) + end + def new_messages_message_bus_id @opts[:new_messages_message_bus_last_id] || MessageBus.last_id(Chat::Publisher.new_messages_message_bus_channel(object.id)) diff --git a/plugins/chat/app/serializers/chat/structured_channel_serializer.rb b/plugins/chat/app/serializers/chat/structured_channel_serializer.rb index 553f85795d8..ba6de4b977c 100644 --- a/plugins/chat/app/serializers/chat/structured_channel_serializer.rb +++ b/plugins/chat/app/serializers/chat/structured_channel_serializer.rb @@ -15,6 +15,10 @@ module Chat chat_message_bus_last_ids[Chat::Publisher.new_messages_message_bus_channel(channel.id)], new_mentions_message_bus_last_id: chat_message_bus_last_ids[Chat::Publisher.new_mentions_message_bus_channel(channel.id)], + kick_message_bus_last_id: + chat_message_bus_last_ids[Chat::Publisher.kick_users_message_bus_channel(channel.id)], + channel_message_bus_last_id: + chat_message_bus_last_ids[Chat::Publisher.root_message_bus_channel(channel.id)], ) end end @@ -30,8 +34,8 @@ module Chat chat_message_bus_last_ids[Chat::Publisher.new_messages_message_bus_channel(channel.id)], new_mentions_message_bus_last_id: chat_message_bus_last_ids[Chat::Publisher.new_mentions_message_bus_channel(channel.id)], - kick_message_bus_last_id: - chat_message_bus_last_ids[Chat::Publisher.kick_users_message_bus_channel(channel.id)], + channel_message_bus_last_id: + chat_message_bus_last_ids[Chat::Publisher.root_message_bus_channel(channel.id)], ) end end @@ -87,11 +91,13 @@ module Chat message_bus_channels.push(Chat::Publisher.new_messages_message_bus_channel(channel.id)) message_bus_channels.push(Chat::Publisher.new_mentions_message_bus_channel(channel.id)) message_bus_channels.push(Chat::Publisher.kick_users_message_bus_channel(channel.id)) + message_bus_channels.push(Chat::Publisher.root_message_bus_channel(channel.id)) end object[:direct_message_channels].each do |channel| message_bus_channels.push(Chat::Publisher.new_messages_message_bus_channel(channel.id)) message_bus_channels.push(Chat::Publisher.new_mentions_message_bus_channel(channel.id)) + message_bus_channels.push(Chat::Publisher.root_message_bus_channel(channel.id)) end MessageBus.last_ids(*message_bus_channels) diff --git a/plugins/chat/app/serializers/chat/view_serializer.rb b/plugins/chat/app/serializers/chat/view_serializer.rb index 3acfffc2162..271e33ce7a7 100644 --- a/plugins/chat/app/serializers/chat/view_serializer.rb +++ b/plugins/chat/app/serializers/chat/view_serializer.rb @@ -24,7 +24,8 @@ module Chat can_moderate: scope.can_moderate_chat?(object.chat_channel.chatable), can_delete_self: scope.can_delete_own_chats?(object.chat_channel.chatable), can_delete_others: scope.can_delete_other_chats?(object.chat_channel.chatable), - channel_message_bus_last_id: MessageBus.last_id("/chat/#{object.chat_channel.id}"), + channel_message_bus_last_id: + Chat::Publisher.root_message_bus_channel(object.chat_channel.id), } meta_hash[ :can_load_more_past diff --git a/plugins/chat/app/serializers/chat_channel_serializer.rb b/plugins/chat/app/serializers/chat_channel_serializer.rb deleted file mode 100644 index 58a5939100d..00000000000 --- a/plugins/chat/app/serializers/chat_channel_serializer.rb +++ /dev/null @@ -1,143 +0,0 @@ -# frozen_string_literal: true - -class ChatChannelSerializer < ApplicationSerializer - attributes :id, - :auto_join_users, - :allow_channel_wide_mentions, - :chatable, - :chatable_id, - :chatable_type, - :chatable_url, - :description, - :title, - :slug, - :last_message_sent_at, - :status, - :archive_failed, - :archive_completed, - :archived_messages, - :total_messages, - :archive_topic_id, - :memberships_count, - :current_user_membership, - :meta, - :threading_enabled - - def threading_enabled - SiteSetting.enable_experimental_chat_threaded_discussions && object.threading_enabled - end - - def initialize(object, opts) - super(object, opts) - - @opts = opts - @current_user_membership = opts[:membership] - end - - def include_description? - object.description.present? - end - - def memberships_count - object.user_count - end - - def chatable_url - object.chatable_url - end - - def title - object.name || object.title(scope.user) - end - - def chatable - case object.chatable_type - when "Category" - BasicCategorySerializer.new(object.chatable, root: false).as_json - when "DirectMessage" - DirectMessageSerializer.new(object.chatable, scope: scope, root: false).as_json - when "Site" - nil - end - end - - def archive - object.chat_channel_archive - end - - def include_archive_status? - !object.direct_message_channel? && scope.is_staff? && archive.present? - end - - def archive_completed - archive.complete? - end - - def archive_failed - archive.failed? - end - - def archived_messages - archive.archived_messages - end - - def total_messages - archive.total_messages - end - - def archive_topic_id - archive.destination_topic_id - end - - def include_auto_join_users? - scope.can_edit_chat_channel? - end - - def include_current_user_membership? - @current_user_membership.present? - end - - def current_user_membership - @current_user_membership.chat_channel = object - - BaseChatChannelMembershipSerializer.new( - @current_user_membership, - scope: scope, - root: false, - ).as_json - end - - def meta - { - message_bus_last_ids: { - new_messages: new_messages_message_bus_id, - new_mentions: new_mentions_message_bus_id, - kick: kick_message_bus_id, - channel_message_bus_last_id: MessageBus.last_id("/chat/#{object.id}"), - }, - } - end - - alias_method :include_archive_topic_id?, :include_archive_status? - alias_method :include_total_messages?, :include_archive_status? - alias_method :include_archived_messages?, :include_archive_status? - alias_method :include_archive_failed?, :include_archive_status? - alias_method :include_archive_completed?, :include_archive_status? - - private - - def new_messages_message_bus_id - @opts[:new_messages_message_bus_last_id] || - MessageBus.last_id(Chat::Publisher.new_messages_message_bus_channel(object.id)) - end - - def new_mentions_message_bus_id - @opts[:new_mentions_message_bus_last_id] || - MessageBus.last_id(Chat::Publisher.new_mentions_message_bus_channel(object.id)) - end - - def kick_message_bus_id - @opts[:kick_message_bus_last_id] || - MessageBus.last_id(Chat::Publisher.kick_users_message_bus_channel(object.id)) - end -end diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index fc6a3f12a81..fd42e1428dd 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -6,6 +6,10 @@ module Chat "/chat/#{chat_channel_id}/new-messages" end + def self.root_message_bus_channel(chat_channel_id) + "/chat/#{chat_channel_id}" + end + def self.publish_new!(chat_channel, chat_message, staged_id) content = Chat::MessageSerializer.new( @@ -16,7 +20,7 @@ module Chat content[:staged_id] = staged_id permissions = permissions(chat_channel) - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions) + MessageBus.publish(root_message_bus_channel(chat_channel.id), content.as_json, permissions) MessageBus.publish( self.new_messages_message_bus_channel(chat_channel.id), @@ -40,7 +44,11 @@ module Chat cooked: chat_message.cooked, }, } - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions(chat_channel)) + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + content.as_json, + permissions(chat_channel), + ) end def self.publish_edit!(chat_channel, chat_message) @@ -50,7 +58,11 @@ module Chat { scope: anonymous_guardian, root: :chat_message }, ).as_json content[:type] = :edit - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions(chat_channel)) + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + content.as_json, + permissions(chat_channel), + ) end def self.publish_refresh!(chat_channel, chat_message) @@ -60,7 +72,11 @@ module Chat { scope: anonymous_guardian, root: :chat_message }, ).as_json content[:type] = :refresh - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions(chat_channel)) + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + content.as_json, + permissions(chat_channel), + ) end def self.publish_reaction!(chat_channel, chat_message, action, user, emoji) @@ -71,7 +87,11 @@ module Chat type: :reaction, chat_message_id: chat_message.id, } - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions(chat_channel)) + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + content.as_json, + permissions(chat_channel), + ) end def self.publish_presence!(chat_channel, user, typ) @@ -80,7 +100,7 @@ module Chat def self.publish_delete!(chat_channel, chat_message) MessageBus.publish( - "/chat/#{chat_channel.id}", + root_message_bus_channel(chat_channel.id), { type: "delete", deleted_id: chat_message.id, deleted_at: chat_message.deleted_at }, permissions(chat_channel), ) @@ -88,7 +108,7 @@ module Chat def self.publish_bulk_delete!(chat_channel, deleted_message_ids) MessageBus.publish( - "/chat/#{chat_channel.id}", + root_message_bus_channel(chat_channel.id), { typ: "bulk_delete", deleted_ids: deleted_message_ids, deleted_at: Time.zone.now }, permissions(chat_channel), ) @@ -101,7 +121,11 @@ module Chat { scope: anonymous_guardian, root: :chat_message }, ).as_json content[:type] = :restore - MessageBus.publish("/chat/#{chat_channel.id}", content.as_json, permissions(chat_channel)) + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + content.as_json, + permissions(chat_channel), + ) end def self.publish_flag!(chat_message, user, reviewable, score)