Cache the number of forks of a projectforks-count-cache

The number of forks of a project doesn't change very frequently and
running a COUNT(*) every time this information is requested can be quite
expensive. We also end up running such a COUNT(*) query at least twice
on the homepage of a project.

By caching this data and refreshing it when necessary we can reduce
project homepage loading times by around 60 milliseconds (based on the
timings of https://gitlab.com/gitlab-org/gitlab-ce).

14 files changed, 141 insertions, 1 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 7010664e1c8..92ca126bafc 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -196,7 +196,6 @@ class Project < ActiveRecord::Base
   accepts_nested_attributes_for :import_data
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
-  delegate :count, to: :forks, prefix: true
   delegate :members, to: :team, prefix: true
   delegate :add_user, :add_users, to: :team
   delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team
@@ -1398,6 +1397,10 @@ class Project < ActiveRecord::Base
   # @deprecated cannot remove yet because it has an index with its name in elasticsearch
   alias_method :path_with_namespace, :full_path
 
+  def forks_count
+    Projects::ForksCountService.new(self).count
+  end
+
   private
 
   def cross_namespace_reference?(from)
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 11ad4838471..54eb75ab9bf 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -128,6 +128,8 @@ module Projects
       project.repository.before_delete
 
       Repository.new(wiki_path, project, disk_path: repo_path).before_delete
+
+      Projects::ForksCountService.new(project).delete_cache
     end
   end
 end
diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb
index a2b23ea6171..ad67e68a86a 100644
--- a/app/services/projects/fork_service.rb
+++ b/app/services/projects/fork_service.rb
@@ -21,11 +21,17 @@ module Projects
       builds_access_level = @project.project_feature.builds_access_level
       new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
 
+      refresh_forks_count
+
       new_project
     end
 
     private
 
+    def refresh_forks_count
+      Projects::ForksCountService.new(@project).refresh_cache
+    end
+
     def allowed_visibility_level
       project_level = @project.visibility_level
 
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
new file mode 100644
index 00000000000..e2e2b1da91d
--- /dev/null
+++ b/app/services/projects/forks_count_service.rb
@@ -0,0 +1,30 @@
+module Projects
+  # Service class for getting and caching the number of forks of a project.
+  class ForksCountService
+    def initialize(project)
+      @project = project
+    end
+
+    def count
+      Rails.cache.fetch(cache_key) { uncached_count }
+    end
+
+    def refresh_cache
+      Rails.cache.write(cache_key, uncached_count)
+    end
+
+    def delete_cache
+      Rails.cache.delete(cache_key)
+    end
+
+    private
+
+    def uncached_count
+      @project.forks.count
+    end
+
+    def cache_key
+      ['projects', @project.id, 'forks_count']
+    end
+  end
+end
diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb
index f385e426827..f30b40423c8 100644
--- a/app/services/projects/unlink_fork_service.rb
+++ b/app/services/projects/unlink_fork_service.rb
@@ -13,7 +13,13 @@ module Projects
         ::MergeRequests::CloseService.new(@project, @current_user).execute(mr)
       end
 
+      refresh_forks_count(@project.forked_from_project)
+
       @project.forked_project_link.destroy
     end
+
+    def refresh_forks_count(project)
+      Projects::ForksCountService.new(project).refresh_cache
+    end
   end
 end
diff --git a/changelogs/unreleased/forks-count-cache.yml b/changelogs/unreleased/forks-count-cache.yml
new file mode 100644
index 00000000000..da8c53c2abd
--- /dev/null
+++ b/changelogs/unreleased/forks-count-cache.yml
@@ -0,0 +1,5 @@
+---
+title: Cache the number of forks of a project
+merge_request: 13535
+author:
+type: other
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 89dda88d3f5..15c3832b032 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -351,6 +351,8 @@ module API
 
         if user_project.forked_from_project.nil?
           user_project.create_forked_project_link(forked_to_project_id: user_project.id, forked_from_project_id: forked_from_project.id)
+
+          ::Projects::ForksCountService.new(forked_from_project).refresh_cache
         else
           render_api_error!("Project already forked", 409)
         end
diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb
index eb090453b48..449876c10d9 100644
--- a/lib/api/v3/projects.rb
+++ b/lib/api/v3/projects.rb
@@ -388,6 +388,8 @@ module API
 
           if user_project.forked_from_project.nil?
             user_project.create_forked_project_link(forked_to_project_id: user_project.id, forked_from_project_id: forked_from_project.id)
+
+            ::Projects::ForksCountService.new(forked_from_project).refresh_cache
           else
             render_api_error!("Project already forked", 409)
           end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d9ab44dc49f..eba71ba2f72 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2310,4 +2310,14 @@ describe Project do
       end
     end
   end
+
+  describe '#forks_count' do
+    it 'returns the number of forks' do
+      project = build(:project)
+
+      allow(project.forks).to receive(:count).and_return(1)
+
+      expect(project.forks_count).to eq(1)
+    end
+  end
 end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 6cb27d16fe5..a89a58ff713 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -1065,6 +1065,14 @@ describe API::Projects do
         expect(project_fork_target.forked?).to be_truthy
       end
 
+      it 'refreshes the forks count cachce' do
+        expect(project_fork_source.forks_count).to be_zero
+
+        post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
+
+        expect(project_fork_source.forks_count).to eq(1)
+      end
+
       it 'fails if forked_from project which does not exist' do
         post api("/projects/#{project_fork_target.id}/fork/9999", admin)
         expect(response).to have_http_status(404)
diff --git a/spec/requests/api/v3/projects_spec.rb b/spec/requests/api/v3/projects_spec.rb
index fca5b5b5d82..a514166274a 100644
--- a/spec/requests/api/v3/projects_spec.rb
+++ b/spec/requests/api/v3/projects_spec.rb
@@ -1004,6 +1004,14 @@ describe API::V3::Projects do
         expect(project_fork_target.forked?).to be_truthy
       end
 
+      it 'refreshes the forks count cachce' do
+        expect(project_fork_source.forks_count).to be_zero
+
+        post v3_api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
+
+        expect(project_fork_source.forks_count).to eq(1)
+      end
+
       it 'fails if forked_from project which does not exist' do
         post v3_api("/projects/#{project_fork_target.id}/fork/9999", admin)
         expect(response).to have_http_status(404)
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index c90536ba346..21c4b30734c 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -50,6 +50,14 @@ describe Projects::ForkService do
 
           expect(@from_project.avatar.file).to be_exists
         end
+
+        it 'flushes the forks count cache of the source project' do
+          expect(@from_project.forks_count).to be_zero
+
+          fork_project(@from_project, @to_user)
+
+          expect(@from_project.forks_count).to eq(1)
+        end
       end
     end
 
diff --git a/spec/services/projects/forks_count_service_spec.rb b/spec/services/projects/forks_count_service_spec.rb
new file mode 100644
index 00000000000..cf299c5d09b
--- /dev/null
+++ b/spec/services/projects/forks_count_service_spec.rb
@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe Projects::ForksCountService do
+  let(:project) { build(:project, id: 42) }
+  let(:service) { described_class.new(project) }
+
+  describe '#count' do
+    it 'returns the number of forks' do
+      allow(service).to receive(:uncached_count).and_return(1)
+
+      expect(service.count).to eq(1)
+    end
+
+    it 'caches the forks count', :use_clean_rails_memory_store_caching do
+      expect(service).to receive(:uncached_count).once.and_return(1)
+
+      2.times { service.count }
+    end
+  end
+
+  describe '#refresh_cache', :use_clean_rails_memory_store_caching do
+    it 'refreshes the cache' do
+      expect(service).to receive(:uncached_count).once.and_return(1)
+
+      service.refresh_cache
+
+      expect(service.count).to eq(1)
+    end
+  end
+
+  describe '#delete_cache', :use_clean_rails_memory_store_caching do
+    it 'removes the cache' do
+      expect(service).to receive(:uncached_count).twice.and_return(1)
+
+      service.count
+      service.delete_cache
+      service.count
+    end
+  end
+end
diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb
index 2ae8d5f7c54..4f1ab697460 100644
--- a/spec/services/projects/unlink_fork_service_spec.rb
+++ b/spec/services/projects/unlink_fork_service_spec.rb
@@ -29,4 +29,14 @@ describe Projects::UnlinkForkService do
 
     subject.execute
   end
+
+  it 'refreshes the forks count cache of the source project' do
+    source = fork_project.forked_from_project
+
+    expect(source.forks_count).to eq(1)
+
+    subject.execute
+
+    expect(source.forks_count).to be_zero
+  end
 end