Improve performance of DiffDiscussion#truncated_diff_lines and DiffNote#diff_line by removing expensive diff position calculation and comparison
This commit is contained in:
parent
92e15071c1
commit
771bf9527f
|
@ -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
|
||||
|
||||
|
|
|
@ -14,10 +14,6 @@ module NoteOnDiff
|
|||
raise NotImplementedError
|
||||
end
|
||||
|
||||
def for_line?(line)
|
||||
raise NotImplementedError
|
||||
end
|
||||
|
||||
def original_line_code
|
||||
raise NotImplementedError
|
||||
end
|
||||
|
|
|
@ -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?
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Improve performance of MR discussions on large diffs
|
||||
merge_request:
|
||||
author:
|
||||
type: performance
|
|
@ -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)
|
||||
|
|
|
@ -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) }
|
||||
|
|
Loading…
Reference in New Issue