Commit e1780825 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

Merge branch 'finding-issues-by-labels-performance' into 'master'

Improve performance of finding issues with/without labels

The changes in this MR ultimately lead to finding issues with(out) labels being about 2x faster due to:

1. Newly added indexes on `issues.state` and `projects.visibility_level`
2. Adjusting the query so that finding issues for multiple projects is more efficient

See merge request !1787
parents 56476f18 094e1cc0
...@@ -6,6 +6,7 @@ v 8.2.0 ...@@ -6,6 +6,7 @@ v 8.2.0
- Improved performance of finding projects and groups in various places - Improved performance of finding projects and groups in various places
- Improved performance of rendering user profile pages and Atom feeds - Improved performance of rendering user profile pages and Atom feeds
- Fix grouping of contributors by email in graph. - Fix grouping of contributors by email in graph.
- Improved performance of finding issues with/without labels
- Remove CSS property preventing hard tabs from rendering in Chromium 45 (Stan Hu) - Remove CSS property preventing hard tabs from rendering in Chromium 45 (Stan Hu)
- Fix Drone CI service template not saving properly (Stan Hu) - Fix Drone CI service template not saving properly (Stan Hu)
- Fix avatars not showing in Atom feeds and project issues when Gravatar disabled (Stan Hu) - Fix avatars not showing in Atom feeds and project issues when Gravatar disabled (Stan Hu)
......
...@@ -77,11 +77,11 @@ class IssuableFinder ...@@ -77,11 +77,11 @@ class IssuableFinder
return @projects if defined?(@projects) return @projects if defined?(@projects)
if project? if project?
project @projects = project
elsif current_user && params[:authorized_only].presence && !current_user_related? elsif current_user && params[:authorized_only].presence && !current_user_related?
current_user.authorized_projects @projects = current_user.authorized_projects
else else
ProjectsFinder.new.execute(current_user) @projects = ProjectsFinder.new.execute(current_user)
end end
end end
...@@ -190,8 +190,10 @@ class IssuableFinder ...@@ -190,8 +190,10 @@ class IssuableFinder
def by_project(items) def by_project(items)
items = items =
if projects if project?
items.of_projects(projects).references(:project) items.of_projects(projects).references_project
elsif projects
items.merge(projects.reorder(nil)).join_project
else else
items.none items.none
end end
...@@ -206,7 +208,9 @@ class IssuableFinder ...@@ -206,7 +208,9 @@ class IssuableFinder
end end
def sort(items) def sort(items)
items.sort(params[:sort]) # Ensure we always have an explicit sort order (instead of inheriting
# multiple orders when combining ActiveRecord::Relation objects).
params[:sort] ? items.sort(params[:sort]) : items.reorder(id: :desc)
end end
def by_assignee(items) def by_assignee(items)
......
...@@ -35,6 +35,9 @@ module Issuable ...@@ -35,6 +35,9 @@ module Issuable
scope :order_milestone_due_desc, -> { joins(:milestone).reorder('milestones.due_date DESC, milestones.id DESC') } scope :order_milestone_due_desc, -> { joins(:milestone).reorder('milestones.due_date DESC, milestones.id DESC') }
scope :order_milestone_due_asc, -> { joins(:milestone).reorder('milestones.due_date ASC, milestones.id ASC') } scope :order_milestone_due_asc, -> { joins(:milestone).reorder('milestones.due_date ASC, milestones.id ASC') }
scope :join_project, -> { joins(:project) }
scope :references_project, -> { references(:project) }
delegate :name, delegate :name,
:email, :email,
to: :author, to: :author,
......
...@@ -134,6 +134,9 @@ class MergeRequest < ActiveRecord::Base ...@@ -134,6 +134,9 @@ class MergeRequest < ActiveRecord::Base
scope :closed, -> { with_state(:closed) } scope :closed, -> { with_state(:closed) }
scope :closed_and_merged, -> { with_states(:closed, :merged) } scope :closed_and_merged, -> { with_states(:closed, :merged) }
scope :join_project, -> { joins(:target_project) }
scope :references_project, -> { references(:target_project) }
def self.reference_prefix def self.reference_prefix
'!' '!'
end end
......
class AddIssuesStateIndex < ActiveRecord::Migration
def change
add_index :issues, :state
end
end
class AddProjectsVisibilityLevelIndex < ActiveRecord::Migration
def change
add_index :projects, :visibility_level
end
end
...@@ -384,6 +384,7 @@ ActiveRecord::Schema.define(version: 20151118162244) do ...@@ -384,6 +384,7 @@ ActiveRecord::Schema.define(version: 20151118162244) do
add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
add_index "issues", ["project_id"], name: "index_issues_on_project_id", using: :btree add_index "issues", ["project_id"], name: "index_issues_on_project_id", using: :btree
add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
add_index "issues", ["title"], name: "index_issues_on_title", using: :btree add_index "issues", ["title"], name: "index_issues_on_title", using: :btree
create_table "keys", force: true do |t| create_table "keys", force: true do |t|
...@@ -642,8 +643,6 @@ ActiveRecord::Schema.define(version: 20151118162244) do ...@@ -642,8 +643,6 @@ ActiveRecord::Schema.define(version: 20151118162244) do
t.string "import_type" t.string "import_type"
t.string "import_source" t.string "import_source"
t.integer "commit_count", default: 0 t.integer "commit_count", default: 0
t.boolean "merge_requests_ff_only_enabled", default: false
t.text "issues_template"
t.text "import_error" t.text "import_error"
end end
...@@ -653,6 +652,7 @@ ActiveRecord::Schema.define(version: 20151118162244) do ...@@ -653,6 +652,7 @@ ActiveRecord::Schema.define(version: 20151118162244) do
add_index "projects", ["namespace_id"], name: "index_projects_on_namespace_id", using: :btree add_index "projects", ["namespace_id"], name: "index_projects_on_namespace_id", using: :btree
add_index "projects", ["path"], name: "index_projects_on_path", using: :btree add_index "projects", ["path"], name: "index_projects_on_path", using: :btree
add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree
add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
create_table "protected_branches", force: true do |t| create_table "protected_branches", force: true do |t|
t.integer "project_id", null: false t.integer "project_id", null: false
......
require 'spec_helper'
describe IssuesFinder, benchmark: true do
describe '#execute' do
let(:user) { create(:user) }
let(:project) { create(:project, :public) }
let(:label1) { create(:label, project: project, title: 'A') }
let(:label2) { create(:label, project: project, title: 'B') }
before do
10.times do |n|
issue = create(:issue, author: user, project: project)
if n > 4
create(:label_link, label: label1, target: issue)
create(:label_link, label: label2, target: issue)
end
end
end
describe 'retrieving issues without labels' do
let(:finder) do
IssuesFinder.new(user, scope: 'all', label_name: Label::None.title,
state: 'opened')
end
benchmark_subject { finder.execute }
it { is_expected.to iterate_per_second(2000) }
end
describe 'retrieving issues with labels' do
let(:finder) do
IssuesFinder.new(user, scope: 'all', label_name: label1.title,
state: 'opened')
end
benchmark_subject { finder.execute }
it { is_expected.to iterate_per_second(1000) }
end
describe 'retrieving issues for a single project' do
let(:finder) do
IssuesFinder.new(user, scope: 'all', label_name: Label::None.title,
state: 'opened', project_id: project.id)
end
benchmark_subject { finder.execute }
it { is_expected.to iterate_per_second(2000) }
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