Optimize merge request refresh by using the database to check commit SHAs
Previously for a given merge request, we would: 1. Create the array of commit SHAs involved in the push (A) 2. Request all merge request commits and map the SHA (B) 3. Reload the diff if there were any common commits between A and B We can avoid additional database querying and overhead by checking with the database whether the merge request contains any of the commit SHAs. Relates to https://gitlab.com/gitlab-org/gitlab-ce/issues/53213
This commit is contained in:
parent
6f12e3929d
commit
dbc03ce3a9
|
@ -353,6 +353,15 @@ class MergeRequest < ActiveRecord::Base
|
|||
end
|
||||
end
|
||||
|
||||
# Returns true if there are commits that match at least one commit SHA.
|
||||
def includes_any_commits?(shas)
|
||||
if persisted?
|
||||
merge_request_diff.commits_by_shas(shas).exists?
|
||||
else
|
||||
(commit_shas & shas).present?
|
||||
end
|
||||
end
|
||||
|
||||
# Calls `MergeWorker` to proceed with the merge process and
|
||||
# updates `merge_jid` with the MergeWorker#jid.
|
||||
# This helps tracking enqueued and ongoing merge jobs.
|
||||
|
|
|
@ -140,6 +140,12 @@ class MergeRequestDiff < ActiveRecord::Base
|
|||
merge_request_diff_commits.map(&:sha)
|
||||
end
|
||||
|
||||
def commits_by_shas(shas)
|
||||
return [] unless shas.present?
|
||||
|
||||
merge_request_diff_commits.where(sha: shas)
|
||||
end
|
||||
|
||||
def diff_refs=(new_diff_refs)
|
||||
self.base_commit_sha = new_diff_refs&.base_sha
|
||||
self.start_commit_sha = new_diff_refs&.start_sha
|
||||
|
|
|
@ -87,11 +87,8 @@ module MergeRequests
|
|||
filter_merge_requests(merge_requests).each do |merge_request|
|
||||
if branch_and_project_match?(merge_request) || @push.force_push?
|
||||
merge_request.reload_diff(current_user)
|
||||
else
|
||||
mr_commit_ids = merge_request.commit_shas
|
||||
push_commit_ids = @commits.map(&:id)
|
||||
matches = mr_commit_ids & push_commit_ids
|
||||
merge_request.reload_diff(current_user) if matches.any?
|
||||
elsif merge_request.includes_any_commits?(push_commit_ids)
|
||||
merge_request.reload_diff(current_user)
|
||||
end
|
||||
|
||||
merge_request.mark_as_unchecked
|
||||
|
@ -104,6 +101,10 @@ module MergeRequests
|
|||
end
|
||||
# rubocop: enable CodeReuse/ActiveRecord
|
||||
|
||||
def push_commit_ids
|
||||
@push_commit_ids ||= @commits.map(&:id)
|
||||
end
|
||||
|
||||
def branch_and_project_match?(merge_request)
|
||||
merge_request.source_project == @project &&
|
||||
merge_request.source_branch == @push.branch_name
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Optimize merge request refresh by using the database to check commit SHAs
|
||||
merge_request: 22731
|
||||
author:
|
||||
type: performance
|
|
@ -211,4 +211,25 @@ describe MergeRequestDiff do
|
|||
expect(diff_with_commits.commits_count).to eq(29)
|
||||
end
|
||||
end
|
||||
|
||||
describe '#commits_by_shas' do
|
||||
let(:commit_shas) { diff_with_commits.commit_shas }
|
||||
|
||||
it 'returns empty if no SHAs were provided' do
|
||||
expect(diff_with_commits.commits_by_shas([])).to be_empty
|
||||
end
|
||||
|
||||
it 'returns one SHA' do
|
||||
commits = diff_with_commits.commits_by_shas([commit_shas.first, Gitlab::Git::BLANK_SHA])
|
||||
|
||||
expect(commits.count).to eq(1)
|
||||
end
|
||||
|
||||
it 'returns all matching SHAs' do
|
||||
commits = diff_with_commits.commits_by_shas(commit_shas)
|
||||
|
||||
expect(commits.count).to eq(commit_shas.count)
|
||||
expect(commits.map(&:sha)).to match_array(commit_shas)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -2611,6 +2611,32 @@ describe MergeRequest do
|
|||
end
|
||||
end
|
||||
|
||||
describe '#includes_any_commits?' do
|
||||
it 'returns false' do
|
||||
expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
|
||||
end
|
||||
|
||||
it 'returns true' do
|
||||
expect(subject.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
|
||||
end
|
||||
|
||||
it 'returns true even when there is a non-existent comit' do
|
||||
expect(subject.includes_any_commits?([Gitlab::Git::BLANK_SHA, subject.merge_request_diff.head_commit_sha])).to be_truthy
|
||||
end
|
||||
|
||||
context 'unpersisted merge request' do
|
||||
let(:new_mr) { build(:merge_request) }
|
||||
|
||||
it 'returns false' do
|
||||
expect(new_mr.includes_any_commits?([Gitlab::Git::BLANK_SHA])).to be_falsey
|
||||
end
|
||||
|
||||
it 'returns true' do
|
||||
expect(new_mr.includes_any_commits?([subject.merge_request_diff.head_commit_sha])).to be_truthy
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '#can_allow_collaboration?' do
|
||||
let(:target_project) { create(:project, :public) }
|
||||
let(:source_project) { fork_project(target_project) }
|
||||
|
|
Loading…
Reference in New Issue