Commit a2f3675d authored by Marc Shaw's avatar Marc Shaw Committed by Nick Thomas

Memoize the source_branch_exists method for a MR

Each time this will make a redis call, which is expensive if called
enough times. This was an issue when serializing large diffs for a merge
request.

Clear the memoized source_branch_exists on MR update
Clear memoized variables before proceeding with checks

Issue: gitlab.com/gitlab-org/gitlab/-/issues/209786
Merge Request: gitlab.com/gitlab-org/gitlab/-/merge_requests/34516
parent 0d34cf28
...@@ -107,6 +107,7 @@ class MergeRequest < ApplicationRecord ...@@ -107,6 +107,7 @@ class MergeRequest < ApplicationRecord
after_create :ensure_merge_request_diff after_create :ensure_merge_request_diff
after_update :clear_memoized_shas after_update :clear_memoized_shas
after_update :clear_memoized_source_branch_exists
after_update :reload_diff_if_branch_changed after_update :reload_diff_if_branch_changed
after_commit :ensure_metrics, on: [:create, :update], unless: :importing? after_commit :ensure_metrics, on: [:create, :update], unless: :importing?
after_commit :expire_etag_cache, unless: :importing? after_commit :expire_etag_cache, unless: :importing?
...@@ -862,6 +863,10 @@ class MergeRequest < ApplicationRecord ...@@ -862,6 +863,10 @@ class MergeRequest < ApplicationRecord
clear_memoization(:target_branch_head) clear_memoization(:target_branch_head)
end end
def clear_memoized_source_branch_exists
clear_memoization(:source_branch_exists)
end
def reload_diff_if_branch_changed def reload_diff_if_branch_changed
if (saved_change_to_source_branch? || saved_change_to_target_branch?) && if (saved_change_to_source_branch? || saved_change_to_target_branch?) &&
(source_branch_head && target_branch_head) (source_branch_head && target_branch_head)
...@@ -1109,9 +1114,17 @@ class MergeRequest < ApplicationRecord ...@@ -1109,9 +1114,17 @@ class MergeRequest < ApplicationRecord
end end
def source_branch_exists? def source_branch_exists?
return false unless self.source_project if Feature.enabled?(:memoize_source_branch_merge_request, project)
strong_memoize(:source_branch_exists) do
next false unless self.source_project
self.source_project.repository.branch_exists?(self.source_branch)
end
else
return false unless self.source_project
self.source_project.repository.branch_exists?(self.source_branch) self.source_project.repository.branch_exists?(self.source_branch)
end
end end
def target_branch_exists? def target_branch_exists?
......
---
title: Further improve the performance for loading large diffs on a Merge request
merge_request: 34516
author:
type: performance
...@@ -1217,6 +1217,59 @@ RSpec.describe MergeRequest do ...@@ -1217,6 +1217,59 @@ RSpec.describe MergeRequest do
end end
end end
describe "#source_branch_exists?" do
let(:merge_request) { subject }
let(:repository) { merge_request.source_project.repository }
context 'when memoize_source_branch_merge_request feature is enabled' do
before do
stub_feature_flags(memoize_source_branch_merge_request: true)
end
context 'when the source project is set' do
it 'memoizes the value and returns the result' do
expect(repository).to receive(:branch_exists?).once.with(merge_request.source_branch).and_return(true)
2.times { expect(merge_request.source_branch_exists?).to eq(true) }
end
end
context 'when the source project is not set' do
before do
merge_request.source_project = nil
end
it 'returns false' do
expect(merge_request.source_branch_exists?).to eq(false)
end
end
end
context 'when memoize_source_branch_merge_request feature is disabled' do
before do
stub_feature_flags(memoize_source_branch_merge_request: false)
end
context 'when the source project is set' do
it 'does not memoize the value and returns the result' do
expect(repository).to receive(:branch_exists?).twice.with(merge_request.source_branch).and_return(true)
2.times { expect(merge_request.source_branch_exists?).to eq(true) }
end
end
context 'when the source project is not set' do
before do
merge_request.source_project = nil
end
it 'returns false' do
expect(merge_request.source_branch_exists?).to eq(false)
end
end
end
end
describe '#default_merge_commit_message' do describe '#default_merge_commit_message' do
it 'includes merge information as the title' do it 'includes merge information as the title' do
request = build(:merge_request, source_branch: 'source', target_branch: 'target') request = build(:merge_request, source_branch: 'source', target_branch: 'target')
......
...@@ -30,6 +30,7 @@ RSpec.describe MergeRequests::Conflicts::ListService do ...@@ -30,6 +30,7 @@ RSpec.describe MergeRequests::Conflicts::ListService do
it 'returns a falsey value when one of the MR branches is missing' do it 'returns a falsey value when one of the MR branches is missing' do
merge_request = create_merge_request('conflict-resolvable') merge_request = create_merge_request('conflict-resolvable')
merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable') merge_request.project.repository.rm_branch(merge_request.author, 'conflict-resolvable')
merge_request.clear_memoized_source_branch_exists
expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
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