Improved performance of notification preloading (#15640)

* Improved performance of notification preloading

* Remove Cacheable from Notification

* Fix test

diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb
index 522c35ba5483e5fb89f0c1003dc2519e7ecb2b94..f9d780839849d39341c9efd3212bb14a77d65cbf 100644 (file)
--- a/app/controllers/api/v1/notifications_controller.rb
+++ b/app/controllers/api/v1/notifications_controller.rb
@@ -31,12 +31,13 @@ class Api::V1::NotificationsController < Api::BaseController
   private
 
   def load_notifications
-    cache_collection_paginated_by_id(
-      browserable_account_notifications,
-      Notification,
+    notifications = browserable_account_notifications.includes(from_account: :account_stat).to_a_paginated_by_id(
       limit_param(DEFAULT_NOTIFICATIONS_LIMIT),
       params_slice(:max_id, :since_id, :min_id)
     )
+    Notification.preload_cache_collection_target_statuses(notifications) do |target_statuses|
+      cache_collection(target_statuses, Status)
+    end
   end
 
   def browserable_account_notifications

diff --git a/app/models/notification.rb b/app/models/notification.rb
index 5e9ea62a0d527422e1b486a5e50311d46b49724a..98a6a618f5fe2b76f032eb47e0a8e20d6a5c85f7 100644 (file)
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -17,7 +17,6 @@ class Notification < ApplicationRecord
   self.inheritance_column = nil
 
   include Paginable
-  include Cacheable
 
   LEGACY_TYPE_CLASS_MAP = {
     'Mention'       => :mention,
@@ -38,7 +37,13 @@ class Notification < ApplicationRecord
     poll
   ).freeze
 
-  STATUS_INCLUDES = [:account, :application, :preloadable_poll, :media_attachments, :tags, active_mentions: :account, reblog: [:account, :application, :preloadable_poll, :media_attachments, :tags, active_mentions: :account]].freeze
+  TARGET_STATUS_INCLUDES_BY_TYPE = {
+    status: :status,
+    reblog: [status: :reblog],
+    mention: [mention: :status],
+    favourite: [favourite: :status],
+    poll: [poll: :status],
+  }.freeze
 
   belongs_to :account, optional: true
   belongs_to :from_account, class_name: 'Account', optional: true
@@ -65,8 +70,6 @@ class Notification < ApplicationRecord
     end
   }
 
-  cache_associated :from_account, status: STATUS_INCLUDES, mention: [status: STATUS_INCLUDES], favourite: [:account, status: STATUS_INCLUDES], follow: :account, follow_request: :account, poll: [status: STATUS_INCLUDES]
-
   def type
     @type ||= (super || LEGACY_TYPE_CLASS_MAP[activity_type]).to_sym
   end
@@ -87,21 +90,40 @@ class Notification < ApplicationRecord
   end
 
   class << self
-    def cache_ids
-      select(:id, :updated_at, :activity_type, :activity_id)
-    end
-
-    def reload_stale_associations!(cached_items)
-      account_ids = (cached_items.map(&:from_account_id) + cached_items.filter_map { |item| item.target_status&.account_id }).uniq
-
-      return if account_ids.empty?
-
-      accounts = Account.where(id: account_ids).includes(:account_stat).index_by(&:id)
+    def preload_cache_collection_target_statuses(notifications, &_block)
+      notifications.group_by(&:type).each do |type, grouped_notifications|
+        associations = TARGET_STATUS_INCLUDES_BY_TYPE[type]
+        next unless associations
+
+        # Instead of using the usual `includes`, manually preload each type.
+        # If polymorphic associations are loaded with the usual `includes`, other types of associations will be loaded more.
+        ActiveRecord::Associations::Preloader.new.preload(grouped_notifications, associations)
+      end
 
-      cached_items.each do |item|
-        item.from_account = accounts[item.from_account_id]
-        item.target_status.account = accounts[item.target_status.account_id] if item.target_status
+      unique_target_statuses = notifications.map(&:target_status).compact.uniq
+      # Call cache_collection in block
+      cached_statuses_by_id = yield(unique_target_statuses).index_by(&:id)
+
+      notifications.each do |notification|
+        next if notification.target_status.nil?
+
+        cached_status = cached_statuses_by_id[notification.target_status.id]
+
+        case notification.type
+        when :status
+          notification.status = cached_status
+        when :reblog
+          notification.status.reblog = cached_status
+        when :favourite
+          notification.favourite.status = cached_status
+        when :mention
+          notification.mention.status = cached_status
+        when :poll
+          notification.poll.status = cached_status
+        end
       end
+
+      notifications
     end
   end
 

diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 63ae27a924ef98ec1120676cbec9a1affec43d9c..e73a08a0e998d040c74030fdf518689d586e4be0 100644 (file)
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -349,10 +349,6 @@ describe ApplicationController, type: :controller do
       expect(C.new.cache_collection(raw, Object)).to eq raw
     end
 
-    context 'Notification' do
-      include_examples 'cacheable', :notification, Notification
-    end
-
     context 'Status' do
       include_examples 'cacheable', :status, Status
     end

diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 0a045904bb21ab2b46adbd6f43baf8888ad3f41e..1e9e45d8d6ddfd48aa14d38defebc7071858b59b 100644 (file)
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -56,47 +56,114 @@ RSpec.describe Notification, type: :model do
     end
   end
 
-  describe '.reload_stale_associations!' do
-    context 'account_ids are empty' do
-      let(:cached_items) { [] }
+  describe '.preload_cache_collection_target_statuses' do
+    subject do
+      described_class.preload_cache_collection_target_statuses(notifications) do |target_statuses|
+        # preload account for testing instead of using cache_collection
+        Status.preload(:account).where(id: target_statuses.map(&:id))
+      end
+    end
 
-      subject { described_class.reload_stale_associations!(cached_items) }
+    context 'notifications are empty' do
+      let(:notifications) { [] }
 
-      it 'returns nil' do
-        is_expected.to be nil
+      it 'returns []' do
+        is_expected.to eq []
       end
     end
 
-    context 'account_ids are present' do
+    context 'notifications are present' do
       before do
-        allow(accounts_with_ids).to receive(:[]).with(stale_account1.id).and_return(account1)
-        allow(accounts_with_ids).to receive(:[]).with(stale_account2.id).and_return(account2)
-        allow(Account).to receive_message_chain(:where, :includes, :index_by).and_return(accounts_with_ids)
+        notifications.each(&:reload)
       end
 
-      let(:cached_items) do
+      let(:mention) { Fabricate(:mention) }
+      let(:status) { Fabricate(:status) }
+      let(:reblog) { Fabricate(:status, reblog: Fabricate(:status)) }
+      let(:follow) { Fabricate(:follow) }
+      let(:follow_request) { Fabricate(:follow_request) }
+      let(:favourite) { Fabricate(:favourite) }
+      let(:poll) { Fabricate(:poll) }
+
+      let(:notifications) do
         [
-          Fabricate(:notification, activity: Fabricate(:status)),
-          Fabricate(:notification, activity: Fabricate(:follow)),
+          Fabricate(:notification, type: :mention, activity: mention),
+          Fabricate(:notification, type: :status, activity: status),
+          Fabricate(:notification, type: :reblog, activity: reblog),
+          Fabricate(:notification, type: :follow, activity: follow),
+          Fabricate(:notification, type: :follow_request, activity: follow_request),
+          Fabricate(:notification, type: :favourite, activity: favourite),
+          Fabricate(:notification, type: :poll, activity: poll),
         ]
       end
 
-      let(:stale_account1) { cached_items[0].from_account }
-      let(:stale_account2) { cached_items[1].from_account }
-
-      let(:account1) { Fabricate(:account) }
-      let(:account2) { Fabricate(:account) }
-
-      let(:accounts_with_ids) { { account1.id => account1, account2.id => account2 } }
-
-      it 'reloads associations' do
-        expect(cached_items[0].from_account).to be stale_account1
-        expect(cached_items[1].from_account).to be stale_account2
-
-        described_class.reload_stale_associations!(cached_items)
+      it 'preloads target status' do
+        # mention
+        expect(subject[0].type).to eq :mention
+        expect(subject[0].association(:mention)).to be_loaded
+        expect(subject[0].mention.association(:status)).to be_loaded
+
+        # status
+        expect(subject[1].type).to eq :status
+        expect(subject[1].association(:status)).to be_loaded
+
+        # reblog
+        expect(subject[2].type).to eq :reblog
+        expect(subject[2].association(:status)).to be_loaded
+        expect(subject[2].status.association(:reblog)).to be_loaded
+
+        # follow: nothing
+        expect(subject[3].type).to eq :follow
+        expect(subject[3].target_status).to be_nil
+
+        # follow_request: nothing
+        expect(subject[4].type).to eq :follow_request
+        expect(subject[4].target_status).to be_nil
+
+        # favourite
+        expect(subject[5].type).to eq :favourite
+        expect(subject[5].association(:favourite)).to be_loaded
+        expect(subject[5].favourite.association(:status)).to be_loaded
+
+        # poll
+        expect(subject[6].type).to eq :poll
+        expect(subject[6].association(:poll)).to be_loaded
+        expect(subject[6].poll.association(:status)).to be_loaded
+      end
 
-        expect(cached_items[0].from_account).to be account1
-        expect(cached_items[1].from_account).to be account2
+      it 'replaces to cached status' do
+        # mention
+        expect(subject[0].type).to eq :mention
+        expect(subject[0].target_status.association(:account)).to be_loaded
+        expect(subject[0].target_status).to eq mention.status
+
+        # status
+        expect(subject[1].type).to eq :status
+        expect(subject[1].target_status.association(:account)).to be_loaded
+        expect(subject[1].target_status).to eq status
+
+        # reblog
+        expect(subject[2].type).to eq :reblog
+        expect(subject[2].target_status.association(:account)).to be_loaded
+        expect(subject[2].target_status).to eq reblog.reblog
+
+        # follow: nothing
+        expect(subject[3].type).to eq :follow
+        expect(subject[3].target_status).to be_nil
+
+        # follow_request: nothing
+        expect(subject[4].type).to eq :follow_request
+        expect(subject[4].target_status).to be_nil
+
+        # favourite
+        expect(subject[5].type).to eq :favourite
+        expect(subject[5].target_status.association(:account)).to be_loaded
+        expect(subject[5].target_status).to eq favourite.status
+
+        # poll
+        expect(subject[6].type).to eq :poll
+        expect(subject[6].target_status.association(:account)).to be_loaded
+        expect(subject[6].target_status).to eq poll.status
       end
     end
   end