Merge branch 'osw-fix-excessive-redis-calls-on-new-diffs-cache' into 'master'

Fix excessive Redis calls for new HSET/HMGET diffs cache

Closes #39133

See merge request gitlab-org/gitlab!21449

lib/gitlab/diff/highlight_cache.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module Diff
     class HighlightCache
+      include Gitlab::Utils::StrongMemoize
+
       EXPIRATION = 1.week
       VERSION = 1
 
@@ -30,12 +32,11 @@ module Gitlab
       #   IO generated by N+1's (1 writing for each highlighted line or file).
       #
       def write_if_empty
-        return if uncached_files.empty?
+        return if cacheable_files.empty?
 
         new_cache_content = {}
-        uncached_files.each do |diff_file|
-          next unless cacheable?(diff_file)
 
+        cacheable_files.each do |diff_file|
           new_cache_content[diff_file.file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
@@ -49,7 +50,9 @@ module Gitlab
       end
 
       def key
-        @redis_key ||= ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":")
+        strong_memoize(:redis_key) do
+          ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":")
+        end
       end
 
       private
@@ -60,13 +63,17 @@ module Gitlab
       # See https://gitlab.com/gitlab-org/gitlab/issues/38008
       #
       def deprecated_cache
-        @deprecated_cache ||= Gitlab::Diff::DeprecatedHighlightCache.new(@diff_collection)
+        strong_memoize(:deprecated_cache) do
+          Gitlab::Diff::DeprecatedHighlightCache.new(@diff_collection)
+        end
       end
 
-      def uncached_files
-        diff_files = @diff_collection.diff_files
+      def cacheable_files
+        strong_memoize(:cacheable_files) do
+          diff_files = @diff_collection.diff_files
 
-        diff_files.select { |file| read_cache[file.file_path].nil? }
+          diff_files.select { |file| cacheable?(file) && read_file(file).nil? }
+        end
       end
 
       # Given a hash of:
@@ -95,13 +102,20 @@ module Gitlab
           end
         end
 
+        # Subsequent read_file calls would need the latest cache.
+        #
+        clear_memoization(:cached_content)
+        clear_memoization(:cacheable_files)
+
         # Clean up any deprecated hash entries
         #
         deprecated_cache.clear
       end
 
       def file_paths
-        @file_paths ||= @diff_collection.diffs.collect(&:file_path)
+        strong_memoize(:file_paths) do
+          @diff_collection.diffs.collect(&:file_path)
+        end
       end
 
       def read_file(diff_file)
@@ -109,7 +123,7 @@ module Gitlab
       end
 
       def cached_content
-        @cached_content ||= read_cache
+        strong_memoize(:cached_content) { read_cache }
       end
 
       def read_cache

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -72,13 +72,22 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
   describe '#write_if_empty' do
     it 'filters the key/value list of entries to be caches for each invocation' do
+      paths = merge_request.diffs.diff_files.select(&:text?).map(&:file_path)
+
       expect(cache).to receive(:write_to_redis_hash)
-        .once.with(hash_including(".gitignore")).and_call_original
-      expect(cache).to receive(:write_to_redis_hash).once.with({}).and_call_original
+        .with(hash_including(*paths))
+        .once
+        .and_call_original
 
       2.times { cache.write_if_empty }
     end
 
+    it 'reads from cache once' do
+      expect(cache).to receive(:read_cache).once.and_call_original
+
+      cache.write_if_empty
+    end
+
     context 'different diff_collections for the same diffable' do
       before do
         cache.write_if_empty