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

Use memoization for commits on diffs

See merge request gitlab-org/gitlab-ce!15857

app/models/merge_request.rb

@@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base
   include ManualInverseAssociation
   include EachBatch
   include ThrottledTouch
+  include Gitlab::Utils::StrongMemoize
 
   ignore_column :locked_at,
                 :ref_fetched
@@ -53,6 +54,7 @@ class MergeRequest < ActiveRecord::Base
 
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
+  after_update :clear_memoized_shas
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
@@ -387,14 +389,18 @@ 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
+    strong_memoize(:target_branch_head) do
       target_project.repository.commit(target_branch_ref)
     end
+  end
 
   def branch_merge_base_commit
     start_sha = target_branch_sha
@@ -525,6 +531,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
@@ -733,7 +734,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
@@ -1468,6 +1469,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