Merge branch '44191-reduce-redis-usage-from-merge-request-diffs-caching' into 'master'

Resolve "Reduce Redis usage from merge request diffs caching"

Closes #44191

See merge request gitlab-org/gitlab-ce!17746

Author: stanhu

app/models/merge_request.rb

@@ -579,9 +579,10 @@ def reload_diff(current_user = nil)
     return unless open?
 
     old_diff_refs = self.diff_refs
+    new_diff = create_merge_request_diff
+
+    MergeRequests::MergeRequestDiffCacheService.new.execute(self, new_diff)
 
-    create_merge_request_diff
-    MergeRequests::MergeRequestDiffCacheService.new.execute(self)
     new_diff_refs = self.diff_refs
 
     update_diff_discussion_positions(

app/services/merge_requests/merge_request_diff_cache_service.rb

@@ -1,8 +1,17 @@
 module MergeRequests
   class MergeRequestDiffCacheService
-    def execute(merge_request)
+    def execute(merge_request, new_diff)
       # Executing the iteration we cache all the highlighted diff information
       merge_request.diffs.diff_files.to_a
+
+      # 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|
+        next if merge_request_diff == new_diff
+
+        merge_request_diff.diffs.clear_cache!
+      end
     end
   end
 end

changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml

@@ -0,0 +1,5 @@
+---
+title: Stop caching highlighted diffs in Redis unnecessarily
+merge_request: 17746
+author:
+type: performance

lib/gitlab/diff/file_collection/merge_request_diff.rb

@@ -29,6 +29,14 @@ def real_size
           @merge_request_diff.real_size
         end
 
+        def clear_cache!
+          Rails.cache.delete(cache_key)
+        end
+
+        def cache_key
+          [@merge_request_diff, 'highlighted-diff-files', diff_options]
+        end
+
         private
 
         def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
@@ -64,16 +72,12 @@ def highlight_cache
         end
 
         def store_highlight_cache
-          Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty
+          Rails.cache.write(cache_key, highlight_cache, expires_in: 1.week) if @highlight_cache_was_empty
         end
 
         def cacheable?(diff_file)
           @merge_request_diff.present? && diff_file.text? && diff_file.diffable?
         end
-
-        def cache_key
-          [@merge_request_diff, 'highlighted-diff-files', diff_options]
-        end
       end
     end
   end

spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

@@ -12,7 +12,7 @@
     diff_files
   end
 
-  it 'does not files marked as undiffable in .gitattributes' do
+  it 'does not highlight files marked as undiffable in .gitattributes' do
     allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
 
     expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)

spec/models/merge_request_spec.rb

@@ -1544,7 +1544,7 @@ def set_compare(merge_request)
     end
 
     it "executes diff cache service" do
-      expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
+      expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject, an_instance_of(MergeRequestDiff))
 
       subject.reload_diff
     end

spec/services/merge_requests/merge_request_diff_cache_service_spec.rb

@@ -1,19 +1,39 @@
 require 'spec_helper'
 
-describe MergeRequests::MergeRequestDiffCacheService do
+describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_store_caching do
   let(:subject) { described_class.new }
+  let(:merge_request) { create(:merge_request) }
 
   describe '#execute' do
-    it 'retrieves the diff files to cache the highlighted result' do
-      merge_request = create(:merge_request)
-      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection::MergeRequestDiff.default_options]
-
-      expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
-      expect(Rails.cache).to receive(:write).with(cache_key, anything)
+    before do
       allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
       allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
+    end
+
+    it 'retrieves the diff files to cache the highlighted result' do
+      new_diff = merge_request.merge_request_diff
+      cache_key = new_diff.diffs.cache_key
+
+      expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
+      expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
+
+      subject.execute(merge_request, new_diff)
+    end
+
+    it 'clears the cache for older diffs on the merge request' do
+      old_diff = merge_request.merge_request_diff
+      old_cache_key = old_diff.diffs.cache_key
+
+      subject.execute(merge_request, old_diff)
+
+      new_diff = merge_request.create_merge_request_diff
+      new_cache_key = new_diff.diffs.cache_key
+
+      expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
+      expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
+      expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
 
-      subject.execute(merge_request)
+      subject.execute(merge_request, new_diff)
     end
   end
 end