summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2018-11-02 10:09:50 +0000
committerSean McGivern <sean@mcgivern.me.uk>2018-11-02 10:09:50 +0000
commitc0d30fad978de58da6d78d8fddcfccf602ebfe82 (patch)
treea74fa73b1f14940935716988ca78e80e6bf5d010
parent816700949028c556457d03fe134810ada2913eb7 (diff)
parent9338c11f61432cccc939c0912877c1b128b9b668 (diff)
downloadgitlab-ce-c0d30fad978de58da6d78d8fddcfccf602ebfe82.tar.gz
Merge branch 'sh-optimize-reload-diffs-service' into 'master'
Significantly cut memory and SQL queries when reloading diffs See merge request gitlab-org/gitlab-ce!22725
-rw-r--r--app/services/merge_requests/reload_diffs_service.rb5
-rw-r--r--changelogs/unreleased/sh-optimize-reload-diffs-service.yml5
-rw-r--r--lib/gitlab/diff/file_collection/base.rb7
-rw-r--r--spec/models/merge_request_diff_spec.rb12
-rw-r--r--spec/models/merge_request_spec.rb4
-rw-r--r--spec/services/merge_requests/reload_diffs_service_spec.rb11
6 files changed, 33 insertions, 11 deletions
diff --git a/app/services/merge_requests/reload_diffs_service.rb b/app/services/merge_requests/reload_diffs_service.rb
index b4d48fe92ad..b47d8f3f63a 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: :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/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)
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 85a4ebac66c..7d500f9e579 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -552,9 +552,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
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