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

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

changelogs/unreleased/sh-optimize-reload-diffs-service.yml

+---
+title: Significantly cut memory usage and SQL queries when reloading diffs
+merge_request: 22725
+author:
+type: performance

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)

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

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
 

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