From 0844ba04b518b4cd0a326a2ee0e8431eeb26950d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 31 Oct 2018 12:51:11 -0700 Subject: Significantly cut memory usage and SQL queries when reloading diffs By preloading certain models with the diff, we can eliminate many N+1 queries. For a push to the staging GitLab.com www-gitlab-com repository, this eliminates over 3000 SQL queries and appears to bring down the RSS usage by several gigabytes. Relates to https://gitlab.com/gitlab-org/gitlab-ce/issues/49703 --- app/services/merge_requests/reload_diffs_service.rb | 5 ++++- changelogs/unreleased/sh-optimize-reload-diffs-service.yml | 5 +++++ spec/services/merge_requests/reload_diffs_service_spec.rb | 11 +++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-optimize-reload-diffs-service.yml diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb index b4d48fe92ad..44997cf012b 100644 --- a/app/services/merge_requests/reload_diffs_service.rb +++ b/app/services/merge_requests/reload_diffs_service.rb @@ -36,7 +36,10 @@ module MergeRequests # Remove cache for all diffs on this MR. Do not use the association on the # model, as that will interfere with other actions happening when # reloading the diff. - MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff| + MergeRequestDiff + .where(merge_request: merge_request) + .preload(:merge_request_diff_files, merge_request: :target_project) + .find_each do |merge_request_diff| next if merge_request_diff == new_diff cacheable_collection(merge_request_diff).clear_cache diff --git a/changelogs/unreleased/sh-optimize-reload-diffs-service.yml b/changelogs/unreleased/sh-optimize-reload-diffs-service.yml new file mode 100644 index 00000000000..422102560ed --- /dev/null +++ b/changelogs/unreleased/sh-optimize-reload-diffs-service.yml @@ -0,0 +1,5 @@ +--- +title: Significantly cut memory usage and SQL queries when reloading diffs +merge_request: 22725 +author: +type: performance diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb index 21f369a3818..546c9f277c5 100644 --- a/spec/services/merge_requests/reload_diffs_service_spec.rb +++ b/spec/services/merge_requests/reload_diffs_service_spec.rb @@ -60,6 +60,17 @@ describe MergeRequests::ReloadDiffsService, :use_clean_rails_memory_store_cachin subject.execute end + + it 'avoids N+1 queries', :request_store do + current_user + merge_request + + control_count = ActiveRecord::QueryRecorder.new do + subject.execute + end.count + + expect { subject.execute }.not_to exceed_query_limit(control_count) + end end end end -- cgit v1.2.1 From de1db4972cff7539b4ed490e9ebaf87f06500bbd Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 1 Nov 2018 07:50:39 -0700 Subject: Avoidp loading merge request diff files when not needed --- app/services/merge_requests/reload_diffs_service.rb | 2 +- lib/gitlab/diff/file_collection/base.rb | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb index 44997cf012b..b47d8f3f63a 100644 --- a/app/services/merge_requests/reload_diffs_service.rb +++ b/app/services/merge_requests/reload_diffs_service.rb @@ -38,7 +38,7 @@ module MergeRequests # reloading the diff. MergeRequestDiff .where(merge_request: merge_request) - .preload(:merge_request_diff_files, merge_request: :target_project) + .preload(merge_request: :target_project) .find_each do |merge_request_diff| next if merge_request_diff == new_diff diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb index b79ff771a2b..2ad6fe8449d 100644 --- a/lib/gitlab/diff/file_collection/base.rb +++ b/lib/gitlab/diff/file_collection/base.rb @@ -17,7 +17,6 @@ module Gitlab @diffable = diffable @include_stats = diff_options.delete(:include_stats) - @diffs = diffable.raw_diffs(diff_options) @project = project @diff_options = diff_options @diff_refs = diff_refs @@ -25,8 +24,12 @@ module Gitlab @repository = project.repository end + def diffs + @diffs ||= diffable.raw_diffs(diff_options) + end + def diff_files - @diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) } + @diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) } end def diff_file_with_old_path(old_path) -- cgit v1.2.1 From 9338c11f61432cccc939c0912877c1b128b9b668 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 1 Nov 2018 09:51:42 -0700 Subject: Fix merge request specs that expect diff_files to be called --- spec/models/merge_request_diff_spec.rb | 12 ++++++------ spec/models/merge_request_spec.rb | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 90cce826b6c..562ccaf6c0b 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -52,9 +52,9 @@ describe MergeRequestDiff do context 'when it was not cleaned by the system' do it 'returns persisted diffs' do - expect(diff).to receive(:load_diffs) + expect(diff).to receive(:load_diffs).and_call_original - diff.diffs + diff.diffs.diff_files end end @@ -76,19 +76,19 @@ describe MergeRequestDiff do end it 'returns persisted diffs if cannot compare with diff refs' do - expect(diff).to receive(:load_diffs) + expect(diff).to receive(:load_diffs).and_call_original diff.update!(head_commit_sha: 'invalid-sha') - diff.diffs + diff.diffs.diff_files end it 'returns persisted diffs if diff refs does not exist' do - expect(diff).to receive(:load_diffs) + expect(diff).to receive(:load_diffs).and_call_original diff.update!(start_commit_sha: nil, base_commit_sha: nil) - diff.diffs + diff.diffs.diff_files end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c8943f2d86f..37f1456710d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -538,9 +538,9 @@ describe MergeRequest do it 'delegates to the MR diffs' do merge_request.save - expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options)) + expect(merge_request.merge_request_diff).to receive(:raw_diffs).with(hash_including(options)).and_call_original - merge_request.diffs(options) + merge_request.diffs(options).diff_files end end -- cgit v1.2.1