diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-12-22 14:49:56 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2017-12-22 20:07:15 +0300 |
commit | 771bf9527ffd5fd8fe258381593f686d5d960a42 (patch) | |
tree | e15ff12e3419aa751e3d54e6af7823a88f7be930 | |
parent | 92e15071c13f65cf7250315f1a138284880b0074 (diff) |
Improve performance of DiffDiscussion#truncated_diff_lines and DiffNote#diff_line by removing expensive diff position calculation and comparison
-rw-r--r-- | app/models/concerns/discussion_on_diff.rb | 10 | ||||
-rw-r--r-- | app/models/concerns/note_on_diff.rb | 4 | ||||
-rw-r--r-- | app/models/diff_note.rb | 6 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 6 | ||||
-rw-r--r-- | changelogs/unreleased/dm-diff-note-for-line-performance.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/diff/file.rb | 4 | ||||
-rw-r--r-- | spec/models/diff_note_spec.rb | 16 |
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) } |