Merge branch 'zj-memoization-mr-commits-ee' into 'master'

Use memoization for commits on diffs

See merge request gitlab-org/gitlab-ee!3744

app/models/merge_request.rb

@@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base
   include TimeTrackable
   include ManualInverseAssociation
   include EachBatch
+  include Gitlab::Utils::StrongMemoize
 
   ignore_column :locked_at,
                 :ref_fetched
@@ -55,6 +56,7 @@ class MergeRequest < ActiveRecord::Base
   serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize
 
   after_create :ensure_merge_request_diff, unless: :importing?
+  after_update :clear_memoized_shas
   after_update :reload_diff_if_branch_changed
 
   # When this attribute is true some MR validation is ignored
@@ -403,13 +405,17 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def source_branch_head
-    return unless source_project
-
-    source_project.repository.commit(source_branch_ref) if source_branch_ref
+    strong_memoize(:source_branch_head) do
+      if source_project && source_branch_ref
+        source_project.repository.commit(source_branch_ref)
+      end
+    end
   end
 
   def target_branch_head
-    target_project.repository.commit(target_branch_ref)
+    strong_memoize(:target_branch_head) do
+      target_project.repository.commit(target_branch_ref)
+    end
   end
 
   def branch_merge_base_commit
@@ -557,6 +563,13 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
+  def clear_memoized_shas
+    @target_branch_sha = @source_branch_sha = nil
+
+    clear_memoization(:source_branch_head)
+    clear_memoization(:target_branch_head)
+  end
+
   def reload_diff_if_branch_changed
     if (source_branch_changed? || target_branch_changed?) &&
         (source_branch_head && target_branch_head)

app/models/merge_request_diff.rb

@@ -104,19 +104,19 @@ class MergeRequestDiff < ActiveRecord::Base
   def base_commit
     return unless base_commit_sha
 
-    project.commit(base_commit_sha)
+    project.commit_by(oid: base_commit_sha)
   end
 
   def start_commit
     return unless start_commit_sha
 
-    project.commit(start_commit_sha)
+    project.commit_by(oid: start_commit_sha)
   end
 
   def head_commit
     return unless head_commit_sha
 
-    project.commit(head_commit_sha)
+    project.commit_by(oid: head_commit_sha)
   end
 
   def commit_shas

changelogs/unreleased/zj-memoization-mr-commits.yml

+---
+title: Cache commits for MergeRequest diffs
+merge_request:
+author:
+type: performance

lib/gitlab/conflict/file_collection.rb

@@ -19,6 +19,8 @@ module Gitlab
           commit_message: commit_message || default_commit_message
         }
         resolver.resolve_conflicts(user, files, args)
+      ensure
+        @merge_request.clear_memoized_shas
       end
 
       def files

lib/gitlab/utils/strong_memoize.rb

@@ -11,6 +11,8 @@ module Gitlab
       #
       # We could write it like:
       #
+      #     include Gitlab::Utils::StrongMemoize
+      #
       #     def trigger_from_token
       #       strong_memoize(:trigger) do
       #         Ci::Trigger.find_by_token(params[:token].to_s)
@@ -18,14 +20,22 @@ module Gitlab
       #     end
       #
       def strong_memoize(name)
-        ivar_name = "@#{name}"
-
-        if instance_variable_defined?(ivar_name)
-          instance_variable_get(ivar_name)
+        if instance_variable_defined?(ivar(name))
+          instance_variable_get(ivar(name))
         else
-          instance_variable_set(ivar_name, yield)
+          instance_variable_set(ivar(name), yield)
         end
       end
+
+      def clear_memoization(name)
+        remove_instance_variable(ivar(name)) if instance_variable_defined?(ivar(name))
+      end
+
+      private
+
+      def ivar(name)
+        "@#{name}"
+      end
     end
   end
 end

spec/lib/gitlab/utils/strong_memoize_spec.rb

@@ -49,4 +49,16 @@ describe Gitlab::Utils::StrongMemoize do
       end
     end
   end
+
+  describe '#clear_memoization' do
+    let(:value) { 'mepmep' }
+
+    it 'removes the instance variable' do
+      object.method_name
+
+      object.clear_memoization(:method_name)
+
+      expect(object.instance_variable_defined?(:@method_name)).to be(false)
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -124,6 +124,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
         project.repository.rm_branch(subject.author, subject.target_branch)
+        subject.clear_memoized_shas
       end
 
       it 'returns nil' do
@@ -809,30 +810,30 @@ describe MergeRequest do
   end
 
   describe '#can_remove_source_branch?' do
-    let(:user) { create(:user) }
-    let(:user2) { create(:user) }
+    set(:user) { create(:user) }
+    set(:merge_request) { create(:merge_request, :simple) }
 
-    before do
-      subject.source_project.team << [user, :master]
+    subject { merge_request }
 
-      subject.source_branch = "feature"
-      subject.target_branch = "master"
-      subject.save!
+    before do
+      subject.source_project.add_master(user)
     end
 
     it "can't be removed when its a protected branch" do
       allow(ProtectedBranch).to receive(:protected?).and_return(true)
+
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
     it "can't remove a root ref" do
-      subject.source_branch = "master"
-      subject.target_branch = "feature"
+      subject.update(source_branch: 'master', target_branch: 'feature')
 
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
     it "is unable to remove the source branch for a project the user cannot push to" do
+      user2 = create(:user)
+
       expect(subject.can_remove_source_branch?(user2)).to be_falsey
     end
 
@@ -843,6 +844,7 @@ describe MergeRequest do
     end
 
     it "cannot be removed if the last commit is not also the head of the source branch" do
+      subject.clear_memoized_shas
       subject.source_branch = "lfs"
 
       expect(subject.can_remove_source_branch?(user)).to be_falsey
@@ -942,7 +944,7 @@ describe MergeRequest do
 
       before do
         project.repository.raw_repository.delete_branch(subject.target_branch)
-        subject.reload
+        subject.clear_memoized_shas
       end
 
       it 'does not crash' do
@@ -1633,6 +1635,16 @@ describe MergeRequest do
 
       subject.reload_diff
     end
+
+    context 'when using the after_update hook to update' do
+      context 'when the branches are updated' do
+        it 'uses the new heads to generate the diff' do
+          expect { subject.update!(source_branch: subject.target_branch, target_branch: subject.source_branch) }
+            .to change { subject.merge_request_diff.start_commit_sha }
+            .and change { subject.merge_request_diff.head_commit_sha }
+        end
+      end
+    end
   end
 
   describe '#update_diff_discussion_positions' do
@@ -1895,6 +1907,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
         subject.project.repository.rm_branch(subject.author, subject.target_branch)
+        subject.clear_memoized_shas
       end
 
       it 'returns nil' do

spec/services/merge_requests/merge_service_spec.rb

 require 'spec_helper'
 
 describe MergeRequests::MergeService do
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
+  set(:user) { create(:user) }
+  set(:user2) { create(:user) }
   let(:merge_request) { create(:merge_request, :simple, author: user2, assignee: user2) }
   let(:project) { merge_request.project }
 
   before do
-    project.team << [user, :master]
-    project.team << [user2, :developer]
+    project.add_master(user)
+    project.add_developer(user2)
   end
 
   describe '#execute' do