Commit 52fdddbd authored by Stan Hu's avatar Stan Hu

Cache avatar URLs and paths within a request

In a merge request with many discussions, the avatar URL can be called
thousands of times, inflicting a significant performance penalty
especially when avatars are stored in object storage. To mitigate this
problem, we can just cache the generated path any time it is requested.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/55355
parent 00096b52
......@@ -43,7 +43,18 @@ module Avatarable
end
def avatar_path(only_path: true, size: nil)
return unless self[:avatar].present?
unless self.try(:id)
return uncached_avatar_path(only_path: only_path, size: size)
end
# Cache this avatar path only within the request because avatars in
# object storage may be generated with time-limited, signed URLs.
key = "#{self.class.name}:#{self.id}:#{only_path}:#{size}"
Gitlab::SafeRequestStore[key] ||= uncached_avatar_path(only_path: only_path, size: size)
end
def uncached_avatar_path(only_path: true, size: nil)
return unless self.try(:avatar).present?
asset_host = ActionController::Base.asset_host
use_asset_host = asset_host.present?
......
---
title: Cache avatar URLs and paths within a request
merge_request: 23950
author:
type: performance
......@@ -33,6 +33,43 @@ describe Avatarable do
end
describe '#avatar_path' do
context 'with caching enabled', :request_store do
let!(:avatar_path) { [relative_url_root, project.avatar.local_url].join }
let!(:avatar_url) { [gitlab_host, relative_url_root, project.avatar.local_url].join }
it 'only calls local_url once' do
expect(project.avatar).to receive(:local_url).once.and_call_original
2.times do
expect(project.avatar_path).to eq(avatar_path)
end
end
it 'calls local_url twice for path and URLs' do
expect(project.avatar).to receive(:local_url).exactly(2).times.and_call_original
expect(project.avatar_path(only_path: true)).to eq(avatar_path)
expect(project.avatar_path(only_path: false)).to eq(avatar_url)
end
it 'calls local_url twice for different sizes' do
expect(project.avatar).to receive(:local_url).exactly(2).times.and_call_original
expect(project.avatar_path).to eq(avatar_path)
expect(project.avatar_path(size: 40)).to eq(avatar_path + "?width=40")
end
it 'handles unpersisted objects' do
new_project = build(:project, :with_avatar)
path = [relative_url_root, new_project.avatar.local_url].join
expect(new_project.avatar).to receive(:local_url).exactly(2).times.and_call_original
2.times do
expect(new_project.avatar_path).to eq(path)
end
end
end
using RSpec::Parameterized::TableSyntax
where(:has_asset_host, :visibility_level, :only_path, :avatar_path_prefix) do
......
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