Speedup DiffNote#active? on discussions, preloading noteables and avoid touching git repository to return diff_refs when possible14202-speed-up-diff-note-active

- Preloading noteable we share the same noteable instance when more than one 
discussion refers to the same noteable.
- Any other call to that object that is cached in that object will be for any 
discussion.
- In those cases where merge_request_diff has all the sha stored to build a diff_refs get that 
diff_refs using directly those sha instead accessing to the git repository to first get the 
commits and later the sha.

8 files changed, 63 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 39b77460deb..890adeba915 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -36,6 +36,7 @@ v 8.11.0 (unreleased)
   - Multiple trigger variables show in separate lines (Katarzyna Kobierska Ula Budziszewska)
   - Profile requests when a header is passed
   - Avoid calculation of line_code and position for _line partial when showing diff notes on discussion tab.
+  - Speedup DiffNote#active? on discussions, preloading noteables and avoid touching git repository to return diff_refs when possible
   - Add commit stats in commit api. !5517 (dixpac)
   - Make error pages responsive (Takuya Noguchi)
   - Change requests_profiles resource constraint to catch virtually any file
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 03166294ddd..90d232fea4b 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -378,6 +378,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       fresh.
       discussions
 
+    preload_noteable_for_not_commit_notes(@discussions.flat_map(&:notes))
+
     # This is not executed lazily
     @notes = Banzai::NoteRenderer.render(
       @discussions.flat_map(&:notes),
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 0c47abe0fba..491ff254033 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -92,6 +92,10 @@ module NotesHelper
     project.team.max_member_access_for_user_ids(user_ids)
   end
 
+  def preload_noteable_for_not_commit_notes(notes)
+    ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable)
+  end
+
   def note_max_access_for_user(note)
     note.project.team.human_max_access(note.author_id)
   end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 9671955db36..2ea770285a0 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -67,7 +67,7 @@ class DiffNote < Note
     return false unless supported?
     return true if for_commit?
 
-    diff_refs ||= self.noteable.diff_refs
+    diff_refs ||= noteable_diff_refs
 
     self.position.diff_refs == diff_refs
   end
@@ -78,6 +78,14 @@ class DiffNote < Note
     !self.for_merge_request? || self.noteable.support_new_diff_notes?
   end
 
+  def noteable_diff_refs
+    if noteable.method(:diff_refs).arity > 0
+      noteable.diff_refs(only_sha: true)
+    else
+      noteable.diff_refs
+    end
+  end
+
   def set_original_position
     self.original_position = self.position.dup
   end
@@ -96,7 +104,7 @@ class DiffNote < Note
       self.project,
       nil,
       old_diff_refs: self.position.diff_refs,
-      new_diff_refs: self.noteable.diff_refs,
+      new_diff_refs: noteable_diff_refs,
       paths: self.position.paths
     ).execute(self)
   end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 74facfd1c9c..e2218a5f02b 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -49,6 +49,12 @@ class Discussion
     self.noteable == target && !diff_discussion?
   end
 
+  def active?
+    return @active if defined?(@active)
+
+    @active = first_note.active?
+  end
+
   def expanded?
     !diff_discussion? || active?
   end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f1b9c1d6feb..41b104fff6f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -245,7 +245,16 @@ class MergeRequest < ActiveRecord::Base
     @source_branch_sha || source_branch_head.try(:sha)
   end
 
-  def diff_refs
+  def diff_refs(only_sha: false)
+    # This way we avoid to touch the git repository
+    if only_sha && merge_request_diff && merge_request_diff.diff_refs_by_sha?
+      return Gitlab::Diff::DiffRefs.new(
+        base_sha:  merge_request_diff.base_commit_sha,
+        start_sha: merge_request_diff.start_commit_sha,
+        head_sha:  merge_request_diff.head_commit_sha
+      )
+    end
+
     return unless diff_start_commit || diff_base_commit
 
     Gitlab::Diff::DiffRefs.new(
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index 3f520c8f3ff..119266f2d2c 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -82,6 +82,10 @@ class MergeRequestDiff < ActiveRecord::Base
     project.commit(self.head_commit_sha)
   end
 
+  def diff_refs_by_sha?
+    base_commit_sha? && head_commit_sha? && start_commit_sha?
+  end
+
   def compare
     @compare ||=
       begin
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a0e3c26e542..f368c7a8ea4 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -686,4 +686,30 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
   end
+
+  describe "#diff_refs" do
+    context "with diffs" do
+      subject { create(:merge_request, :with_diffs) }
+
+      context "when asking for only_sha diff_refs" do
+        it "returns diff_refs without touching the repository" do
+          subject # Instantiate the object
+
+          expect_any_instance_of(Repository).not_to receive(:commit)
+
+          subject.diff_refs(only_sha: true)
+        end
+
+        it "returns expected diff_refs" do
+          expected_diff_refs = Gitlab::Diff::DiffRefs.new(
+            base_sha:  subject.merge_request_diff.base_commit_sha,
+            start_sha: subject.merge_request_diff.start_commit_sha,
+            head_sha:  subject.merge_request_diff.head_commit_sha
+          )
+
+          expect(subject.diff_refs(only_sha: true)).to eq(expected_diff_refs)
+        end
+      end
+    end
+  end
 end