Commit f2fa7c32 authored by Stan Hu's avatar Stan Hu

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.
parent e2425149
...@@ -88,4 +88,10 @@ class Projects::ApplicationController < ApplicationController ...@@ -88,4 +88,10 @@ class Projects::ApplicationController < ApplicationController
def check_issues_available! def check_issues_available!
return render_404 unless @project.feature_available?(:issues, current_user) return render_404 unless @project.feature_available?(:issues, current_user)
end end
def allow_gitaly_ref_name_caching
::Gitlab::GitalyClient.allow_ref_name_caching do
yield
end
end
end end
...@@ -16,6 +16,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -16,6 +16,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
before_action :authenticate_user!, only: [:assign_related_issues] before_action :authenticate_user!, only: [:assign_related_issues]
before_action :check_user_can_push_to_source_branch!, only: [:rebase] before_action :check_user_can_push_to_source_branch!, only: [:rebase]
around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
def index def index
@merge_requests = @issuables @merge_requests = @issuables
...@@ -315,10 +317,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -315,10 +317,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end end
def serializer def serializer
::Gitlab::GitalyClient.allow_ref_name_caching do
MergeRequestSerializer.new(current_user: current_user, project: merge_request.project) MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
end end
end
def define_edit_vars def define_edit_vars
@source_project = @merge_request.source_project @source_project = @merge_request.source_project
......
...@@ -8,6 +8,8 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -8,6 +8,8 @@ class Projects::PipelinesController < Projects::ApplicationController
before_action :authorize_create_pipeline!, only: [:new, :create] before_action :authorize_create_pipeline!, only: [:new, :create]
before_action :authorize_update_pipeline!, only: [:retry, :cancel] before_action :authorize_update_pipeline!, only: [:retry, :cancel]
around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
wrap_parameters Ci::Pipeline wrap_parameters Ci::Pipeline
POLLING_INTERVAL = 10_000 POLLING_INTERVAL = 10_000
...@@ -148,13 +150,11 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -148,13 +150,11 @@ class Projects::PipelinesController < Projects::ApplicationController
private private
def serialize_pipelines def serialize_pipelines
::Gitlab::GitalyClient.allow_ref_name_caching do
PipelineSerializer PipelineSerializer
.new(project: @project, current_user: @current_user) .new(project: @project, current_user: @current_user)
.with_pagination(request, response) .with_pagination(request, response)
.represent(@pipelines, disable_coverage: true, preload: true) .represent(@pipelines, disable_coverage: true, preload: true)
end end
end
def render_show def render_show
respond_to do |format| respond_to do |format|
......
---
title: Fix and expand Gitaly FindCommit caching
merge_request: 27018
author:
type: performance
...@@ -60,6 +60,8 @@ describe Projects::MergeRequestsController do ...@@ -60,6 +60,8 @@ describe Projects::MergeRequestsController do
end end
it "renders merge request page" do it "renders merge request page" do
expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
go(format: :html) go(format: :html)
expect(response).to be_success expect(response).to be_success
......
...@@ -97,6 +97,8 @@ describe Projects::PipelinesController do ...@@ -97,6 +97,8 @@ describe Projects::PipelinesController do
RequestStore.clear! RequestStore.clear!
RequestStore.begin! RequestStore.begin!
expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
expect { get_pipelines_index_json } expect { get_pipelines_index_json }
.to change { Gitlab::GitalyClient.get_request_count }.by(2) .to change { Gitlab::GitalyClient.get_request_count }.by(2)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment