Merge branch '30536-branches-list-api-performance-degrades-notably-under-load' into 'master'

Improvement for branches list API performance

See merge request gitlab-org/gitlab!23458

app/models/repository.rb

@@ -41,8 +41,8 @@ class Repository
   CACHED_METHODS = %i(size commit_count rendered_readme readme_path contribution_guide
                       changelog license_blob license_key gitignore
                       gitlab_ci_yml branch_names tag_names branch_count
-                      tag_count avatar exists? root_ref has_visible_content?
-                      issue_template_names merge_request_template_names
+                      tag_count avatar exists? root_ref merged_branch_names
+                      has_visible_content? issue_template_names merge_request_template_names
                       metrics_dashboard_paths xcode_project?).freeze
 
   # Methods that use cache_method but only memoize the value
@@ -65,6 +65,8 @@ class Repository
     xcode_config: :xcode_project?
   }.freeze
 
+  MERGED_BRANCH_NAMES_CACHE_DURATION = 10.minutes
+
   def initialize(full_path, project, disk_path: nil, repo_type: Gitlab::GlRepository::PROJECT)
     @full_path = full_path
     @disk_path = disk_path || full_path
@@ -296,7 +298,7 @@ class Repository
   end
 
   def expire_branches_cache
-    expire_method_caches(%i(branch_names branch_count has_visible_content?))
+    expire_method_caches(%i(branch_names merged_branch_names branch_count has_visible_content?))
     @local_branches = nil
     @branch_exists_memo = nil
   end
@@ -916,7 +918,39 @@ class Repository
     @root_ref_sha ||= commit(root_ref).sha
   end
 
-  delegate :merged_branch_names, to: :raw_repository
+  def merged_branch_names(branch_names = [])
+    # Currently we should skip caching if requesting all branch names
+    # This is only used in a few places, notably app/services/branches/delete_merged_service.rb,
+    # and it could potentially result in a very large cache/performance issues with the current
+    # implementation.
+    skip_cache = branch_names.empty? || Feature.disabled?(:merged_branch_names_redis_caching)
+    return raw_repository.merged_branch_names(branch_names) if skip_cache
+
+    cached_branch_names = cache.read(:merged_branch_names)
+    merged_branch_names_hash = cached_branch_names || {}
+    missing_branch_names = branch_names.select { |bn| !merged_branch_names_hash.key?(bn) }
+
+    # Track some metrics here whilst feature flag is enabled
+    if cached_branch_names.present?
+      counter = Gitlab::Metrics.counter(
+        :gitlab_repository_merged_branch_names_cache_hit,
+        "Count of cache hits for Repository#merged_branch_names"
+      )
+      counter.increment(full_hit: missing_branch_names.empty?)
+    end
+
+    if missing_branch_names.any?
+      merged = raw_repository.merged_branch_names(missing_branch_names)
+
+      missing_branch_names.each do |bn|
+        merged_branch_names_hash[bn] = merged.include?(bn)
+      end
+
+      cache.write(:merged_branch_names, merged_branch_names_hash, expires_in: MERGED_BRANCH_NAMES_CACHE_DURATION)
+    end
+
+    Set.new(merged_branch_names_hash.select { |_, v| v }.keys)
+  end
 
   def merge_base(*commits_or_ids)
     commit_ids = commits_or_ids.map do |commit_or_id|

lib/gitlab/repository_cache.rb

@@ -33,8 +33,8 @@ module Gitlab
       backend.read(cache_key(key))
     end
 
-    def write(key, value)
-      backend.write(cache_key(key), value)
+    def write(key, value, *args)
+      backend.write(cache_key(key), value, *args)
     end
 
     def fetch_without_caching_false(key, &block)

spec/lib/gitlab/repository_cache_spec.rb

@@ -50,6 +50,18 @@ describe Gitlab::RepositoryCache do
     end
   end
 
+  describe '#write' do
+    it 'writes the given key and value to the cache' do
+      cache.write(:test, 'test')
+      expect(backend).to have_received(:write).with("test:#{namespace}", 'test')
+    end
+
+    it 'passes additional options to the backend' do
+      cache.write(:test, 'test', expires_in: 10.minutes)
+      expect(backend).to have_received(:write).with("test:#{namespace}", 'test', expires_in: 10.minutes)
+    end
+  end
+
   describe '#fetch_without_caching_false', :use_clean_rails_memory_store_caching do
     let(:key) { :foo }
     let(:backend) { Rails.cache }

spec/models/repository_spec.rb

@@ -494,6 +494,100 @@ describe Repository do
     it { is_expected.to eq(commit.sha) }
   end
 
+  describe "#merged_branch_names", :clean_gitlab_redis_cache do
+    subject { repository.merged_branch_names(branch_names) }
+
+    let(:branch_names) { %w(test beep boop definitely_merged) }
+    let(:already_merged) { Set.new(["definitely_merged"]) }
+
+    let(:merge_state_hash) do
+      {
+        "test" => false,
+        "beep" => false,
+        "boop" => false,
+        "definitely_merged" => true
+      }
+    end
+
+    let_it_be(:cache) do
+      caching_config_hash = Gitlab::Redis::Cache.params
+      ActiveSupport::Cache.lookup_store(:redis_cache_store, caching_config_hash)
+    end
+
+    let(:repository_cache) do
+      Gitlab::RepositoryCache.new(repository, backend: Rails.cache)
+    end
+
+    let(:cache_key) { repository_cache.cache_key(:merged_branch_names) }
+
+    before do
+      allow(Rails).to receive(:cache) { cache }
+      allow(repository).to receive(:cache) { repository_cache }
+      allow(repository.raw_repository).to receive(:merged_branch_names).with(branch_names).and_return(already_merged)
+    end
+
+    it { is_expected.to eq(already_merged) }
+    it { is_expected.to be_a(Set) }
+
+    context "cache is empty" do
+      before do
+        cache.delete(cache_key)
+      end
+
+      it { is_expected.to eq(already_merged) }
+
+      describe "cache values" do
+        it "writes the values to redis" do
+          expect(cache).to receive(:write).with(cache_key, merge_state_hash, expires_in: Repository::MERGED_BRANCH_NAMES_CACHE_DURATION)
+
+          subject
+        end
+
+        it "matches the supplied hash" do
+          subject
+
+          expect(cache.read(cache_key)).to eq(merge_state_hash)
+        end
+      end
+    end
+
+    context "cache is not empty" do
+      before do
+        cache.write(cache_key, merge_state_hash)
+      end
+
+      it { is_expected.to eq(already_merged) }
+
+      it "doesn't fetch from the disk" do
+        expect(repository.raw_repository).not_to receive(:merged_branch_names)
+
+        subject
+      end
+    end
+
+    context "cache is partially complete" do
+      before do
+        allow(repository.raw_repository).to receive(:merged_branch_names).with(["boop"]).and_return([])
+        hash = merge_state_hash.except("boop")
+        cache.write(cache_key, hash)
+      end
+
+      it { is_expected.to eq(already_merged) }
+
+      it "does fetch from the disk" do
+        expect(repository.raw_repository).to receive(:merged_branch_names).with(["boop"])
+
+        subject
+      end
+    end
+
+    context "requested branches array is empty" do
+      let(:branch_names) { [] }
+
+      it { is_expected.to eq(already_merged) }
+    end
+  end
+
   describe '#can_be_merged?' do
     context 'mergeable branches' do
       subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') }
@@ -1784,6 +1878,7 @@ describe Repository do
         :avatar,
         :exists?,
         :root_ref,
+        :merged_branch_names,
         :has_visible_content?,
         :issue_template_names,
         :merge_request_template_names,
@@ -1959,7 +2054,7 @@ describe Repository do
   describe '#expire_branches_cache' do
     it 'expires the cache' do
       expect(repository).to receive(:expire_method_caches)
-        .with(%i(branch_names branch_count has_visible_content?))
+        .with(%i(branch_names merged_branch_names branch_count has_visible_content?))
         .and_call_original
 
       repository.expire_branches_cache

spec/requests/api/branches_spec.rb

@@ -608,7 +608,7 @@ describe API::Branches do
       expect(json_response['message']).to eq('Branch name is invalid')
     end
 
-    it 'returns 400 if branch already exists' do
+    it 'returns 400 if branch already exists', :clean_gitlab_redis_cache do
       post api(route, user), params: { branch: 'new_design1', ref: branch_sha }
 
       expect(response).to have_gitlab_http_status(201)