Allow ref name caching CommitService#find_commit

For a given merge request, it's quite common to see duplicate FindCommit
Gitaly requests because the Gitaly CommitService caches the request by
the commit SHA, not by the ref name. However, most of the duplicate
requests use the ref name, so the cache is never actually used in
practice. This leads to unnecessary requests that slow performance.

This commit allows certain callers to bypass the ref name to
OID conversion in the cache. We don't do this by default because it's
possible the tip of the branch changes during the commit, which
would cause the caller to get stale data.

This commit also forces the Ci::Pipeline to use the full ref name
so that caching can work for merge requests.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/57083

app/controllers/projects/merge_requests_controller.rb

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

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,16 +781,18 @@ module Ci
     end
 
     def git_ref
-      if merge_request_event?
-        ##
-        # In the future, we're going to change this ref to
-        # merge request's merged reference, such as "refs/merge-requests/:iid/merge".
-        # In order to do that, we have to update GitLab-Runner's source pulling
-        # logic.
-        # See https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/1092
-        Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
-      else
-        super
+      strong_memoize(:git_pref) do
+        if merge_request_event?
+          ##
+          # In the future, we're going to change this ref to
+          # merge request's merged reference, such as "refs/merge-requests/:iid/merge".
+          # In order to do that, we have to update GitLab-Runner's source pulling
+          # logic.
+          # See https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/1092
+          Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
+        else
+          super
+        end
       end
     end
 

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

@@ -296,6 +296,25 @@ 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?
+
+      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(