Merge remote-tracking branch 'public/trending-projects-performance'

7 files changed, 141 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 66b6e9fdf19..424dadccbbb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.1.0 (unreleased)
   - Add support for creating directories from Files page (Stan Hu)
   - Allow removing of project without confirmation when JavaScript is disabled (Stan Hu)
   - Support filtering by "Any" milestone or issue and fix "No Milestone" and "No Label" filters (Stan Hu)
+  - Improved performance of the trending projects page
   - Fix bug where transferring a project would result in stale commit links (Stan Hu)
   - Include full path of source and target branch names in New Merge Request page (Stan Hu)
   - Add user preference to view activities as default dashboard (Stan Hu)
diff --git a/app/finders/trending_projects_finder.rb b/app/finders/trending_projects_finder.rb
index 9ea342cb26d..81a12403801 100644
--- a/app/finders/trending_projects_finder.rb
+++ b/app/finders/trending_projects_finder.rb
@@ -1,13 +1,6 @@
 class TrendingProjectsFinder
-  def execute(current_user, start_date = nil)
-    start_date ||= Date.today - 1.month
-
-    projects = projects_for(current_user)
-
-    # Determine trending projects based on comments count
-    # for period of time - ex. month
-    projects.joins(:notes).where('notes.created_at > ?', start_date).
-      group("projects.id").reorder("count(notes.id) DESC")
+  def execute(current_user, start_date = 1.month.ago)
+    projects_for(current_user).trending(start_date)
   end
 
   private
diff --git a/app/models/project.rb b/app/models/project.rb
index bb47b9abb03..4661522b8a0 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -260,6 +260,20 @@ class Project < ActiveRecord::Base
       name_pattern = Gitlab::Regex::NAMESPACE_REGEX_STR
       %r{(?<project>#{name_pattern}/#{name_pattern})}
     end
+
+    def trending(since = 1.month.ago)
+      # By counting in the JOIN we don't expose the GROUP BY to the outer query.
+      # This means that calls such as "any?" and "count" just return a number of
+      # the total count, instead of the counts grouped per project as a Hash.
+      join_body = "INNER JOIN (
+        SELECT project_id, COUNT(*) AS amount
+        FROM notes
+        WHERE created_at >= #{sanitize(since)}
+        GROUP BY project_id
+      ) join_note_counts ON projects.id = join_note_counts.project_id"
+
+      joins(join_body).reorder('join_note_counts.amount DESC')
+    end
   end
 
   def team
diff --git a/spec/benchmarks/finders/trending_projects_finder_spec.rb b/spec/benchmarks/finders/trending_projects_finder_spec.rb
new file mode 100644
index 00000000000..551ce21840d
--- /dev/null
+++ b/spec/benchmarks/finders/trending_projects_finder_spec.rb
@@ -0,0 +1,14 @@
+require 'spec_helper'
+
+describe TrendingProjectsFinder, benchmark: true do
+  describe '#execute' do
+    let(:finder) { described_class.new }
+    let(:user)   { create(:user) }
+
+    # to_a is used to force actually running the query (instead of just building
+    # it).
+    benchmark_subject { finder.execute(user).non_archived.to_a }
+
+    it { is_expected.to iterate_per_second(500) }
+  end
+end
diff --git a/spec/benchmarks/models/project_spec.rb b/spec/benchmarks/models/project_spec.rb
new file mode 100644
index 00000000000..f1dd10440a9
--- /dev/null
+++ b/spec/benchmarks/models/project_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe Project, benchmark: true do
+  describe '.trending' do
+    let(:group)    { create(:group) }
+    let(:project1) { create(:empty_project, :public, group: group) }
+    let(:project2) { create(:empty_project, :public, group: group) }
+
+    let(:iterations) { 500 }
+
+    before do
+      2.times do
+        create(:note_on_commit, project: project1)
+      end
+
+      create(:note_on_commit, project: project2)
+    end
+
+    describe 'without an explicit start date' do
+      benchmark_subject { described_class.trending.to_a }
+
+      it { is_expected.to iterate_per_second(iterations) }
+    end
+
+    describe 'with an explicit start date' do
+      let(:date) { 1.month.ago }
+
+      benchmark_subject { described_class.trending(date).to_a }
+
+      it { is_expected.to iterate_per_second(iterations) }
+    end
+  end
+end
diff --git a/spec/finders/trending_projects_finder_spec.rb b/spec/finders/trending_projects_finder_spec.rb
new file mode 100644
index 00000000000..a49cbfd5160
--- /dev/null
+++ b/spec/finders/trending_projects_finder_spec.rb
@@ -0,0 +1,39 @@
+require 'spec_helper'
+
+describe TrendingProjectsFinder do
+  let(:user) { build(:user) }
+
+  describe '#execute' do
+    describe 'without an explicit start date' do
+      subject { described_class.new }
+
+      it 'returns the trending projects' do
+        relation = double(:ar_relation)
+
+        allow(subject).to receive(:projects_for)
+          .with(user)
+          .and_return(relation)
+
+        allow(relation).to receive(:trending)
+          .with(an_instance_of(ActiveSupport::TimeWithZone))
+      end
+    end
+
+    describe 'with an explicit start date' do
+      let(:date) { 2.months.ago }
+
+      subject { described_class.new }
+
+      it 'returns the trending projects' do
+        relation = double(:ar_relation)
+
+        allow(subject).to receive(:projects_for)
+          .with(user)
+          .and_return(relation)
+
+        allow(relation).to receive(:trending)
+          .with(date)
+      end
+    end
+  end
+end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 8b5d2c3a1c1..f93935ebe3b 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -423,4 +423,42 @@ describe Project do
     it { expect(project.gitlab_ci?).to be_truthy }
     it { expect(project.gitlab_ci_project).to be_a(Ci::Project) }
   end
+
+  describe '.trending' do
+    let(:group)    { create(:group) }
+    let(:project1) { create(:empty_project, :public, group: group) }
+    let(:project2) { create(:empty_project, :public, group: group) }
+
+    before do
+      2.times do
+        create(:note_on_commit, project: project1)
+      end
+
+      create(:note_on_commit, project: project2)
+    end
+
+    describe 'without an explicit start date' do
+      subject { described_class.trending.to_a }
+
+      it 'sorts Projects by the amount of notes in descending order' do
+        expect(subject).to eq([project1, project2])
+      end
+    end
+
+    describe 'with an explicit start date' do
+      let(:date) { 2.months.ago }
+
+      subject { described_class.trending(date).to_a }
+
+      before do
+        2.times do
+          create(:note_on_commit, project: project2, created_at: date)
+        end
+      end
+
+      it 'sorts Projects by the amount of notes in descending order' do
+        expect(subject).to eq([project2, project1])
+      end
+    end
+  end
 end