From a2f3675dc7d8faaa33b2465df95a00c08b15eea1 Mon Sep 17 00:00:00 2001
From: Marc Shaw <mshaw@gitlab.com>
Date: Fri, 3 Jul 2020 13:42:31 +0000
Subject: [PATCH] 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
---
app/models/merge_request.rb | 17 +++++-
..._batch-diffs-metadata-source-branch-ex.yml | 5 ++
spec/models/merge_request_spec.rb | 53 +++++++++++++++++++
.../conflicts/list_service_spec.rb | 1 +
4 files changed, 74 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/209786-improve-performance-of-diffs_batch-diffs-metadata-source-branch-ex.yml
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index d4a56709edb5f..6a3fba93aea87 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -107,6 +107,7 @@ def merge_request_diff
after_create :ensure_merge_request_diff
after_update :clear_memoized_shas
+ after_update :clear_memoized_source_branch_exists
after_update :reload_diff_if_branch_changed
after_commit :ensure_metrics, on: [:create, :update], unless: :importing?
after_commit :expire_etag_cache, unless: :importing?
@@ -862,6 +863,10 @@ def clear_memoized_shas
clear_memoization(:target_branch_head)
end
+ def clear_memoized_source_branch_exists
+ clear_memoization(:source_branch_exists)
+ end
+
def reload_diff_if_branch_changed
if (saved_change_to_source_branch? || saved_change_to_target_branch?) &&
(source_branch_head && target_branch_head)
@@ -1109,9 +1114,17 @@ def target_project_namespace
end
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
def target_branch_exists?
diff --git a/changelogs/unreleased/209786-improve-performance-of-diffs_batch-diffs-metadata-source-branch-ex.yml b/changelogs/unreleased/209786-improve-performance-of-diffs_batch-diffs-metadata-source-branch-ex.yml
new file mode 100644
index 0000000000000..215d64bbacbcd
--- /dev/null
+++ b/changelogs/unreleased/209786-improve-performance-of-diffs_batch-diffs-metadata-source-branch-ex.yml
@@ -0,0 +1,5 @@
+---
+title: Further improve the performance for loading large diffs on a Merge request
+merge_request: 34516
+author:
+type: performance
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 332f5d62c62b7..4853fc2438e43 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1217,6 +1217,59 @@ def set_compare(merge_request)
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
it 'includes merge information as the title' do
request = build(:merge_request, source_branch: 'source', target_branch: 'target')
diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb
index cd9eae2f2eed9..14133731e3795 100644
--- a/spec/services/merge_requests/conflicts/list_service_spec.rb
+++ b/spec/services/merge_requests/conflicts/list_service_spec.rb
@@ -30,6 +30,7 @@ def conflicts_service(merge_request)
it 'returns a falsey value when one of the MR branches is missing' do
merge_request = create_merge_request('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
end
--
GitLab