From f2fa7c32992db5a2b1b6acadc6c203c93c139f3b Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Thu, 4 Apr 2019 13:22:11 -0700
Subject: Fix and expand Gitaly FindCommit caching

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26248 added
support for deduplicating FindCommit requests using Gitaly ref name
caching. However, not all endpoints were covered, and in one case the
Gitaly wrapper wasn't actually surrounding the serialization step. We
can safely cache ref names between FindCommit calls for #index and #show
endpoints for merge requests and pipelines. This can significantly
reduce the number of FindCommit requests.
---
 app/controllers/projects/application_controller.rb          |  6 ++++++
 app/controllers/projects/merge_requests_controller.rb       |  6 +++---
 app/controllers/projects/pipelines_controller.rb            | 12 ++++++------
 changelogs/unreleased/sh-fix-ref-name-caching.yml           |  5 +++++
 spec/controllers/projects/merge_requests_controller_spec.rb |  2 ++
 spec/controllers/projects/pipelines_controller_spec.rb      |  2 ++
 6 files changed, 24 insertions(+), 9 deletions(-)
 create mode 100644 changelogs/unreleased/sh-fix-ref-name-caching.yml

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 6504fd6c08a..781eac7f080 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -88,4 +88,10 @@ class Projects::ApplicationController < ApplicationController
   def check_issues_available!
     return render_404 unless @project.feature_available?(:issues, current_user)
   end
+
+  def allow_gitaly_ref_name_caching
+    ::Gitlab::GitalyClient.allow_ref_name_caching do
+      yield
+    end
+  end
 end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 34cb0416965..39ba2a651d4 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -16,6 +16,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
 
+  around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
+
   def index
     @merge_requests = @issuables
 
@@ -315,9 +317,7 @@ 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
+    MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
   end
 
   def define_edit_vars
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index c306ba3ffcf..22c4b8eef1f 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -8,6 +8,8 @@ class Projects::PipelinesController < Projects::ApplicationController
   before_action :authorize_create_pipeline!, only: [:new, :create]
   before_action :authorize_update_pipeline!, only: [:retry, :cancel]
 
+  around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
+
   wrap_parameters Ci::Pipeline
 
   POLLING_INTERVAL = 10_000
@@ -148,12 +150,10 @@ class Projects::PipelinesController < Projects::ApplicationController
   private
 
   def serialize_pipelines
-    ::Gitlab::GitalyClient.allow_ref_name_caching do
-      PipelineSerializer
-        .new(project: @project, current_user: @current_user)
-        .with_pagination(request, response)
-        .represent(@pipelines, disable_coverage: true, preload: true)
-    end
+    PipelineSerializer
+      .new(project: @project, current_user: @current_user)
+      .with_pagination(request, response)
+      .represent(@pipelines, disable_coverage: true, preload: true)
   end
 
   def render_show
diff --git a/changelogs/unreleased/sh-fix-ref-name-caching.yml b/changelogs/unreleased/sh-fix-ref-name-caching.yml
new file mode 100644
index 00000000000..6abd86688b4
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-ref-name-caching.yml
@@ -0,0 +1,5 @@
+---
+title: Fix and expand Gitaly FindCommit caching
+merge_request: 27018
+author:
+type: performance
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c8fa93a74ee..017162519d8 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -60,6 +60,8 @@ describe Projects::MergeRequestsController do
       end
 
       it "renders merge request page" do
+        expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
+
         go(format: :html)
 
         expect(response).to be_success
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index b64ae552efc..814100f7d5d 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -97,6 +97,8 @@ describe Projects::PipelinesController do
         RequestStore.clear!
         RequestStore.begin!
 
+        expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
+
         expect { get_pipelines_index_json }
           .to change { Gitlab::GitalyClient.get_request_count }.by(2)
       end
-- 
cgit v1.2.1