Optimize merge request refresh by using the database to check commit SHAssh-optimize-mr-commit-sha-lookup

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

6 files changed, 73 insertions, 5 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7eef08aa6a3..735d9fba966 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -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.
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 02c6b650f33..bb6ff8921df 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -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
diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb
index f01872b205e..53768ff2cbe 100644
--- a/app/services/merge_requests/refresh_service.rb
+++ b/app/services/merge_requests/refresh_service.rb
@@ -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
diff --git a/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml b/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml
new file mode 100644
index 00000000000..bea73f8d329
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-mr-commit-sha-lookup.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize merge request refresh by using the database to check commit SHAs
+merge_request: 22731
+author:
+type: performance
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 562ccaf6c0b..47e8f04e728 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 7d500f9e579..2eb5e39ccfd 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -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) }