Use memoization for commits on diffs

The Gitaly CommitService is being hammered by n + 1 calls, mostly when
finding commits. This leads to this gRPC being turned of on production:
https://gitlab.com/gitlab-org/gitaly/issues/514#note_48991378

Hunting down where it came from, most of them were due to
MergeRequest#show. To prove this, I set a script to request the
MergeRequest#show page 50 times. The GDK was being scraped by
Prometheus, where we have metrics on controller#action and their Gitaly
calls performed. On both occations I've restarted the full GDK so all
caches had to be rebuild.

Current master, 806a68a81f, needed 435 requests
After this commit, 154 requests

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,13 +389,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
@@ -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

@@ -0,0 +1,5 @@
+---
+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,17 @@ 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