diff --git a/app/models/repository.rb b/app/models/repository.rb index b957b9b0bdd685fa476dbe010479e9daa0fdfbba..6f63cd32da49f6e914ca96160cd98882e8a82b0b 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -239,13 +239,13 @@ class Repository def branch_exists?(branch_name) return false unless raw_repository - branch_names_include?(branch_name) + branch_names.include?(branch_name) end def tag_exists?(tag_name) return false unless raw_repository - tag_names_include?(tag_name) + tag_names.include?(tag_name) end def ref_exists?(ref) @@ -565,10 +565,10 @@ class Repository end delegate :branch_names, to: :raw_repository - cache_method_as_redis_set :branch_names, fallback: [] + cache_method :branch_names, fallback: [] delegate :tag_names, to: :raw_repository - cache_method_as_redis_set :tag_names, fallback: [] + cache_method :tag_names, fallback: [] delegate :branch_count, :tag_count, :has_visible_content?, to: :raw_repository cache_method :branch_count, fallback: 0 @@ -1130,10 +1130,6 @@ class Repository @cache ||= Gitlab::RepositoryCache.new(self) end - def redis_set_cache - @redis_set_cache ||= Gitlab::RepositorySetCache.new(self) - end - def request_store_cache @request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore) end diff --git a/changelogs/unreleased/64251-branch-name-set-cache.yml b/changelogs/unreleased/64251-branch-name-set-cache.yml deleted file mode 100644 index 6ce4bdf5e43dc7741ec4bc78928e8a3de0a06de2..0000000000000000000000000000000000000000 --- a/changelogs/unreleased/64251-branch-name-set-cache.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Cache branch and tag names as Redis sets -merge_request: 30476 -author: -type: performance diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index 75503ee1789adca051264cdf02a0e696c5ae58b7..e40c366ed02dc9d9273339fd8ef899c65588d08f 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -23,37 +23,6 @@ module Gitlab end end - # Caches and strongly memoizes the method as a Redis Set. - # - # This only works for methods that do not take any arguments. The method - # should return an Array of Strings to be cached. - # - # In addition to overriding the named method, a "name_include?" method is - # defined. This uses the "SISMEMBER" query to efficiently check membership - # without needing to load the entire set into memory. - # - # name - The name of the method to be cached. - # fallback - A value to fall back to if the repository does not exist, or - # in case of a Git error. Defaults to nil. - def cache_method_as_redis_set(name, fallback: nil) - uncached_name = alias_uncached_method(name) - - define_method(name) do - cache_method_output_as_redis_set(name, fallback: fallback) do - __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend - end - end - - define_method("#{name}_include?") do |value| - # If the cache isn't populated, we can't rely on it - return redis_set_cache.include?(name, value) if redis_set_cache.exist?(name) - - # Since we have to pull all branch names to populate the cache, use - # the data we already have to answer the query just this once - __send__(name).include?(value) # rubocop:disable GitlabSecurity/PublicSend - end - end - # Caches truthy values from the method. All values are strongly memoized, # and cached in RequestStore. # @@ -115,11 +84,6 @@ module Gitlab raise NotImplementedError end - # RepositorySetCache to be used. Should be overridden by the including class - def redis_set_cache - raise NotImplementedError - end - # List of cached methods. Should be overridden by the including class def cached_methods raise NotImplementedError @@ -136,18 +100,6 @@ module Gitlab end end - # Caches and strongly memoizes the supplied block as a Redis Set. The result - # will be provided as a sorted array. - # - # name - The name of the method to be cached. - # fallback - A value to fall back to if the repository does not exist, or - # in case of a Git error. Defaults to nil. - def cache_method_output_as_redis_set(name, fallback: nil, &block) - memoize_method_output(name, fallback: fallback) do - redis_set_cache.fetch(name, &block).sort - end - end - # Caches truthy values from the supplied block. All values are strongly # memoized, and cached in RequestStore. # @@ -202,7 +154,6 @@ module Gitlab clear_memoization(memoizable_name(name)) end - expire_redis_set_method_caches(methods) expire_request_store_method_caches(methods) end @@ -218,10 +169,6 @@ module Gitlab end end - def expire_redis_set_method_caches(methods) - methods.each { |name| redis_set_cache.expire(name) } - end - # All cached repository methods depend on the existence of a Git repository, # so if the repository doesn't exist, we already know not to call it. def fallback_early?(method_name) diff --git a/lib/gitlab/repository_set_cache.rb b/lib/gitlab/repository_set_cache.rb deleted file mode 100644 index fb634328a9558924bd0677d41c60c48f740facf7..0000000000000000000000000000000000000000 --- a/lib/gitlab/repository_set_cache.rb +++ /dev/null @@ -1,67 +0,0 @@ -# frozen_string_literal: true - -# Interface to the Redis-backed cache store for keys that use a Redis set -module Gitlab - class RepositorySetCache - attr_reader :repository, :namespace, :expires_in - - def initialize(repository, extra_namespace: nil, expires_in: 2.weeks) - @repository = repository - @namespace = "#{repository.full_path}:#{repository.project.id}" - @namespace = "#{@namespace}:#{extra_namespace}" if extra_namespace - @expires_in = expires_in - end - - def cache_key(type) - [type, namespace, 'set'].join(':') - end - - def expire(key) - with { |redis| redis.del(cache_key(key)) } - end - - def exist?(key) - with { |redis| redis.exists(cache_key(key)) } - end - - def read(key) - with { |redis| redis.smembers(cache_key(key)) } - end - - def write(key, value) - full_key = cache_key(key) - - with do |redis| - redis.multi do - redis.del(full_key) - - # Splitting into groups of 1000 prevents us from creating a too-long - # Redis command - value.in_groups_of(1000, false) { |subset| redis.sadd(full_key, subset) } - - redis.expire(full_key, expires_in) - end - end - - value - end - - def fetch(key, &block) - if exist?(key) - read(key) - else - write(key, yield) - end - end - - def include?(key, value) - with { |redis| redis.sismember(cache_key(key), value) } - end - - private - - def with(&blk) - Gitlab::Redis::Cache.with(&blk) # rubocop:disable CodeReuse/ActiveRecord - end - end -end diff --git a/spec/lib/gitlab/repository_cache_adapter_spec.rb b/spec/lib/gitlab/repository_cache_adapter_spec.rb index fd1338b55a6a2abbb2bea6846dea403fe3a694bf..808eb865a214a423b38b75b68ad62f00c67571ef 100644 --- a/spec/lib/gitlab/repository_cache_adapter_spec.rb +++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb @@ -6,7 +6,6 @@ describe Gitlab::RepositoryCacheAdapter do let(:project) { create(:project, :repository) } let(:repository) { project.repository } let(:cache) { repository.send(:cache) } - let(:redis_set_cache) { repository.send(:redis_set_cache) } describe '#cache_method_output', :use_clean_rails_memory_store_caching do let(:fallback) { 10 } @@ -209,11 +208,9 @@ describe Gitlab::RepositoryCacheAdapter do describe '#expire_method_caches' do it 'expires the caches of the given methods' do expect(cache).to receive(:expire).with(:rendered_readme) - expect(cache).to receive(:expire).with(:branch_names) - expect(redis_set_cache).to receive(:expire).with(:rendered_readme) - expect(redis_set_cache).to receive(:expire).with(:branch_names) + expect(cache).to receive(:expire).with(:gitignore) - repository.expire_method_caches(%i(rendered_readme branch_names)) + repository.expire_method_caches(%i(rendered_readme gitignore)) end it 'does not expire caches for non-existent methods' do diff --git a/spec/lib/gitlab/repository_set_cache_spec.rb b/spec/lib/gitlab/repository_set_cache_spec.rb deleted file mode 100644 index 87e51f801e5d42d7508d7340b4cb568b542ddddc..0000000000000000000000000000000000000000 --- a/spec/lib/gitlab/repository_set_cache_spec.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::RepositorySetCache, :clean_gitlab_redis_cache do - let(:project) { create(:project) } - let(:repository) { project.repository } - let(:namespace) { "#{repository.full_path}:#{project.id}" } - let(:cache) { described_class.new(repository) } - - describe '#cache_key' do - subject { cache.cache_key(:foo) } - - it 'includes the namespace' do - is_expected.to eq("foo:#{namespace}:set") - end - - context 'with a given namespace' do - let(:extra_namespace) { 'my:data' } - let(:cache) { described_class.new(repository, extra_namespace: extra_namespace) } - - it 'includes the full namespace' do - is_expected.to eq("foo:#{namespace}:#{extra_namespace}:set") - end - end - end - - describe '#expire' do - it 'expires the given key from the cache' do - cache.write(:foo, ['value']) - - expect(cache.read(:foo)).to contain_exactly('value') - expect(cache.expire(:foo)).to eq(1) - expect(cache.read(:foo)).to be_empty - end - end - - describe '#exist?' do - it 'checks whether the key exists' do - expect(cache.exist?(:foo)).to be(false) - - cache.write(:foo, ['value']) - - expect(cache.exist?(:foo)).to be(true) - end - end - - describe '#fetch' do - let(:blk) { -> { ['block value'] } } - - subject { cache.fetch(:foo, &blk) } - - it 'fetches the key from the cache when filled' do - cache.write(:foo, ['value']) - - is_expected.to contain_exactly('value') - end - - it 'writes the value of the provided block when empty' do - cache.expire(:foo) - - is_expected.to contain_exactly('block value') - expect(cache.read(:foo)).to contain_exactly('block value') - end - end - - describe '#include?' do - it 'checks inclusion in the Redis set' do - cache.write(:foo, ['value']) - - expect(cache.include?(:foo, 'value')).to be(true) - expect(cache.include?(:foo, 'bar')).to be(false) - end - end -end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 79395fcc994b72c826a025ad3b0989a704093f06..419e1dc2459a147b092e370b8024cb7b8afd7dac 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1223,66 +1223,36 @@ describe Repository do end describe '#branch_exists?' do - let(:branch) { repository.root_ref } + it 'uses branch_names' do + allow(repository).to receive(:branch_names).and_return(['foobar']) - subject { repository.branch_exists?(branch) } - - it 'delegates to branch_names when the cache is empty' do - repository.expire_branches_cache - - expect(repository).to receive(:branch_names).and_call_original - is_expected.to eq(true) - end - - it 'uses redis set caching when the cache is filled' do - repository.branch_names # ensure the branch name cache is filled - - expect(repository) - .to receive(:branch_names_include?) - .with(branch) - .and_call_original - - is_expected.to eq(true) + expect(repository.branch_exists?('foobar')).to eq(true) + expect(repository.branch_exists?('master')).to eq(false) end end describe '#tag_exists?' do - let(:tag) { repository.tags.first.name } - - subject { repository.tag_exists?(tag) } - - it 'delegates to tag_names when the cache is empty' do - repository.expire_tags_cache - - expect(repository).to receive(:tag_names).and_call_original - is_expected.to eq(true) - end - - it 'uses redis set caching when the cache is filled' do - repository.tag_names # ensure the tag name cache is filled - - expect(repository) - .to receive(:tag_names_include?) - .with(tag) - .and_call_original + it 'uses tag_names' do + allow(repository).to receive(:tag_names).and_return(['foobar']) - is_expected.to eq(true) + expect(repository.tag_exists?('foobar')).to eq(true) + expect(repository.tag_exists?('master')).to eq(false) end end - describe '#branch_names', :clean_gitlab_redis_cache do + describe '#branch_names', :use_clean_rails_memory_store_caching do let(:fake_branch_names) { ['foobar'] } it 'gets cached across Repository instances' do allow(repository.raw_repository).to receive(:branch_names).once.and_return(fake_branch_names) - expect(repository.branch_names).to match_array(fake_branch_names) + expect(repository.branch_names).to eq(fake_branch_names) fresh_repository = Project.find(project.id).repository expect(fresh_repository.object_id).not_to eq(repository.object_id) expect(fresh_repository.raw_repository).not_to receive(:branch_names) - expect(fresh_repository.branch_names).to match_array(fake_branch_names) + expect(fresh_repository.branch_names).to eq(fake_branch_names) end end