Merge branch 'sh-fix-gitaly-find-commit-caching' into 'master'

Allow ref name caching CommitService#find_commit

Closes #57083

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

app/controllers/projects/merge_requests_controller.rb

@@ -315,8 +315,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
 
   def serializer
+    ::Gitlab::GitalyClient.allow_ref_name_caching do
       MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
     end
+  end
 
   def define_edit_vars
     @source_project = @merge_request.source_project

app/models/ci/pipeline.rb

@@ -465,9 +465,9 @@ module Ci
     end
 
     def latest?
-      return false unless ref && commit.present?
+      return false unless git_ref && commit.present?
 
-      project.commit(ref) == commit
+      project.commit(git_ref) == commit
     end
 
     def retried
@@ -781,6 +781,7 @@ module Ci
     end
 
     def git_ref
+      strong_memoize(:git_ref) do
         if merge_request_event?
           ##
           # In the future, we're going to change this ref to
@@ -793,6 +794,7 @@ module Ci
           super
         end
       end
+    end
 
     def latest_builds_status
       return 'failed' unless yaml_errors.blank?

changelogs/unreleased/sh-fix-gitaly-find-commit-caching.yml

+---
+title: Allow ref name caching CommitService#find_commit
+merge_request: 26248
+author:
+type: performance

lib/gitlab/gitaly_client.rb

@@ -302,6 +302,26 @@ module Gitlab
       end
     end
 
+    # Normally a FindCommit RPC will cache the commit with its SHA
+    # instead of a ref name, since it's possible the branch is mutated
+    # afterwards. However, for read-only requests that never mutate the
+    # branch, this method allows caching of the ref name directly.
+    def self.allow_ref_name_caching
+      return yield unless Gitlab::SafeRequestStore.active?
+      return yield if ref_name_caching_allowed?
+
+      begin
+        Gitlab::SafeRequestStore[:allow_ref_name_caching] = true
+        yield
+      ensure
+        Gitlab::SafeRequestStore[:allow_ref_name_caching] = false
+      end
+    end
+
+    def self.ref_name_caching_allowed?
+      Gitlab::SafeRequestStore[:allow_ref_name_caching]
+    end
+
     def self.get_call_count(key)
       Gitlab::SafeRequestStore[key] || 0
     end

lib/gitlab/gitaly_client/commit_service.rb

@@ -286,7 +286,7 @@ module Gitlab
           commit = call_find_commit(revision)
           return unless commit
 
-          key[:commit_id] = commit.id
+          key[:commit_id] = commit.id unless GitalyClient.ref_name_caching_allowed?
           Gitlab::SafeRequestStore[key] = commit
         else
           call_find_commit(revision)

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -86,6 +86,10 @@ describe Projects::MergeRequestsController do
     end
 
     describe 'as json' do
+      before do
+        expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
+      end
+
       context 'with basic serializer param' do
         it 'renders basic MR entity as json' do
           go(serializer: 'basic', format: :json)

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -221,6 +221,21 @@ describe Gitlab::GitalyClient::CommitService do
           expect(commit).to eq(commit_dbl)
         end
       end
+
+      context 'when caching of the ref name is enabled' do
+        it 'returns a cached commit' do
+          expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commit).once.and_return(double(commit: commit_dbl))
+
+          commit = nil
+          2.times do
+            ::Gitlab::GitalyClient.allow_ref_name_caching do
+              commit = described_class.new(repository).find_commit('master')
+            end
+          end
+
+          expect(commit).to eq(commit_dbl)
+        end
+      end
     end
   end
 

spec/models/ci/pipeline_spec.rb

@@ -1460,6 +1460,14 @@ describe Ci::Pipeline, :mailer do
         end
       end
 
+      context 'with a branch name as the ref' do
+        it 'looks up commit with the full ref name' do
+          expect(pipeline.project).to receive(:commit).with('refs/heads/master').and_call_original
+
+          expect(pipeline).to be_latest
+        end
+      end
+
       context 'with not latest sha' do
         before do
           pipeline.update(