Commit 25818bd7 authored by Stan Hu's avatar Stan Hu

Disable method replacement in avatar loading

We've seen a significant performance penalty when using
`BatchLoader#__replace_with!`. This defines methods on the batch loader
that proxy to the 'real' object using send. The alternative is
`method_missing`, which is slower.  However, we've noticed that
`method_missing` can be faster if:

1. The objects being loaded have a large interface.
2. We don't call too many methods on the loaded object.

Avatar uploads meet both criteria above, so let's use the newly-released
feature in https://github.com/exAspArk/batch-loader/pull/45.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/60903
parent 7ae2107d
...@@ -285,7 +285,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0' ...@@ -285,7 +285,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0'
gem 'gettext_i18n_rails_js', '~> 1.3' gem 'gettext_i18n_rails_js', '~> 1.3'
gem 'gettext', '~> 3.2.2', require: false, group: :development gem 'gettext', '~> 3.2.2', require: false, group: :development
gem 'batch-loader', '~> 1.2.2' gem 'batch-loader', '~> 1.4.0'
# Perf bar # Perf bar
gem 'peek', '~> 1.0.1' gem 'peek', '~> 1.0.1'
......
...@@ -76,7 +76,7 @@ GEM ...@@ -76,7 +76,7 @@ GEM
thread_safe (~> 0.3, >= 0.3.1) thread_safe (~> 0.3, >= 0.3.1)
babosa (1.0.2) babosa (1.0.2)
base32 (0.3.2) base32 (0.3.2)
batch-loader (1.2.2) batch-loader (1.4.0)
bcrypt (3.1.12) bcrypt (3.1.12)
bcrypt_pbkdf (1.0.0) bcrypt_pbkdf (1.0.0)
benchmark-ips (2.3.0) benchmark-ips (2.3.0)
...@@ -999,7 +999,7 @@ DEPENDENCIES ...@@ -999,7 +999,7 @@ DEPENDENCIES
awesome_print awesome_print
babosa (~> 1.0.2) babosa (~> 1.0.2)
base32 (~> 0.3.0) base32 (~> 0.3.0)
batch-loader (~> 1.2.2) batch-loader (~> 1.4.0)
bcrypt_pbkdf (~> 1.0) bcrypt_pbkdf (~> 1.0)
benchmark-ips (~> 2.3.0) benchmark-ips (~> 2.3.0)
better_errors (~> 2.5.0) better_errors (~> 2.5.0)
......
...@@ -91,7 +91,8 @@ module Avatarable ...@@ -91,7 +91,8 @@ module Avatarable
private private
def retrieve_upload_from_batch(identifier) def retrieve_upload_from_batch(identifier)
BatchLoader.for(identifier: identifier, model: self).batch(key: self.class) do |upload_params, loader, args| BatchLoader.for(identifier: identifier, model: self)
.batch(key: self.class, cache: true, replace_methods: false) do |upload_params, loader, args|
model_class = args[:key] model_class = args[:key]
paths = upload_params.flat_map do |params| paths = upload_params.flat_map do |params|
params[:model].upload_paths(params[:identifier]) params[:model].upload_paths(params[:identifier])
......
---
title: Disable method replacement in avatar loading
merge_request: 27866
author:
type: performance
...@@ -771,6 +771,14 @@ describe ObjectStorage do ...@@ -771,6 +771,14 @@ describe ObjectStorage do
expect { avatars }.not_to exceed_query_limit(1) expect { avatars }.not_to exceed_query_limit(1)
end end
it 'does not attempt to replace methods' do
models.each do |model|
expect(model.avatar.upload).to receive(:method_missing).and_call_original
model.avatar.upload.path
end
end
it 'fetches a unique upload for each model' do it 'fetches a unique upload for each model' do
expect(avatars.map(&:url).uniq).to eq(avatars.map(&:url)) expect(avatars.map(&:url).uniq).to eq(avatars.map(&:url))
expect(avatars.map(&:upload).uniq).to eq(avatars.map(&:upload)) expect(avatars.map(&:upload).uniq).to eq(avatars.map(&:upload))
......
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