Commit 39c9928c authored by Sean McGivern's avatar Sean McGivern

Fix N+1 in `MergeRequest#merge_request_diff_for`

Previously, this would issue a query for each unique `diff_refs_or_sha`
passed. This was because we didn't want to load other MR diffs into memory, as
they had some very large columns.

Now they are actually very small, and it's more efficient to just load them all
at once and do the finding in Ruby.
parent 398f13f3
...@@ -536,18 +536,25 @@ class MergeRequest < ActiveRecord::Base ...@@ -536,18 +536,25 @@ class MergeRequest < ActiveRecord::Base
merge_request_diff(true) merge_request_diff(true)
end end
def viewable_diffs
@viewable_diffs ||= merge_request_diffs.viewable.to_a
end
def merge_request_diff_for(diff_refs_or_sha) def merge_request_diff_for(diff_refs_or_sha)
@merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha| matcher =
diffs = merge_request_diffs.viewable
h[diff_refs_or_sha] =
if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs) if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs)
diffs.find_by_diff_refs(diff_refs_or_sha) {
'start_commit_sha' => diff_refs_or_sha.start_sha,
'head_commit_sha' => diff_refs_or_sha.head_sha,
'base_commit_sha' => diff_refs_or_sha.base_sha
}
else else
diffs.find_by(head_commit_sha: diff_refs_or_sha) { 'head_commit_sha' => diff_refs_or_sha }
end
end end
@merge_request_diffs_by_diff_refs_or_sha[diff_refs_or_sha] viewable_diffs.find do |diff|
diff.attributes.slice(*matcher.keys) == matcher
end
end end
def version_params_for(diff_refs) def version_params_for(diff_refs)
......
---
title: Reduce number of queries when viewing a merge request
merge_request:
author:
type: performance
...@@ -1961,6 +1961,17 @@ describe MergeRequest do ...@@ -1961,6 +1961,17 @@ describe MergeRequest do
expect(subject.merge_request_diff_for(merge_request_diff3.head_commit_sha)).to eq(merge_request_diff3) expect(subject.merge_request_diff_for(merge_request_diff3.head_commit_sha)).to eq(merge_request_diff3)
end end
end end
it 'runs a single query on the initial call, and none afterwards' do
expect { subject.merge_request_diff_for(merge_request_diff1.diff_refs) }
.not_to exceed_query_limit(1)
expect { subject.merge_request_diff_for(merge_request_diff2.diff_refs) }
.not_to exceed_query_limit(0)
expect { subject.merge_request_diff_for(merge_request_diff3.head_commit_sha) }
.not_to exceed_query_limit(0)
end
end end
describe '#version_params_for' do describe '#version_params_for' do
......
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