diff options
author | Stan Hu <stanhu@gmail.com> | 2018-12-19 15:18:20 -0800 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-12-19 15:34:29 -0800 |
commit | 52fdddbd2d1e89a6a41a59a41adf0820cc98c435 (patch) | |
tree | d55bb5d076fe1a0e23338186ccf6eabfcb26f040 | |
parent | 00096b52ced2962d237540c494f2ad6c3add70ef (diff) | |
download | gitlab-ce-52fdddbd2d1e89a6a41a59a41adf0820cc98c435.tar.gz |
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
-rw-r--r-- | app/models/concerns/avatarable.rb | 13 | ||||
-rw-r--r-- | changelogs/unreleased/sh-cache-avatar-paths.yml | 5 | ||||
-rw-r--r-- | spec/models/concerns/avatarable_spec.rb | 37 |
3 files changed, 54 insertions, 1 deletions
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index b42236c1fa2..4687ec7d166 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -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? diff --git a/changelogs/unreleased/sh-cache-avatar-paths.yml b/changelogs/unreleased/sh-cache-avatar-paths.yml new file mode 100644 index 00000000000..b59a4db413d --- /dev/null +++ b/changelogs/unreleased/sh-cache-avatar-paths.yml @@ -0,0 +1,5 @@ +--- +title: Cache avatar URLs and paths within a request +merge_request: 23950 +author: +type: performance diff --git a/spec/models/concerns/avatarable_spec.rb b/spec/models/concerns/avatarable_spec.rb index 7d617cb7b19..1ea7f2b9985 100644 --- a/spec/models/concerns/avatarable_spec.rb +++ b/spec/models/concerns/avatarable_spec.rb @@ -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 |