Merge branch 'memoize-redis-set-includes' into 'master'

Memoize $name_includes? methods for set caches

See merge request gitlab-org/gitlab!39710

app/models/repository.rb

@@ -314,12 +314,14 @@ class Repository
   def expire_tags_cache
     expire_method_caches(%i(tag_names tag_count has_ambiguous_refs?))
     @tags = nil
+    @tag_names_include = nil
   end
 
   def expire_branches_cache
     expire_method_caches(%i(branch_names merged_branch_names branch_count has_visible_content? has_ambiguous_refs?))
     @local_branches = nil
     @branch_exists_memo = nil
+    @branch_names_include = nil
   end
 
   def expire_statistics_caches

lib/gitlab/repository_cache_adapter.rb

@@ -58,11 +58,19 @@ module Gitlab
         # wrong answer. We handle that by querying the full list - which fills
         # the cache - and using it directly to answer the question.
         define_method("#{name}_include?") do |value|
-          if strong_memoized?(name) || !redis_set_cache.exist?(name)
-            return __send__(name).include?(value) # rubocop:disable GitlabSecurity/PublicSend
-          end
+          ivar = "@#{name}_include"
+          memoized = instance_variable_get(ivar) || {}
+
+          next memoized[value] if memoized.key?(value)
+
+          memoized[value] =
+            if strong_memoized?(name) || !redis_set_cache.exist?(name)
+              __send__(name).include?(value) # rubocop:disable GitlabSecurity/PublicSend
+            else
+              redis_set_cache.include?(name, value)
+            end
 
-          redis_set_cache.include?(name, value)
+          instance_variable_set(ivar, memoized)[value]
         end
       end
 

spec/lib/gitlab/repository_cache_adapter_spec.rb

@@ -9,6 +9,89 @@ RSpec.describe Gitlab::RepositoryCacheAdapter do
   let(:redis_set_cache) { repository.send(:redis_set_cache) }
   let(:redis_hash_cache) { repository.send(:redis_hash_cache) }
 
+  describe '.cache_method_output_as_redis_set', :clean_gitlab_redis_cache, :aggregate_failures do
+    let(:klass) do
+      Class.new do
+        include Gitlab::RepositoryCacheAdapter # can't use described_class here
+
+        def letters
+          %w(b a c)
+        end
+        cache_method_as_redis_set(:letters)
+
+        def redis_set_cache
+          @redis_set_cache ||= Gitlab::RepositorySetCache.new(self)
+        end
+
+        def full_path
+          'foo/bar'
+        end
+
+        def project
+        end
+      end
+    end
+
+    let(:fake_repository) { klass.new }
+
+    context 'with an existing repository' do
+      it 'caches the output, sorting the results' do
+        expect(fake_repository).to receive(:_uncached_letters).once.and_call_original
+
+        2.times do
+          expect(fake_repository.letters).to eq(%w(a b c))
+        end
+
+        expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(true)
+        expect(fake_repository.instance_variable_get(:@letters)).to eq(%w(a b c))
+      end
+
+      context 'membership checks' do
+        context 'when the cache key does not exist' do
+          it 'calls the original method and populates the cache' do
+            expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(false)
+            expect(fake_repository).to receive(:_uncached_letters).once.and_call_original
+
+            # This populates the cache and memoizes the full result
+            expect(fake_repository.letters_include?('a')).to eq(true)
+            expect(fake_repository.letters_include?('d')).to eq(false)
+            expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(true)
+          end
+        end
+
+        context 'when the cache key exists' do
+          before do
+            fake_repository.redis_set_cache.write(:letters, %w(b a c))
+          end
+
+          it 'calls #include? on the set cache' do
+            expect(fake_repository.redis_set_cache)
+              .to receive(:include?).with(:letters, 'a').and_call_original
+            expect(fake_repository.redis_set_cache)
+              .to receive(:include?).with(:letters, 'd').and_call_original
+
+            expect(fake_repository.letters_include?('a')).to eq(true)
+            expect(fake_repository.letters_include?('d')).to eq(false)
+          end
+
+          it 'memoizes the result' do
+            expect(fake_repository.redis_set_cache)
+              .to receive(:include?).once.and_call_original
+
+            expect(fake_repository.letters_include?('a')).to eq(true)
+            expect(fake_repository.letters_include?('a')).to eq(true)
+
+            expect(fake_repository.redis_set_cache)
+              .to receive(:include?).once.and_call_original
+
+            expect(fake_repository.letters_include?('d')).to eq(false)
+            expect(fake_repository.letters_include?('d')).to eq(false)
+          end
+        end
+      end
+    end
+  end
+
   describe '#cache_method_output', :use_clean_rails_memory_store_caching do
     let(:fallback) { 10 }