Commit 8716ff7f authored by Paco Guzman's avatar Paco Guzman

Speedup DiffNote#active? on discussions, preloading noteables and avoid...

Speedup DiffNote#active? on discussions, preloading noteables and avoid touching git repository to return diff_refs when possible

- 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.
parent aac8dcf1
...@@ -40,6 +40,7 @@ v 8.11.0 (unreleased) ...@@ -40,6 +40,7 @@ v 8.11.0 (unreleased)
- Multiple trigger variables show in separate lines (Katarzyna Kobierska Ula Budziszewska) - Multiple trigger variables show in separate lines (Katarzyna Kobierska Ula Budziszewska)
- Profile requests when a header is passed - Profile requests when a header is passed
- Avoid calculation of line_code and position for _line partial when showing diff notes on discussion tab. - 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) - Add commit stats in commit api. !5517 (dixpac)
- Make error pages responsive (Takuya Noguchi) - Make error pages responsive (Takuya Noguchi)
- Change requests_profiles resource constraint to catch virtually any file - Change requests_profiles resource constraint to catch virtually any file
......
...@@ -378,6 +378,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -378,6 +378,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
fresh. fresh.
discussions discussions
preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
# This is not executed lazily # This is not executed lazily
@notes = Banzai::NoteRenderer.render( @notes = Banzai::NoteRenderer.render(
@discussions.flat_map(&:notes), @discussions.flat_map(&:notes),
......
...@@ -92,6 +92,10 @@ module NotesHelper ...@@ -92,6 +92,10 @@ module NotesHelper
project.team.max_member_access_for_user_ids(user_ids) project.team.max_member_access_for_user_ids(user_ids)
end end
def preload_noteable_for_regular_notes(notes)
ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable)
end
def note_max_access_for_user(note) def note_max_access_for_user(note)
note.project.team.human_max_access(note.author_id) note.project.team.human_max_access(note.author_id)
end end
......
...@@ -67,7 +67,7 @@ class DiffNote < Note ...@@ -67,7 +67,7 @@ class DiffNote < Note
return false unless supported? return false unless supported?
return true if for_commit? return true if for_commit?
diff_refs ||= self.noteable.diff_refs diff_refs ||= noteable_diff_refs
self.position.diff_refs == diff_refs self.position.diff_refs == diff_refs
end end
...@@ -78,6 +78,14 @@ class DiffNote < Note ...@@ -78,6 +78,14 @@ class DiffNote < Note
!self.for_merge_request? || self.noteable.support_new_diff_notes? !self.for_merge_request? || self.noteable.support_new_diff_notes?
end end
def noteable_diff_refs
if noteable.respond_to?(:diff_sha_refs)
noteable.diff_sha_refs
else
noteable.diff_refs
end
end
def set_original_position def set_original_position
self.original_position = self.position.dup self.original_position = self.position.dup
end end
...@@ -96,7 +104,7 @@ class DiffNote < Note ...@@ -96,7 +104,7 @@ class DiffNote < Note
self.project, self.project,
nil, nil,
old_diff_refs: self.position.diff_refs, old_diff_refs: self.position.diff_refs,
new_diff_refs: self.noteable.diff_refs, new_diff_refs: noteable_diff_refs,
paths: self.position.paths paths: self.position.paths
).execute(self) ).execute(self)
end end
......
...@@ -49,6 +49,12 @@ class Discussion ...@@ -49,6 +49,12 @@ class Discussion
self.noteable == target && !diff_discussion? self.noteable == target && !diff_discussion?
end end
def active?
return @active if defined?(@active)
@active = first_note.active?
end
def expanded? def expanded?
!diff_discussion? || active? !diff_discussion? || active?
end end
......
...@@ -255,6 +255,19 @@ class MergeRequest < ActiveRecord::Base ...@@ -255,6 +255,19 @@ class MergeRequest < ActiveRecord::Base
) )
end end
# Return diff_refs instance trying to not touch the git repository
def diff_sha_refs
if 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
)
else
diff_refs
end
end
def validate_branches def validate_branches
if target_project == source_project && target_branch == source_branch if target_project == source_project && target_branch == source_branch
errors.add :branch_conflict, "You can not use same project/branch for source and target" errors.add :branch_conflict, "You can not use same project/branch for source and target"
...@@ -659,7 +672,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -659,7 +672,7 @@ class MergeRequest < ActiveRecord::Base
end end
def support_new_diff_notes? def support_new_diff_notes?
diff_refs && diff_refs.complete? diff_sha_refs && diff_sha_refs.complete?
end end
def update_diff_notes_positions(old_diff_refs:, new_diff_refs:) def update_diff_notes_positions(old_diff_refs:, new_diff_refs:)
......
...@@ -82,6 +82,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -82,6 +82,10 @@ class MergeRequestDiff < ActiveRecord::Base
project.commit(self.head_commit_sha) project.commit(self.head_commit_sha)
end end
def diff_refs_by_sha?
base_commit_sha? && head_commit_sha? && start_commit_sha?
end
def compare def compare
@compare ||= @compare ||=
begin begin
......
...@@ -119,7 +119,7 @@ describe DiffNote, models: true do ...@@ -119,7 +119,7 @@ describe DiffNote, models: true do
context "when the merge request's diff refs don't match that of the diff note" do context "when the merge request's diff refs don't match that of the diff note" do
before do before do
allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs) allow(subject.noteable).to receive(:diff_sha_refs).and_return(commit.diff_refs)
end end
it "returns false" do it "returns false" do
...@@ -168,7 +168,7 @@ describe DiffNote, models: true do ...@@ -168,7 +168,7 @@ describe DiffNote, models: true do
context "when the note is outdated" do context "when the note is outdated" do
before do before do
allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs) allow(merge_request).to receive(:diff_sha_refs).and_return(commit.diff_refs)
end end
it "uses the DiffPositionUpdateService" do it "uses the DiffPositionUpdateService" do
......
...@@ -686,4 +686,28 @@ describe MergeRequest, models: true do ...@@ -686,4 +686,28 @@ describe MergeRequest, models: true do
subject.reload_diff subject.reload_diff
end end
end end
describe "#diff_sha_refs" do
context "with diffs" do
subject { create(:merge_request, :with_diffs) }
it "does not touch the repository" do
subject # Instantiate the object
expect_any_instance_of(Repository).not_to receive(:commit)
subject.diff_sha_refs
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_sha_refs).to eq(expected_diff_refs)
end
end
end
end end
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