Commit 255e64ef authored by Robert Speicher's avatar Robert Speicher

Merge branch 'dm-diff-note-for-line-performance' into 'master'

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

Closes #41406

See merge request gitlab-org/gitlab-ce!16111
parents 8a488a02 771bf952
...@@ -9,7 +9,6 @@ module DiscussionOnDiff ...@@ -9,7 +9,6 @@ module DiscussionOnDiff
:original_line_code, :original_line_code,
:diff_file, :diff_file,
:diff_line, :diff_line,
:for_line?,
:active?, :active?,
:created_at_diff?, :created_at_diff?,
...@@ -39,17 +38,16 @@ module DiscussionOnDiff ...@@ -39,17 +38,16 @@ module DiscussionOnDiff
# Returns an array of at most 16 highlighted lines above a diff note # Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true) def truncated_diff_lines(highlight: true)
lines = highlight ? highlighted_diff_lines : diff_lines lines = highlight ? highlighted_diff_lines : diff_lines
initial_line_index = [diff_line.index - NUMBER_OF_TRUNCATED_DIFF_LINES + 1, 0].max
prev_lines = [] prev_lines = []
lines.each do |line| lines[initial_line_index..diff_line.index].each do |line|
if line.meta? if line.meta?
prev_lines.clear prev_lines.clear
else else
prev_lines << line prev_lines << line
break if for_line?(line)
prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
end end
end end
......
...@@ -14,10 +14,6 @@ module NoteOnDiff ...@@ -14,10 +14,6 @@ module NoteOnDiff
raise NotImplementedError raise NotImplementedError
end end
def for_line?(line)
raise NotImplementedError
end
def original_line_code def original_line_code
raise NotImplementedError raise NotImplementedError
end end
......
...@@ -21,7 +21,7 @@ class DiffNote < Note ...@@ -21,7 +21,7 @@ class DiffNote < Note
before_validation :set_original_position, on: :create before_validation :set_original_position, on: :create
before_validation :update_position, on: :create, if: :on_text? 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 after_save :keep_around_commits
def discussion_class(*) def discussion_class(*)
...@@ -61,10 +61,6 @@ class DiffNote < Note ...@@ -61,10 +61,6 @@ class DiffNote < Note
@diff_line ||= diff_file&.line_for_position(self.original_position) @diff_line ||= diff_file&.line_for_position(self.original_position)
end end
def for_line?(line)
diff_file.position(line) == self.original_position
end
def original_line_code def original_line_code
return unless on_text? return unless on_text?
......
...@@ -38,11 +38,7 @@ class LegacyDiffNote < Note ...@@ -38,11 +38,7 @@ class LegacyDiffNote < Note
end end
def diff_line def diff_line
@diff_line ||= diff_file.line_for_line_code(self.line_code) if diff_file @diff_line ||= diff_file&.line_for_line_code(self.line_code)
end
def for_line?(line)
line.discussable? && diff_file.line_code(line) == self.line_code
end end
def original_line_code def original_line_code
......
---
title: Improve performance of MR discussions on large diffs
merge_request:
author:
type: performance
...@@ -61,7 +61,9 @@ module Gitlab ...@@ -61,7 +61,9 @@ module Gitlab
end end
def line_for_position(pos) 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 end
def position_for_line_code(code) def position_for_line_code(code)
......
...@@ -112,22 +112,6 @@ describe DiffNote do ...@@ -112,22 +112,6 @@ describe DiffNote do
end end
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 describe "#active?" do
context "when noteable is a commit" do context "when noteable is a commit" do
subject { build(:diff_note_on_commit, project: project, position: position) } subject { build(:diff_note_on_commit, project: project, position: position) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment