Commit 90bca8ea authored by Sean McGivern's avatar Sean McGivern

Fix an N+1 in avatar URLs

This is tricky: the query was being run in
`ObjectStorage::Extension::RecordsUploads#retrieve_from_store!`, but we can't
just add batch loading there, because the `#upload=` method there would use the
result immediately, making the batch only have one item.

Instead, we can pre-emptively add an item to the batch whenever an avatarable
object is initialized, and then reuse that batch item in
`#retrieve_from_store!`. However, this also has problems:

1. There is a lot of logic in `Avatarable#retrieve_upload_from_batch`.
2. Some of that logic constructs a 'fake' model for the batch key. This should
   be fine, because of ActiveRecord's override of `#==`, but it relies on that
   staying the same.
parent 444a334b
......@@ -4,11 +4,14 @@ module Avatarable
included do
prepend ShadowMethods
include ObjectStorage::BackgroundMove
include Gitlab::Utils::StrongMemoize
validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
mount_uploader :avatar, AvatarUploader
after_initialize :add_avatar_to_batch
end
module ShadowMethods
......@@ -18,6 +21,17 @@ module Avatarable
avatar_path(only_path: args.fetch(:only_path, true)) || super
end
def retrieve_upload(identifier, paths)
upload = retrieve_upload_from_batch(identifier)
# This fallback is needed when deleting an upload, because we may have
# already been removed from the DB. We have to check an explicit `#nil?`
# because it's a BatchLoader instance.
upload = super if upload.nil?
upload
end
end
def avatar_type
......@@ -52,4 +66,37 @@ module Avatarable
url_base + avatar.local_url
end
# Path that is persisted in the tracking Upload model. Used to fetch the
# upload from the model.
def upload_paths(identifier)
avatar_mounter.blank_uploader.store_dirs.map { |store, path| File.join(path, identifier) }
end
private
def retrieve_upload_from_batch(identifier)
BatchLoader.for(identifier: identifier, model: self).batch(key: self.class) do |upload_params, loader, args|
model_class = args[:key]
paths = upload_params.flat_map do |params|
params[:model].upload_paths(params[:identifier])
end
Upload.where(uploader: AvatarUploader, path: paths).find_each do |upload|
model = model_class.instantiate('id' => upload.model_id)
loader.call({ model: model, identifier: File.basename(upload.path) }, upload)
end
end
end
def add_avatar_to_batch
return unless avatar_mounter
avatar_mounter.read_identifiers.each { |identifier| retrieve_upload_from_batch(identifier) }
end
def avatar_mounter
strong_memoize(:avatar_mounter) { _mounter(:avatar) }
end
end
......@@ -36,4 +36,8 @@ module WithUploads
upload.destroy
end
end
def retrieve_upload(_identifier, paths)
uploads.find_by(path: paths)
end
end
......@@ -443,6 +443,10 @@ class Note < ActiveRecord::Base
super.merge(noteable: noteable)
end
def retrieve_upload(_identifier, paths)
Upload.find_by(model: self, path: paths)
end
private
def keep_around_commit
......
......@@ -3,4 +3,5 @@ class PersonalSnippet < Snippet
document_type 'snippet'
index_name [Rails.application.class.parent_name.downcase, Rails.env].join('-')
include Elastic::SnippetsSearch
include WithUploads
end
......@@ -35,7 +35,7 @@ module ObjectStorage
unless current_upload_satisfies?(paths, model)
# the upload we already have isn't right, find the correct one
self.upload = uploads.find_by(model: model, path: paths)
self.upload = model&.retrieve_upload(identifier, paths)
end
super
......@@ -48,7 +48,7 @@ module ObjectStorage
end
def upload=(upload)
return unless upload
return if upload.nil?
self.object_store = upload.store
super
......
---
title: Fix an N+1 when loading user avatars
merge_request:
author:
type: performance
......@@ -671,4 +671,26 @@ describe ObjectStorage do
end
end
end
describe '#retrieve_from_store!' do
[:group, :project, :user].each do |model|
context "for #{model}s" do
let(:models) { create_list(model, 3, :with_avatar).map(&:reload) }
let(:avatars) { models.map(&:avatar) }
it 'batches fetching uploads from the database' do
# Ensure that these are all created and fully loaded before we start
# running queries for avatars
models
expect { avatars }.not_to exceed_query_limit(1)
end
it 'fetches a unique upload for each model' do
expect(avatars.map(&:url).uniq).to eq(avatars.map(&:url))
expect(avatars.map(&:upload).uniq).to eq(avatars.map(&:upload))
end
end
end
end
end
......@@ -125,8 +125,10 @@ describe ObjectStorage::BackgroundMoveWorker do
it "migrates file to remote storage" do
perform
project.reload
BatchLoader::Executor.clear_current
expect(project.reload.avatar.file_storage?).to be_falsey
expect(project.avatar).not_to be_file_storage
end
end
......@@ -137,7 +139,7 @@ describe ObjectStorage::BackgroundMoveWorker do
it "migrates file to remote storage" do
perform
expect(project.reload.avatar.file_storage?).to be_falsey
expect(project.reload.avatar).not_to be_file_storage
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment