Commit aef9f1eb authored by Yorick Peterse's avatar Yorick Peterse

Cache the number of forks of a project

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).
parent 21a6898b
...@@ -196,7 +196,6 @@ class Project < ActiveRecord::Base ...@@ -196,7 +196,6 @@ class Project < ActiveRecord::Base
accepts_nested_attributes_for :import_data accepts_nested_attributes_for :import_data
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
delegate :count, to: :forks, prefix: true
delegate :members, to: :team, prefix: true delegate :members, to: :team, prefix: true
delegate :add_user, :add_users, to: :team delegate :add_user, :add_users, to: :team
delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team
...@@ -1398,6 +1397,10 @@ class Project < ActiveRecord::Base ...@@ -1398,6 +1397,10 @@ class Project < ActiveRecord::Base
# @deprecated cannot remove yet because it has an index with its name in elasticsearch # @deprecated cannot remove yet because it has an index with its name in elasticsearch
alias_method :path_with_namespace, :full_path alias_method :path_with_namespace, :full_path
def forks_count
Projects::ForksCountService.new(self).count
end
private private
def cross_namespace_reference?(from) def cross_namespace_reference?(from)
......
...@@ -128,6 +128,8 @@ module Projects ...@@ -128,6 +128,8 @@ module Projects
project.repository.before_delete project.repository.before_delete
Repository.new(wiki_path, project, disk_path: repo_path).before_delete Repository.new(wiki_path, project, disk_path: repo_path).before_delete
Projects::ForksCountService.new(project).delete_cache
end end
end end
end end
...@@ -21,11 +21,17 @@ module Projects ...@@ -21,11 +21,17 @@ module Projects
builds_access_level = @project.project_feature.builds_access_level builds_access_level = @project.project_feature.builds_access_level
new_project.project_feature.update_attributes(builds_access_level: builds_access_level) new_project.project_feature.update_attributes(builds_access_level: builds_access_level)
refresh_forks_count
new_project new_project
end end
private private
def refresh_forks_count
Projects::ForksCountService.new(@project).refresh_cache
end
def allowed_visibility_level def allowed_visibility_level
project_level = @project.visibility_level project_level = @project.visibility_level
......
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
...@@ -13,7 +13,13 @@ module Projects ...@@ -13,7 +13,13 @@ module Projects
::MergeRequests::CloseService.new(@project, @current_user).execute(mr) ::MergeRequests::CloseService.new(@project, @current_user).execute(mr)
end end
refresh_forks_count(@project.forked_from_project)
@project.forked_project_link.destroy @project.forked_project_link.destroy
end end
def refresh_forks_count(project)
Projects::ForksCountService.new(project).refresh_cache
end
end end
end end
---
title: Cache the number of forks of a project
merge_request: 13535
author:
type: other
...@@ -351,6 +351,8 @@ module API ...@@ -351,6 +351,8 @@ module API
if user_project.forked_from_project.nil? 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) 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 else
render_api_error!("Project already forked", 409) render_api_error!("Project already forked", 409)
end end
......
...@@ -388,6 +388,8 @@ module API ...@@ -388,6 +388,8 @@ module API
if user_project.forked_from_project.nil? 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) 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 else
render_api_error!("Project already forked", 409) render_api_error!("Project already forked", 409)
end end
......
...@@ -2310,4 +2310,14 @@ describe Project do ...@@ -2310,4 +2310,14 @@ describe Project do
end end
end 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 end
...@@ -1065,6 +1065,14 @@ describe API::Projects do ...@@ -1065,6 +1065,14 @@ describe API::Projects do
expect(project_fork_target.forked?).to be_truthy expect(project_fork_target.forked?).to be_truthy
end 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 it 'fails if forked_from project which does not exist' do
post api("/projects/#{project_fork_target.id}/fork/9999", admin) post api("/projects/#{project_fork_target.id}/fork/9999", admin)
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
......
...@@ -1004,6 +1004,14 @@ describe API::V3::Projects do ...@@ -1004,6 +1004,14 @@ describe API::V3::Projects do
expect(project_fork_target.forked?).to be_truthy expect(project_fork_target.forked?).to be_truthy
end 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 it 'fails if forked_from project which does not exist' do
post v3_api("/projects/#{project_fork_target.id}/fork/9999", admin) post v3_api("/projects/#{project_fork_target.id}/fork/9999", admin)
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
......
...@@ -50,6 +50,14 @@ describe Projects::ForkService do ...@@ -50,6 +50,14 @@ describe Projects::ForkService do
expect(@from_project.avatar.file).to be_exists expect(@from_project.avatar.file).to be_exists
end 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
end end
......
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
...@@ -29,4 +29,14 @@ describe Projects::UnlinkForkService do ...@@ -29,4 +29,14 @@ describe Projects::UnlinkForkService do
subject.execute subject.execute
end 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 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