Use Commit#notes and Note.for_commit_id when possible to make sure we use all the indexes available to us

This commit is contained in:
Douwe Maan 2017-11-07 17:11:37 +01:00
parent dc1e6b4362
commit fec48c6e17
11 changed files with 16 additions and 29 deletions

View file

@ -10,9 +10,6 @@ class Projects::CommitsController < Projects::ApplicationController
before_action :set_commits before_action :set_commits
def show 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 @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) .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)

View file

@ -110,9 +110,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
@commits = prepare_commits_for_rendering(@merge_request.commits) @commits = prepare_commits_for_rendering(@merge_request.commits)
@commit = @merge_request.diff_head_commit @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 @labels = LabelsFinder.new(current_user, project_id: @project.id).execute
set_pipeline_variables set_pipeline_variables

View file

@ -102,8 +102,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
# Get commits from repository # Get commits from repository
# or from cache if already merged # or from cache if already merged
@commits = prepare_commits_for_rendering(@merge_request.commits) @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') } render json: { html: view_to_html_string('projects/merge_requests/_commits') }
end end

View file

@ -409,7 +409,7 @@ module Ci
end end
def notes def notes
Note.for_commit_id(sha) project.notes.for_commit_id(sha)
end end
def process! def process!

View file

@ -47,4 +47,8 @@ class ExternalIssue
id id
end end
def notes
Note.none
end
end end

View file

@ -578,7 +578,7 @@ class MergeRequest < ActiveRecord::Base
commit_notes = Note commit_notes = Note
.except(:order) .except(:order)
.where(project_id: [source_project_id, target_project_id]) .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 # 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 # compared to using OR statements. We're using UNION ALL since the queries

View file

@ -481,17 +481,7 @@ module SystemNoteService
# #
# Returns Boolean # Returns Boolean
def cross_reference_exists?(noteable, mentioner) def cross_reference_exists?(noteable, mentioner)
# Initial scope should be system notes of this noteable type notes = noteable.notes.system
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_for_mentioner(mentioner, noteable, notes).exists? notes_for_mentioner(mentioner, noteable, notes).exists?
end end

View file

@ -1,11 +1,6 @@
- ref = local_assigns.fetch(:ref) - 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_key.push(commit.status(ref)) if commit.status(ref)
= cache(cache_key, expires_in: 1.day) do = cache(cache_key, expires_in: 1.day) do

View file

@ -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

View file

@ -117,7 +117,7 @@ module API
commit = user_project.commit(params[:sha]) commit = user_project.commit(params[:sha])
not_found! 'Commit' unless commit 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 present paginate(notes), with: Entities::CommitNote
end end

View file

@ -106,7 +106,7 @@ module API
commit = user_project.commit(params[:sha]) commit = user_project.commit(params[:sha])
not_found! 'Commit' unless commit 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 present paginate(notes), with: ::API::Entities::CommitNote
end end