Improve performance of DiffDiscussion#truncated_diff_lines and DiffNote#diff_line by removing expensive diff position calculation and comparison

7 files changed, 14 insertions, 37 deletions

diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 4b4d519f3df..db9770fabf4 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -9,7 +9,6 @@ module DiscussionOnDiff
               :original_line_code,
               :diff_file,
               :diff_line,
-              :for_line?,
               :active?,
               :created_at_diff?,
 
@@ -39,17 +38,16 @@ module DiscussionOnDiff
   # Returns an array of at most 16 highlighted lines above a diff note
   def truncated_diff_lines(highlight: true)
     lines = highlight ? highlighted_diff_lines : diff_lines
+
+    initial_line_index = [diff_line.index - NUMBER_OF_TRUNCATED_DIFF_LINES + 1, 0].max
+
     prev_lines = []
 
-    lines.each do |line|
+    lines[initial_line_index..diff_line.index].each do |line|
       if line.meta?
         prev_lines.clear
       else
         prev_lines << line
-
-        break if for_line?(line)
-
-        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
       end
     end
 
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index f734952fa6c..510b8868462 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -14,10 +14,6 @@ module NoteOnDiff
     raise NotImplementedError
   end
 
-  def for_line?(line)
-    raise NotImplementedError
-  end
-
   def original_line_code
     raise NotImplementedError
   end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index b53d44cda95..15122cbc693 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -21,7 +21,7 @@ class DiffNote < Note
 
   before_validation :set_original_position, on: :create
   before_validation :update_position, on: :create, if: :on_text?
-  before_validation :set_line_code
+  before_validation :set_line_code, if: :on_text?
   after_save :keep_around_commits
 
   def discussion_class(*)
@@ -61,10 +61,6 @@ class DiffNote < Note
     @diff_line ||= diff_file&.line_for_position(self.original_position)
   end
 
-  def for_line?(line)
-    diff_file.position(line) == self.original_position
-  end
-
   def original_line_code
     return unless on_text?
 
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index c36be956ff0..d90cafd14b4 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -38,11 +38,7 @@ class LegacyDiffNote < Note
   end
 
   def diff_line
-    @diff_line ||= diff_file.line_for_line_code(self.line_code) if diff_file
-  end
-
-  def for_line?(line)
-    line.discussable? && diff_file.line_code(line) == self.line_code
+    @diff_line ||= diff_file&.line_for_line_code(self.line_code)
   end
 
   def original_line_code
diff --git a/changelogs/unreleased/dm-diff-note-for-line-performance.yml b/changelogs/unreleased/dm-diff-note-for-line-performance.yml
new file mode 100644
index 00000000000..cbc418ab103
--- /dev/null
+++ b/changelogs/unreleased/dm-diff-note-for-line-performance.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance of MR discussions on large diffs
+merge_request:
+author:
+type: performance
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index d0cfe2386ca..cd490aaa291 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -61,7 +61,9 @@ module Gitlab
       end
 
       def line_for_position(pos)
-        diff_lines.find { |line| position(line) == pos }
+        return nil unless pos.position_type == 'text'
+
+        diff_lines.find { |line| line.old_line == pos.old_line && line.new_line == pos.new_line }
       end
 
       def position_for_line_code(code)
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 4d0b3245a13..2705421e540 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -112,22 +112,6 @@ describe DiffNote do
     end
   end
 
-  describe "#for_line?" do
-    context "when provided the correct diff line" do
-      it "returns true" do
-        expect(subject.for_line?(subject.diff_line)).to be true
-      end
-    end
-
-    context "when provided a different diff line" do
-      it "returns false" do
-        some_line = subject.diff_file.diff_lines.first
-
-        expect(subject.for_line?(some_line)).to be false
-      end
-    end
-  end
-
   describe "#active?" do
     context "when noteable is a commit" do
       subject { build(:diff_note_on_commit, project: project, position: position) }