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

8 files changed, 87 insertions, 11 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 57059f944af..5e3e5b75dce 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -6,6 +6,7 @@ v 8.2.0
   - Improved performance of finding projects and groups in various places
   - Improved performance of rendering user profile pages and Atom feeds
   - 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)
   - 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)
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index c407dfc163a..3d5e8b6fbe7 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -62,10 +62,10 @@ class IssuableFinder
 
     if project?
       @project = Project.find(params[:project_id])
-      
+
       unless Ability.abilities.allowed?(current_user, :read_project, @project)
         @project = nil
-      end 
+      end
     else
       @project = nil
     end
@@ -77,11 +77,11 @@ class IssuableFinder
     return @projects if defined?(@projects)
 
     if project?
-      project
+      @projects = project
     elsif current_user && params[:authorized_only].presence && !current_user_related?
-      current_user.authorized_projects
+      @projects = current_user.authorized_projects
     else
-      ProjectsFinder.new.execute(current_user)
+      @projects = ProjectsFinder.new.execute(current_user)
     end
   end
 
@@ -190,8 +190,10 @@ class IssuableFinder
 
   def by_project(items)
     items =
-      if projects
-        items.of_projects(projects).references(:project)
+      if project?
+        items.of_projects(projects).references_project
+      elsif projects
+        items.merge(projects.reorder(nil)).join_project
       else
         items.none
       end
@@ -206,7 +208,9 @@ class IssuableFinder
   end
 
   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
 
   def by_assignee(items)
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 492a026add9..97d3cb18f4e 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -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_asc, -> { joins(:milestone).reorder('milestones.due_date ASC, milestones.id ASC') }
 
+    scope :join_project, -> { joins(:project) }
+    scope :references_project, -> { references(:project) }
+
     delegate :name,
              :email,
              to: :author,
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2eb03b8ba5b..1e8d9908f0a 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -134,6 +134,9 @@ class MergeRequest < ActiveRecord::Base
   scope :closed, -> { with_state(:closed) }
   scope :closed_and_merged, -> { with_states(:closed, :merged) }
 
+  scope :join_project, -> { joins(:target_project) }
+  scope :references_project, -> { references(:target_project) }
+
   def self.reference_prefix
     '!'
   end
diff --git a/db/migrate/20151109134526_add_issues_state_index.rb b/db/migrate/20151109134526_add_issues_state_index.rb
new file mode 100644
index 00000000000..1c4d2e30171
--- /dev/null
+++ b/db/migrate/20151109134526_add_issues_state_index.rb
@@ -0,0 +1,5 @@
+class AddIssuesStateIndex < ActiveRecord::Migration
+  def change
+    add_index :issues, :state
+  end
+end
diff --git a/db/migrate/20151109134916_add_projects_visibility_level_index.rb b/db/migrate/20151109134916_add_projects_visibility_level_index.rb
new file mode 100644
index 00000000000..600b4bafd98
--- /dev/null
+++ b/db/migrate/20151109134916_add_projects_visibility_level_index.rb
@@ -0,0 +1,5 @@
+class AddProjectsVisibilityLevelIndex < ActiveRecord::Migration
+  def change
+    add_index :projects, :visibility_level
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 462d5ed3b29..d687e68d098 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -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", ["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", ["state"], name: "index_issues_on_state", using: :btree
   add_index "issues", ["title"], name: "index_issues_on_title", using: :btree
 
   create_table "keys", force: true do |t|
@@ -641,9 +642,7 @@ ActiveRecord::Schema.define(version: 20151118162244) do
     t.integer  "star_count",             default: 0,        null: false
     t.string   "import_type"
     t.string   "import_source"
-    t.integer  "commit_count",                   default: 0
-    t.boolean  "merge_requests_ff_only_enabled", default: false
-    t.text     "issues_template"
+    t.integer  "commit_count",           default: 0
     t.text     "import_error"
   end
 
@@ -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", ["path"], name: "index_projects_on_path", 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|
     t.integer  "project_id",                          null: false
diff --git a/spec/benchmarks/finders/issues_finder_spec.rb b/spec/benchmarks/finders/issues_finder_spec.rb
new file mode 100644
index 00000000000..b57a33004a4
--- /dev/null
+++ b/spec/benchmarks/finders/issues_finder_spec.rb
@@ -0,0 +1,55 @@
+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