Commit 6ec53f5d authored by Yorick Peterse's avatar Yorick Peterse

Cache the number of open issues and merge requests

Every project page displays a navigation menu that in turn displays the
number of open issues and merge requests. This means that for every
project page we run two COUNT(*) queries, each taking up roughly 30
milliseconds on GitLab.com. By caching these numbers and refreshing them
whenever necessary we can reduce loading times of all these pages by up
to roughly 60 milliseconds.

The number of open issues does not include confidential issues. This is
a trade-off to keep the code simple and to ensure refreshing the data
only needs 2 COUNT(*) queries instead of 3. A downside is that if a
project only has 5 confidential issues the counter will be set to 0.

Because we now have 3 similar counting service classes the code
previously used in Projects::ForksCountService has mostly been moved to
Projects::CountService, which in turn is reused by the various service
classes.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36622
parent 133c72ae
...@@ -53,7 +53,10 @@ class Issue < ActiveRecord::Base ...@@ -53,7 +53,10 @@ class Issue < ActiveRecord::Base
scope :preload_associations, -> { preload(:labels, project: :namespace) } scope :preload_associations, -> { preload(:labels, project: :namespace) }
scope :public_only, -> { where(confidential: false) }
after_save :expire_etag_cache after_save :expire_etag_cache
after_commit :update_project_counter_caches, on: :destroy
attr_spammable :title, spam_title: true attr_spammable :title, spam_title: true
attr_spammable :description, spam_description: true attr_spammable :description, spam_description: true
...@@ -269,6 +272,10 @@ class Issue < ActiveRecord::Base ...@@ -269,6 +272,10 @@ class Issue < ActiveRecord::Base
end end
end end
def update_project_counter_caches
Projects::OpenIssuesCountService.new(project).refresh_cache
end
private private
# Returns `true` if the given User can read the current Issue. # Returns `true` if the given User can read the current Issue.
......
...@@ -32,6 +32,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -32,6 +32,7 @@ class MergeRequest < ActiveRecord::Base
after_create :ensure_merge_request_diff, unless: :importing? after_create :ensure_merge_request_diff, unless: :importing?
after_update :reload_diff_if_branch_changed after_update :reload_diff_if_branch_changed
after_commit :update_project_counter_caches, on: :destroy
# When this attribute is true some MR validation is ignored # When this attribute is true some MR validation is ignored
# It allows us to close or modify broken merge requests # It allows us to close or modify broken merge requests
...@@ -937,6 +938,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -937,6 +938,10 @@ class MergeRequest < ActiveRecord::Base
true true
end end
def update_project_counter_caches
Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
end
private private
def write_ref def write_ref
......
...@@ -1158,7 +1158,11 @@ class Project < ActiveRecord::Base ...@@ -1158,7 +1158,11 @@ class Project < ActiveRecord::Base
end end
def open_issues_count def open_issues_count
issues.opened.count Projects::OpenIssuesCountService.new(self).count
end
def open_merge_requests_count
Projects::OpenMergeRequestsCountService.new(self).count
end end
def visibility_level_allowed_as_fork?(level = self.visibility_level) def visibility_level_allowed_as_fork?(level = self.visibility_level)
......
...@@ -192,6 +192,8 @@ class IssuableBaseService < BaseService ...@@ -192,6 +192,8 @@ class IssuableBaseService < BaseService
def after_create(issuable) def after_create(issuable)
# To be overridden by subclasses # To be overridden by subclasses
issuable.update_project_counter_caches
end end
def before_update(issuable) def before_update(issuable)
...@@ -200,6 +202,8 @@ class IssuableBaseService < BaseService ...@@ -200,6 +202,8 @@ class IssuableBaseService < BaseService
def after_update(issuable) def after_update(issuable)
# To be overridden by subclasses # To be overridden by subclasses
issuable.update_project_counter_caches
end end
def update(issuable) def update(issuable)
......
...@@ -27,6 +27,8 @@ module Issues ...@@ -27,6 +27,8 @@ module Issues
todo_service.new_issue(issuable, current_user) todo_service.new_issue(issuable, current_user)
user_agent_detail_service.create user_agent_detail_service.create
resolve_discussions_with_issue(issuable) resolve_discussions_with_issue(issuable)
super
end end
def resolve_discussions_with_issue(issue) def resolve_discussions_with_issue(issue)
......
...@@ -28,6 +28,8 @@ module MergeRequests ...@@ -28,6 +28,8 @@ module MergeRequests
todo_service.new_merge_request(issuable, current_user) todo_service.new_merge_request(issuable, current_user)
issuable.cache_merge_request_closes_issues!(current_user) issuable.cache_merge_request_closes_issues!(current_user)
update_merge_requests_head_pipeline(issuable) update_merge_requests_head_pipeline(issuable)
super
end end
private private
......
module Projects
# Base class for the various service classes that count project data (e.g.
# issues or forks).
class CountService
def initialize(project)
@project = project
end
def relation_for_count
raise(
NotImplementedError,
'"relation_for_count" must be implemented and return an ActiveRecord::Relation'
)
end
def count
Rails.cache.fetch(cache_key) { uncached_count }
end
def refresh_cache
Rails.cache.write(cache_key, uncached_count)
end
def uncached_count
relation_for_count.count
end
def delete_cache
Rails.cache.delete(cache_key)
end
def cache_key_name
raise(
NotImplementedError,
'"cache_key_name" must be implemented and return a String'
)
end
def cache_key
['projects', @project.id, cache_key_name]
end
end
end
module Projects module Projects
# Service class for getting and caching the number of forks of a project. # Service class for getting and caching the number of forks of a project.
class ForksCountService class ForksCountService < CountService
def initialize(project) def relation_for_count
@project = project @project.forks
end end
def count def cache_key_name
Rails.cache.fetch(cache_key) { uncached_count } 'forks_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 end
end end
module Projects
# Service class for counting and caching the number of open issues of a
# project.
class OpenIssuesCountService < CountService
def relation_for_count
# We don't include confidential issues in this number since this would
# expose the number of confidential issues to non project members.
@project.issues.opened.public_only
end
def cache_key_name
'open_issues_count'
end
end
end
module Projects
# Service class for counting and caching the number of open merge requests of
# a project.
class OpenMergeRequestsCountService < CountService
def relation_for_count
@project.merge_requests.opened
end
def cache_key_name
'open_merge_requests_count'
end
end
end
...@@ -86,7 +86,8 @@ ...@@ -86,7 +86,8 @@
%span.nav-item-name %span.nav-item-name
Issues Issues
- if @project.issues_enabled? - if @project.issues_enabled?
%span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count) %span.badge.count.issue_counter
= number_with_delimiter(@project.open_issues_count)
%ul.sidebar-sub-level-items %ul.sidebar-sub-level-items
= nav_link(controller: :issues) do = nav_link(controller: :issues) do
...@@ -116,7 +117,8 @@ ...@@ -116,7 +117,8 @@
= custom_icon('mr_bold') = custom_icon('mr_bold')
%span.nav-item-name %span.nav-item-name
Merge Requests Merge Requests
%span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count) %span.badge.count.merge_counter.js-merge-counter
= number_with_delimiter(@project.open_merge_requests_count)
- if project_nav_tab? :pipelines - if project_nav_tab? :pipelines
= nav_link(controller: [:pipelines, :builds, :jobs, :pipeline_schedules, :environments, :artifacts]) do = nav_link(controller: [:pipelines, :builds, :jobs, :pipeline_schedules, :environments, :artifacts]) do
......
...@@ -28,7 +28,8 @@ ...@@ -28,7 +28,8 @@
%span %span
Issues Issues
- if @project.issues_enabled? - if @project.issues_enabled?
%span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id))) %span.badge.count.issue_counter
= number_with_delimiter(@project.open_issues_count)
- if project_nav_tab? :merge_requests - if project_nav_tab? :merge_requests
- controllers = [:merge_requests, 'projects/merge_requests/conflicts'] - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
...@@ -37,7 +38,8 @@ ...@@ -37,7 +38,8 @@
= link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do = link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
%span %span
Merge Requests Merge Requests
%span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(issuables_count_for_state(:merge_requests, :opened, finder: MergeRequestsFinder.new(current_user, project_id: @project.id))) %span.badge.count.merge_counter.js-merge-counter
= number_with_delimiter(@project.open_merge_requests_count)
- if project_nav_tab? :pipelines - if project_nav_tab? :pipelines
= nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
......
---
title: Cache the number of open issues and merge requests
merge_request:
author:
type: other
...@@ -751,4 +751,22 @@ describe Issue do ...@@ -751,4 +751,22 @@ describe Issue do
end end
end end
end end
describe 'removing an issue' do
it 'refreshes the number of open issues of the project' do
project = subject.project
expect { subject.destroy }
.to change { project.open_issues_count }.from(1).to(0)
end
end
describe '.public_only' do
it 'only returns public issues' do
public_issue = create(:issue)
create(:issue, confidential: true)
expect(described_class.public_only).to eq([public_issue])
end
end
end end
...@@ -1692,4 +1692,13 @@ describe MergeRequest do ...@@ -1692,4 +1692,13 @@ describe MergeRequest do
expect(subject.ref_fetched?).to be_falsey expect(subject.ref_fetched?).to be_falsey
end end
end end
describe 'removing a merge request' do
it 'refreshes the number of open merge requests of the target project' do
project = subject.target_project
expect { subject.destroy }
.to change { project.open_merge_requests_count }.from(1).to(0)
end
end
end end
...@@ -42,6 +42,11 @@ describe Issues::CloseService do ...@@ -42,6 +42,11 @@ describe Issues::CloseService do
service.execute(issue) service.execute(issue)
end end
it 'refreshes the number of open issues' do
expect { service.execute(issue) }
.to change { project.open_issues_count }.from(1).to(0)
end
it 'invalidates counter cache for assignees' do it 'invalidates counter cache for assignees' do
expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts) expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts)
......
...@@ -35,6 +35,10 @@ describe Issues::CreateService do ...@@ -35,6 +35,10 @@ describe Issues::CreateService do
expect(issue.due_date).to eq Date.tomorrow expect(issue.due_date).to eq Date.tomorrow
end end
it 'refreshes the number of open issues' do
expect { issue }.to change { project.open_issues_count }.from(0).to(1)
end
context 'when current user cannot admin issues in the project' do context 'when current user cannot admin issues in the project' do
let(:guest) { create(:user) } let(:guest) { create(:user) }
......
...@@ -34,6 +34,13 @@ describe Issues::ReopenService do ...@@ -34,6 +34,13 @@ describe Issues::ReopenService do
described_class.new(project, user).execute(issue) described_class.new(project, user).execute(issue)
end end
it 'refreshes the number of opened issues' do
service = described_class.new(project, user)
expect { service.execute(issue) }
.to change { project.open_issues_count }.from(0).to(1)
end
context 'when issue is not confidential' do context 'when issue is not confidential' do
it 'executes issue hooks' do it 'executes issue hooks' do
expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
......
...@@ -52,6 +52,13 @@ describe MergeRequests::CloseService do ...@@ -52,6 +52,13 @@ describe MergeRequests::CloseService do
end end
end end
it 'refreshes the number of open merge requests for a valid MR' do
service = described_class.new(project, user, {})
expect { service.execute(merge_request) }
.to change { project.open_merge_requests_count }.from(1).to(0)
end
context 'current user is not authorized to close merge request' do context 'current user is not authorized to close merge request' do
before do before do
perform_enqueued_jobs do perform_enqueued_jobs do
......
...@@ -18,31 +18,35 @@ describe MergeRequests::CreateService do ...@@ -18,31 +18,35 @@ describe MergeRequests::CreateService do
end end
let(:service) { described_class.new(project, user, opts) } let(:service) { described_class.new(project, user, opts) }
let(:merge_request) { service.execute }
before do before do
project.team << [user, :master] project.team << [user, :master]
project.team << [assignee, :developer] project.team << [assignee, :developer]
allow(service).to receive(:execute_hooks) allow(service).to receive(:execute_hooks)
@merge_request = service.execute
end end
it 'creates an MR' do it 'creates an MR' do
expect(@merge_request).to be_valid expect(merge_request).to be_valid
expect(@merge_request.title).to eq('Awesome merge_request') expect(merge_request.title).to eq('Awesome merge_request')
expect(@merge_request.assignee).to be_nil expect(merge_request.assignee).to be_nil
expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') expect(merge_request.merge_params['force_remove_source_branch']).to eq('1')
end end
it 'executes hooks with default action' do it 'executes hooks with default action' do
expect(service).to have_received(:execute_hooks).with(@merge_request) expect(service).to have_received(:execute_hooks).with(merge_request)
end
it 'refreshes the number of open merge requests' do
expect { service.execute }
.to change { project.open_merge_requests_count }.from(0).to(1)
end end
it 'does not creates todos' do it 'does not creates todos' do
attributes = { attributes = {
project: project, project: project,
target_id: @merge_request.id, target_id: merge_request.id,
target_type: @merge_request.class.name target_type: merge_request.class.name
} }
expect(Todo.where(attributes).count).to be_zero expect(Todo.where(attributes).count).to be_zero
...@@ -51,8 +55,8 @@ describe MergeRequests::CreateService do ...@@ -51,8 +55,8 @@ describe MergeRequests::CreateService do
it 'creates exactly 1 create MR event' do it 'creates exactly 1 create MR event' do
attributes = { attributes = {
action: Event::CREATED, action: Event::CREATED,
target_id: @merge_request.id, target_id: merge_request.id,
target_type: @merge_request.class.name target_type: merge_request.class.name
} }
expect(Event.where(attributes).count).to eq(1) expect(Event.where(attributes).count).to eq(1)
...@@ -69,15 +73,15 @@ describe MergeRequests::CreateService do ...@@ -69,15 +73,15 @@ describe MergeRequests::CreateService do
} }
end end
it { expect(@merge_request.assignee).to eq assignee } it { expect(merge_request.assignee).to eq assignee }
it 'creates a todo for new assignee' do it 'creates a todo for new assignee' do
attributes = { attributes = {
project: project, project: project,
author: user, author: user,
user: assignee, user: assignee,
target_id: @merge_request.id, target_id: merge_request.id,
target_type: @merge_request.class.name, target_type: merge_request.class.name,
action: Todo::ASSIGNED, action: Todo::ASSIGNED,
state: :pending state: :pending
} }
......
...@@ -47,6 +47,13 @@ describe MergeRequests::ReopenService do ...@@ -47,6 +47,13 @@ describe MergeRequests::ReopenService do
end end
end end
it 'refreshes the number of open merge requests for a valid MR' do
service = described_class.new(project, user, {})
expect { service.execute(merge_request) }
.to change { project.open_merge_requests_count }.from(0).to(1)
end
context 'current user is not authorized to reopen merge request' do context 'current user is not authorized to reopen merge request' do
before do before do
perform_enqueued_jobs do perform_enqueued_jobs do
......
require 'spec_helper'
describe Projects::CountService do
let(:project) { build(:project, id: 1) }
let(:service) { described_class.new(project) }
describe '#relation_for_count' do
it 'raises NotImplementedError' do
expect { service.relation_for_count }.to raise_error(NotImplementedError)
end
end
describe '#count' do
before do
allow(service).to receive(:cache_key_name).and_return('count_service')
end
it 'returns the number of rows' do
allow(service).to receive(:uncached_count).and_return(1)
expect(service.count).to eq(1)
end
it 'caches the number of rows', :use_clean_rails_memory_store_caching do
expect(service).to receive(:uncached_count).once.and_return(1)
2.times do
expect(service.count).to eq(1)
end
end
end
describe '#refresh_cache', :use_clean_rails_memory_store_caching do
before do
allow(service).to receive(:cache_key_name).and_return('count_service')
end
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
before do
allow(service).to receive(:cache_key_name).and_return('count_service')
end
it 'removes the cache' do
expect(service).to receive(:uncached_count).twice.and_return(1)
service.count
service.delete_cache
service.count
end
end
describe '#cache_key_name' do
it 'raises NotImplementedError' do
expect { service.cache_key_name }.to raise_error(NotImplementedError)
end
end
describe '#cache_key' do
it 'returns the cache key as an Array' do
allow(service).to receive(:cache_key_name).and_return('count_service')
expect(service.cache_key).to eq(['projects', 1, 'count_service'])
end
end
end
require 'spec_helper' require 'spec_helper'
describe Projects::ForksCountService do describe Projects::ForksCountService do
let(:project) { build(:project, id: 42) }
let(:service) { described_class.new(project) }
describe '#count' do describe '#count' do
it 'returns the number of forks' do it 'returns the number of forks' do
allow(service).to receive(:uncached_count).and_return(1) project = build(:project, id: 42)
service = described_class.new(project)
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 } allow(service).to receive(:uncached_count).and_return(1)
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) expect(service.count).to eq(1)
end end
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 end
require 'spec_helper'
describe Projects::OpenIssuesCountService do
describe '#count' do
it 'returns the number of open issues' do
project = create(:project)
create(:issue, :opened, project: project)
expect(described_class.new(project).count).to eq(1)
end
it 'does not include confidential issues in the issue count' do
project = create(:project)
create(:issue, :opened, project: project)
create(:issue, :opened, confidential: true, project: project)
expect(described_class.new(project).count).to eq(1)
end
end
end
require 'spec_helper'
describe Projects::OpenMergeRequestsCountService do
describe '#count' do
it 'returns the number of open merge requests' do
project = create(:project)
create(:merge_request,
:opened,
source_project: project,
target_project: project)
expect(described_class.new(project).count).to eq(1)
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