Commit ac22576d authored by Robert Speicher's avatar Robert Speicher

Merge branch 'zj-gitaly-pipelines-n-1' into 'master'

Improve performance of Pipelines#index.json

See merge request gitlab-org/gitlab-ce!14846
parents 4e60b4f1 c6edae38
...@@ -263,7 +263,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0' ...@@ -263,7 +263,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0'
gem 'gettext_i18n_rails_js', '~> 1.2.0' gem 'gettext_i18n_rails_js', '~> 1.2.0'
gem 'gettext', '~> 3.2.2', require: false, group: :development gem 'gettext', '~> 3.2.2', require: false, group: :development
gem 'batch-loader' gem 'batch-loader', '~> 1.2.1'
# Perf bar # Perf bar
gem 'peek', '~> 1.0.1' gem 'peek', '~> 1.0.1'
......
...@@ -78,7 +78,7 @@ GEM ...@@ -78,7 +78,7 @@ GEM
thread_safe (~> 0.3, >= 0.3.1) thread_safe (~> 0.3, >= 0.3.1)
babosa (1.0.2) babosa (1.0.2)
base32 (0.3.2) base32 (0.3.2)
batch-loader (1.1.1) batch-loader (1.2.1)
bcrypt (3.1.11) bcrypt (3.1.11)
bcrypt_pbkdf (1.0.0) bcrypt_pbkdf (1.0.0)
benchmark-ips (2.3.0) benchmark-ips (2.3.0)
...@@ -988,7 +988,7 @@ DEPENDENCIES ...@@ -988,7 +988,7 @@ DEPENDENCIES
awesome_print (~> 1.2.0) awesome_print (~> 1.2.0)
babosa (~> 1.0.2) babosa (~> 1.0.2)
base32 (~> 0.3.0) base32 (~> 0.3.0)
batch-loader batch-loader (~> 1.2.1)
bcrypt_pbkdf (~> 1.0) bcrypt_pbkdf (~> 1.0)
benchmark-ips (~> 2.3.0) benchmark-ips (~> 2.3.0)
better_errors (~> 2.1.0) better_errors (~> 2.1.0)
......
...@@ -29,6 +29,8 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -29,6 +29,8 @@ class Projects::PipelinesController < Projects::ApplicationController
@pipelines_count = PipelinesFinder @pipelines_count = PipelinesFinder
.new(project).execute.count .new(project).execute.count
@pipelines.map(&:commit) # List commits for batch loading
respond_to do |format| respond_to do |format|
format.html format.html
format.json do format.json do
......
...@@ -287,8 +287,12 @@ module Ci ...@@ -287,8 +287,12 @@ module Ci
Ci::Pipeline.truncate_sha(sha) Ci::Pipeline.truncate_sha(sha)
end end
# NOTE: This is loaded lazily and will never be nil, even if the commit
# cannot be found.
#
# Use constructs like: `pipeline.commit.present?`
def commit def commit
@commit ||= project.commit_by(oid: sha) @commit ||= Commit.lazy(project, sha)
end end
def branch? def branch?
...@@ -338,12 +342,9 @@ module Ci ...@@ -338,12 +342,9 @@ module Ci
end end
def latest? def latest?
return false unless ref return false unless ref && commit.present?
commit = project.commit(ref)
return false unless commit
commit.sha == sha project.commit(ref) == commit
end end
def retried def retried
......
...@@ -86,6 +86,20 @@ class Commit ...@@ -86,6 +86,20 @@ class Commit
def valid_hash?(key) def valid_hash?(key)
!!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key) !!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key)
end end
def lazy(project, oid)
BatchLoader.for({ project: project, oid: oid }).batch do |items, loader|
items_by_project = items.group_by { |i| i[:project] }
items_by_project.each do |project, commit_ids|
oids = commit_ids.map { |i| i[:oid] }
project.repository.commits_by(oids: oids).each do |commit|
loader.call({ project: commit.project, oid: commit.id }, commit) if commit
end
end
end
end
end end
attr_accessor :raw attr_accessor :raw
...@@ -103,7 +117,7 @@ class Commit ...@@ -103,7 +117,7 @@ class Commit
end end
def ==(other) def ==(other)
(self.class === other) && (raw == other.raw) other.is_a?(self.class) && raw == other.raw
end end
def self.reference_prefix def self.reference_prefix
...@@ -224,8 +238,8 @@ class Commit ...@@ -224,8 +238,8 @@ class Commit
notes.includes(:author) notes.includes(:author)
end end
def method_missing(m, *args, &block) def method_missing(method, *args, &block)
@raw.__send__(m, *args, &block) # rubocop:disable GitlabSecurity/PublicSend @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
end end
def respond_to_missing?(method, include_private = false) def respond_to_missing?(method, include_private = false)
......
...@@ -118,6 +118,18 @@ class Repository ...@@ -118,6 +118,18 @@ class Repository
@commit_cache[oid] = find_commit(oid) @commit_cache[oid] = find_commit(oid)
end end
def commits_by(oids:)
return [] unless oids.present?
commits = Gitlab::Git::Commit.batch_by_oid(raw_repository, oids)
if commits.present?
Commit.decorate(commits, @project)
else
[]
end
end
def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil) def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil)
options = { options = {
repo: raw_repository, repo: raw_repository,
......
#js-pipeline-header-vue.pipeline-header-container #js-pipeline-header-vue.pipeline-header-container
- if @commit - if @commit.present?
.commit-box .commit-box
%h3.commit-title %h3.commit-title
= markdown(@commit.title, pipeline: :single_line) = markdown(@commit.title, pipeline: :single_line)
...@@ -8,28 +8,28 @@ ...@@ -8,28 +8,28 @@
%pre.commit-description %pre.commit-description
= preserve(markdown(@commit.description, pipeline: :single_line)) = preserve(markdown(@commit.description, pipeline: :single_line))
.info-well .info-well
- if @commit.status - if @commit.status
.well-segment.pipeline-info .well-segment.pipeline-info
.icon-container .icon-container
= icon('clock-o') = icon('clock-o')
= pluralize @pipeline.total_size, "job" = pluralize @pipeline.total_size, "job"
- if @pipeline.ref - if @pipeline.ref
from from
= link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
- if @pipeline.duration - if @pipeline.duration
in in
= time_interval_in_words(@pipeline.duration) = time_interval_in_words(@pipeline.duration)
- if @pipeline.queued_duration - if @pipeline.queued_duration
= "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
.well-segment.branch-info .well-segment.branch-info
.icon-container.commit-icon .icon-container.commit-icon
= custom_icon("icon_commit") = custom_icon("icon_commit")
= link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short" = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short"
= link_to("#", class: "js-details-expand hidden-xs hidden-sm") do = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do
%span.text-expander %span.text-expander
\... \...
%span.js-details-content.hide %span.js-details-content.hide
= link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full" = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full"
= clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard") = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard")
...@@ -13,7 +13,7 @@ class ExpirePipelineCacheWorker ...@@ -13,7 +13,7 @@ class ExpirePipelineCacheWorker
store.touch(project_pipelines_path(project)) store.touch(project_pipelines_path(project))
store.touch(project_pipeline_path(project, pipeline)) store.touch(project_pipeline_path(project, pipeline))
store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil?
store.touch(new_merge_request_pipelines_path(project)) store.touch(new_merge_request_pipelines_path(project))
each_pipelines_merge_request_path(project, pipeline) do |path| each_pipelines_merge_request_path(project, pipeline) do |path|
store.touch(path) store.touch(path)
......
...@@ -228,6 +228,19 @@ module Gitlab ...@@ -228,6 +228,19 @@ module Gitlab
end end
end end
end end
# Only to be used when the object ids will not necessarily have a
# relation to each other. The last 10 commits for a branch for example,
# should go through .where
def batch_by_oid(repo, oids)
repo.gitaly_migrate(:list_commits_by_oid) do |is_enabled|
if is_enabled
repo.gitaly_commit_client.list_commits_by_oid(oids)
else
oids.map { |oid| find(repo, oid) }.compact
end
end
end
end end
def initialize(repository, raw_commit, head = nil) def initialize(repository, raw_commit, head = nil)
......
...@@ -169,6 +169,15 @@ module Gitlab ...@@ -169,6 +169,15 @@ module Gitlab
consume_commits_response(response) consume_commits_response(response)
end end
def list_commits_by_oid(oids)
request = Gitaly::ListCommitsByOidRequest.new(repository: @gitaly_repo, oid: oids)
response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_oid, request, timeout: GitalyClient.medium_timeout)
consume_commits_response(response)
rescue GRPC::Unknown # If no repository is found, happens mainly during testing
[]
end
def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0) def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0)
request = Gitaly::CommitsByMessageRequest.new( request = Gitaly::CommitsByMessageRequest.new(
repository: @gitaly_repo, repository: @gitaly_repo,
......
...@@ -17,13 +17,10 @@ describe Projects::PipelinesController do ...@@ -17,13 +17,10 @@ describe Projects::PipelinesController do
describe 'GET index.json' do describe 'GET index.json' do
before do before do
branch_head = project.commit %w(pending running created success).each_with_index do |status, index|
parent = branch_head.parent sha = project.commit("HEAD~#{index}")
create(:ci_empty_pipeline, status: status, project: project, sha: sha)
create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id) end
create(:ci_empty_pipeline, status: 'running', project: project, sha: branch_head.id)
create(:ci_empty_pipeline, status: 'created', project: project, sha: parent.id)
create(:ci_empty_pipeline, status: 'success', project: project, sha: parent.id)
end end
subject do subject do
...@@ -46,7 +43,7 @@ describe Projects::PipelinesController do ...@@ -46,7 +43,7 @@ describe Projects::PipelinesController do
context 'when performing gitaly calls', :request_store do context 'when performing gitaly calls', :request_store do
it 'limits the Gitaly requests' do it 'limits the Gitaly requests' do
expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(8) expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(3)
end end
end end
end end
......
...@@ -13,6 +13,45 @@ describe Commit do ...@@ -13,6 +13,45 @@ describe Commit do
it { is_expected.to include_module(StaticModel) } it { is_expected.to include_module(StaticModel) }
end end
describe '.lazy' do
set(:project) { create(:project, :repository) }
context 'when the commits are found' do
let(:oids) do
%w(
498214de67004b1da3d820901307bed2a68a8ef6
c642fe9b8b9f28f9225d7ea953fe14e74748d53b
6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9
048721d90c449b244b7b4c53a9186b04330174ec
281d3a76f31c812dbf48abce82ccf6860adedd81
)
end
subject { oids.map { |oid| described_class.lazy(project, oid) } }
it 'batches requests for commits' do
expect(project.repository).to receive(:commits_by).once.and_call_original
subject.first.title
subject.last.title
end
it 'maintains ordering' do
subject.each_with_index do |commit, i|
expect(commit.id).to eq(oids[i])
end
end
end
context 'when not found' do
it 'returns nil as commit' do
commit = described_class.lazy(project, 'deadbeef').__sync
expect(commit).to be_nil
end
end
end
describe '#author' do describe '#author' do
it 'looks up the author in a case-insensitive way' do it 'looks up the author in a case-insensitive way' do
user = create(:user, email: commit.author_email.upcase) user = create(:user, email: commit.author_email.upcase)
......
...@@ -239,6 +239,54 @@ describe Repository do ...@@ -239,6 +239,54 @@ describe Repository do
end end
end end
describe '#commits_by' do
set(:project) { create(:project, :repository) }
shared_examples 'batch commits fetching' do
let(:oids) { TestEnv::BRANCH_SHA.values }
subject { project.repository.commits_by(oids: oids) }
it 'finds each commit' do
expect(subject).not_to include(nil)
expect(subject.size).to eq(oids.size)
end
it 'returns only Commit instances' do
expect(subject).to all( be_a(Commit) )
end
context 'when some commits are not found ' do
let(:oids) do
['deadbeef'] + TestEnv::BRANCH_SHA.values.first(10)
end
it 'returns only found commits' do
expect(subject).not_to include(nil)
expect(subject.size).to eq(10)
end
end
context 'when no oids are passed' do
let(:oids) { [] }
it 'does not call #batch_by_oid' do
expect(Gitlab::Git::Commit).not_to receive(:batch_by_oid)
subject
end
end
end
context 'when Gitaly list_commits_by_oid is enabled' do
it_behaves_like 'batch commits fetching'
end
context 'when Gitaly list_commits_by_oid is enabled', :disable_gitaly do
it_behaves_like 'batch commits fetching'
end
end
describe '#find_commits_by_message' do describe '#find_commits_by_message' do
shared_examples 'finding commits by message' do shared_examples 'finding commits by message' do
it 'returns commits with messages containing a given string' do it 'returns commits with messages containing a given string' do
......
require 'spec_helper' require 'spec_helper'
describe PipelineSerializer do describe PipelineSerializer do
set(:project) { create(:project, :repository) }
set(:user) { create(:user) } set(:user) { create(:user) }
let(:serializer) do let(:serializer) do
...@@ -16,7 +17,7 @@ describe PipelineSerializer do ...@@ -16,7 +17,7 @@ describe PipelineSerializer do
end end
context 'when a single object is being serialized' do context 'when a single object is being serialized' do
let(:resource) { create(:ci_empty_pipeline) } let(:resource) { create(:ci_empty_pipeline, project: project) }
it 'serializers the pipeline object' do it 'serializers the pipeline object' do
expect(subject[:id]).to eq resource.id expect(subject[:id]).to eq resource.id
...@@ -24,7 +25,7 @@ describe PipelineSerializer do ...@@ -24,7 +25,7 @@ describe PipelineSerializer do
end end
context 'when multiple objects are being serialized' do context 'when multiple objects are being serialized' do
let(:resource) { create_list(:ci_pipeline, 2) } let(:resource) { create_list(:ci_pipeline, 2, project: project) }
it 'serializers the array of pipelines' do it 'serializers the array of pipelines' do
expect(subject).not_to be_empty expect(subject).not_to be_empty
...@@ -100,7 +101,6 @@ describe PipelineSerializer do ...@@ -100,7 +101,6 @@ describe PipelineSerializer do
context 'number of queries' do context 'number of queries' do
let(:resource) { Ci::Pipeline.all } let(:resource) { Ci::Pipeline.all }
let(:project) { create(:project) }
before do before do
# Since RequestStore.active? is true we have to allow the # Since RequestStore.active? is true we have to allow the
......
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