diff options
author | Rémy Coutable <remy@rymai.me> | 2017-11-08 16:04:15 +0000 |
---|---|---|
committer | Winnie Hellmann <winnie@gitlab.com> | 2017-11-09 23:13:33 +0100 |
commit | ac5815dc3b34ec1de3e91dfdb7828ee1877484ad (patch) | |
tree | dd9da0f599dd7a9b96097af281ef39c116e6e572 | |
parent | e438e5c593d46154216550825d261a9173641f36 (diff) | |
download | gitlab-ce-ac5815dc3b34ec1de3e91dfdb7828ee1877484ad.tar.gz |
Merge branch 'dm-notes-for-commit-id' into 'master'
Use Commit#notes and Note.for_commit_id when possible to make sure we use all indexes available to us
Closes #34509
See merge request gitlab-org/gitlab-ce!15253
(cherry picked from commit bf13746fd8c7af1ef2e28e8a5a46cf0229b31eb8)
-rw-r--r-- | app/controllers/projects/commits_controller.rb | 3 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests/creations_controller.rb | 3 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 2 | ||||
-rw-r--r-- | app/models/ci/pipeline.rb | 2 | ||||
-rw-r--r-- | app/models/external_issue.rb | 4 | ||||
-rw-r--r-- | app/models/merge_request.rb | 2 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 12 | ||||
-rw-r--r-- | app/views/projects/commits/_commit.html.haml | 7 | ||||
-rw-r--r-- | changelogs/unreleased/dm-notes-for-commit-id.yml | 6 | ||||
-rw-r--r-- | lib/api/commits.rb | 2 | ||||
-rw-r--r-- | lib/api/v3/commits.rb | 2 |
11 files changed, 16 insertions, 29 deletions
diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index d48284a4429..28920877635 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -10,9 +10,6 @@ class Projects::CommitsController < Projects::ApplicationController before_action :set_commits def show - @note_counts = project.notes.where(commit_id: @commits.map(&:id)) - .group(:commit_id).count - @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref) diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 129682f64aa..764a9c7111e 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -110,9 +110,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @commits = prepare_commits_for_rendering(@merge_request.commits) @commit = @merge_request.diff_head_commit - @note_counts = Note.where(commit_id: @commits.map(&:id)) - .group(:commit_id).count - @labels = LabelsFinder.new(current_user, project_id: @project.id).execute set_pipeline_variables diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 402420b851e..22de6680511 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -81,8 +81,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Get commits from repository # or from cache if already merged @commits = prepare_commits_for_rendering(@merge_request.commits) - @note_counts = Note.where(commit_id: @commits.map(&:id)) - .group(:commit_id).count render json: { html: view_to_html_string('projects/merge_requests/_commits') } end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index ca65e81f27a..fcbe3d2b67b 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -409,7 +409,7 @@ module Ci end def notes - Note.for_commit_id(sha) + project.notes.for_commit_id(sha) end def process! diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb index 0bf18e529f0..9ff56f229bc 100644 --- a/app/models/external_issue.rb +++ b/app/models/external_issue.rb @@ -47,4 +47,8 @@ class ExternalIssue id end + + def notes + Note.none + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 82d0ae90d77..efd8cca2947 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -576,7 +576,7 @@ class MergeRequest < ActiveRecord::Base commit_notes = Note .except(:order) .where(project_id: [source_project_id, target_project_id]) - .where(noteable_type: 'Commit', commit_id: commit_ids) + .for_commit_id(commit_ids) # We're using a UNION ALL here since this results in better performance # compared to using OR statements. We're using UNION ALL since the queries diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 69bd19c1977..e946218824c 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -481,17 +481,7 @@ module SystemNoteService # # Returns Boolean def cross_reference_exists?(noteable, mentioner) - # Initial scope should be system notes of this noteable type - notes = Note.system.where(noteable_type: noteable.class) - - notes = - if noteable.is_a?(Commit) - # Commits have non-integer IDs, so they're stored in `commit_id` - notes.where(commit_id: noteable.id) - else - notes.where(noteable_id: noteable.id) - end - + notes = noteable.notes.system notes_for_mentioner(mentioner, noteable, notes).exists? end diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index a16ffb433a5..a66177f20e9 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -1,11 +1,6 @@ - ref = local_assigns.fetch(:ref) -- if @note_counts - - note_count = @note_counts.fetch(commit.id, 0) -- else - - notes = commit.notes - - note_count = notes.user.count -- cache_key = [project.full_path, commit.id, current_application_settings, note_count, @path.presence, current_controller?(:commits), I18n.locale] +- cache_key = [project.full_path, commit.id, current_application_settings, @path.presence, current_controller?(:commits), I18n.locale] - cache_key.push(commit.status(ref)) if commit.status(ref) = cache(cache_key, expires_in: 1.day) do diff --git a/changelogs/unreleased/dm-notes-for-commit-id.yml b/changelogs/unreleased/dm-notes-for-commit-id.yml new file mode 100644 index 00000000000..5b83332d82f --- /dev/null +++ b/changelogs/unreleased/dm-notes-for-commit-id.yml @@ -0,0 +1,6 @@ +--- +title: Improve performance of commits list by fully using DB index when getting commit + note counts +merge_request: +author: +type: performance diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 2685dc27252..2bc4039b019 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -117,7 +117,7 @@ module API commit = user_project.commit(params[:sha]) not_found! 'Commit' unless commit - notes = user_project.notes.where(commit_id: commit.id).order(:created_at) + notes = commit.notes.order(:created_at) present paginate(notes), with: Entities::CommitNote end diff --git a/lib/api/v3/commits.rb b/lib/api/v3/commits.rb index ed206a6def0..be360fbfc0c 100644 --- a/lib/api/v3/commits.rb +++ b/lib/api/v3/commits.rb @@ -106,7 +106,7 @@ module API commit = user_project.commit(params[:sha]) not_found! 'Commit' unless commit - notes = Note.where(commit_id: commit.id).order(:created_at) + notes = commit.notes.order(:created_at) present paginate(notes), with: ::API::Entities::CommitNote end |