Improved performance of notification preloading (#15640)
* Improved performance of notification preloading * Remove Cacheable from Notification * Fix test
This commit is contained in:
abcang
GitHub
parent
c8c764dd8b
commit
7ab53f221a
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 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
|
||||
|
||||
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)
|
||||
|
||||
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
|
||||
# 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
|
||||
|
||||
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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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) { [] }
|
||||
|
||||
subject { described_class.reload_stale_associations!(cached_items) }
|
||||
|
||||
it 'returns nil' do
|
||||
is_expected.to be nil
|
||||
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
|
||||
|
||||
context 'account_ids are present' do
|
||||
context 'notifications are empty' do
|
||||
let(:notifications) { [] }
|
||||
|
||||
it 'returns []' do
|
||||
is_expected.to eq []
|
||||
end
|
||||
end
|
||||
|
||||
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 }
|
||||
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
|
||||
|
||||
let(:account1) { Fabricate(:account) }
|
||||
let(:account2) { Fabricate(:account) }
|
||||
# status
|
||||
expect(subject[1].type).to eq :status
|
||||
expect(subject[1].association(:status)).to be_loaded
|
||||
|
||||
let(:accounts_with_ids) { { account1.id => account1, account2.id => account2 } }
|
||||
# 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
|
||||
|
||||
it 'reloads associations' do
|
||||
expect(cached_items[0].from_account).to be stale_account1
|
||||
expect(cached_items[1].from_account).to be stale_account2
|
||||
# follow: nothing
|
||||
expect(subject[3].type).to eq :follow
|
||||
expect(subject[3].target_status).to be_nil
|
||||
|
||||
described_class.reload_stale_associations!(cached_items)
|
||||
# follow_request: nothing
|
||||
expect(subject[4].type).to eq :follow_request
|
||||
expect(subject[4].target_status).to be_nil
|
||||
|
||||
expect(cached_items[0].from_account).to be account1
|
||||
expect(cached_items[1].from_account).to be account2
|
||||
# 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
|
||||
|
||||
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
|
||||
|
|
|
|||
Reference in New Issue