From d13fcca2acc01d1fbbf6240d694fbf1bb69b073e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 3 Mar 2019 23:33:20 -0800 Subject: Optimize MergeRequestDiff#last_commit_sha The previous implementation would load all the MergeRequestDiffCommit entries into memory and retrieve the SHA from the latest entry. We can save a bit of query time and memory by retrieving the SHA directly from the first item from the database. This is particularly helpful when there are many merge requests that need this information. Seen while investigating Todo API performance in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25711. --- app/models/merge_request_diff.rb | 2 +- spec/models/merge_request_diff_spec.rb | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index e286a4e57f2..3556448e547 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -118,7 +118,7 @@ class MergeRequestDiff < ActiveRecord::Base end def last_commit_sha - commit_shas.first + merge_request_diff_commits.first&.sha end def first_commit diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 1849d3bac12..fff3e810977 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe MergeRequestDiff do - let(:diff_with_commits) { create(:merge_request).merge_request_diff } + let(:merge_request) { create(:merge_request) } + let(:diff_with_commits) { merge_request.merge_request_diff } describe 'create new record' do subject { diff_with_commits } @@ -224,6 +225,18 @@ describe MergeRequestDiff do end end + describe '#last_commit_sha' do + let(:commit_shas) { diff_with_commits.commit_shas } + let(:sorted_commits) { merge_request.merge_request_diff.merge_request_diff_commits.sort_by { |x| x.committed_date } } + let(:last_commit_sha) { diff_with_commits.last_commit_sha } + + it 'returns the most recent commit SHA' do + expect(last_commit_sha).to eq(commit_shas.first) + # Sanity check the sort order + expect(last_commit_sha).to eq(sorted_commits.last.sha) + end + end + describe '#commits_by_shas' do let(:commit_shas) { diff_with_commits.commit_shas } -- cgit v1.2.1