From 39c9928cfbf5b72042755c58bd5c484551c5f1ad Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 20 Mar 2018 15:51:54 +0000 Subject: 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. --- app/models/merge_request.rb | 27 ++++++++++++++-------- ...uery-count-for-mergerequestscontroller-show.yml | 5 ++++ spec/models/merge_request_spec.rb | 11 +++++++++ 3 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7e6d89ec9c7..91d8be5559b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -536,18 +536,25 @@ class MergeRequest < ActiveRecord::Base merge_request_diff(true) end + def viewable_diffs + @viewable_diffs ||= merge_request_diffs.viewable.to_a + end + 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| - diffs = merge_request_diffs.viewable - h[diff_refs_or_sha] = - if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs) - diffs.find_by_diff_refs(diff_refs_or_sha) - else - diffs.find_by(head_commit_sha: diff_refs_or_sha) - end - end + matcher = + if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs) + { + '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 + { 'head_commit_sha' => diff_refs_or_sha } + 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 def version_params_for(diff_refs) diff --git a/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml b/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml new file mode 100644 index 00000000000..1f793fe5e7c --- /dev/null +++ b/changelogs/unreleased/reduce-query-count-for-mergerequestscontroller-show.yml @@ -0,0 +1,5 @@ +--- +title: Reduce number of queries when viewing a merge request +merge_request: +author: +type: performance diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index ff5a6f63010..f73f44ca0ad 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1961,6 +1961,17 @@ describe MergeRequest do expect(subject.merge_request_diff_for(merge_request_diff3.head_commit_sha)).to eq(merge_request_diff3) 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 describe '#version_params_for' do -- cgit v1.2.1