diff options
| author | Stan Hu <stanhu@gmail.com> | 2019-04-29 17:07:42 -0700 |
|---|---|---|
| committer | Stan Hu <stanhu@gmail.com> | 2019-04-29 21:29:25 -0700 |
| commit | 25818bd7ae765422c934d0a32efb4ba353d11183 (patch) | |
| tree | bbef891f40c54ee59e79fa04f538562bc11c1afb | |
| parent | 7ae2107d9ebca0adecc8a21cacd1bfb6e89ee3ab (diff) | |
| download | gitlab-ce-25818bd7ae765422c934d0a32efb4ba353d11183.tar.gz | |
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
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/models/concerns/avatarable.rb | 3 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-disable-batch-load-replace-methods.yml | 5 | ||||
| -rw-r--r-- | spec/uploaders/object_storage_spec.rb | 8 |
5 files changed, 18 insertions, 4 deletions
@@ -285,7 +285,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0' gem 'gettext_i18n_rails_js', '~> 1.3' gem 'gettext', '~> 3.2.2', require: false, group: :development -gem 'batch-loader', '~> 1.2.2' +gem 'batch-loader', '~> 1.4.0' # Perf bar gem 'peek', '~> 1.0.1' diff --git a/Gemfile.lock b/Gemfile.lock index c08f0c24ba6..c5ad2357434 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -76,7 +76,7 @@ GEM thread_safe (~> 0.3, >= 0.3.1) babosa (1.0.2) base32 (0.3.2) - batch-loader (1.2.2) + batch-loader (1.4.0) bcrypt (3.1.12) bcrypt_pbkdf (1.0.0) benchmark-ips (2.3.0) @@ -999,7 +999,7 @@ DEPENDENCIES awesome_print babosa (~> 1.0.2) base32 (~> 0.3.0) - batch-loader (~> 1.2.2) + batch-loader (~> 1.4.0) bcrypt_pbkdf (~> 1.0) benchmark-ips (~> 2.3.0) better_errors (~> 2.5.0) diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb index 4687ec7d166..80278e07e65 100644 --- a/app/models/concerns/avatarable.rb +++ b/app/models/concerns/avatarable.rb @@ -91,7 +91,8 @@ module Avatarable private 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] paths = upload_params.flat_map do |params| params[:model].upload_paths(params[:identifier]) diff --git a/changelogs/unreleased/sh-disable-batch-load-replace-methods.yml b/changelogs/unreleased/sh-disable-batch-load-replace-methods.yml new file mode 100644 index 00000000000..00f897ac4b1 --- /dev/null +++ b/changelogs/unreleased/sh-disable-batch-load-replace-methods.yml @@ -0,0 +1,5 @@ +--- +title: Disable method replacement in avatar loading +merge_request: 27866 +author: +type: performance diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 9ce9a353913..a62830c35f1 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -771,6 +771,14 @@ describe ObjectStorage do expect { avatars }.not_to exceed_query_limit(1) 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 expect(avatars.map(&:url).uniq).to eq(avatars.map(&:url)) expect(avatars.map(&:upload).uniq).to eq(avatars.map(&:upload)) |