Commit d272f91a authored by Gosia Ksionek's avatar Gosia Ksionek Committed by Bob Van Landuyt

Refactor BatchForksCountService class

In order to avoid multiple calls to Redis
we gather all the forks count in one mget
call.

Modify group detail class

Modify passed options

Fix rubocop offences

Fix specs
parent 2f7d087b
...@@ -7,12 +7,6 @@ module RendersMemberAccess ...@@ -7,12 +7,6 @@ module RendersMemberAccess
groups groups
end end
def prepare_projects_for_rendering(projects)
preload_max_member_access_for_collection(Project, projects)
projects
end
private private
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
......
# frozen_string_literal: true
module RendersProjectsList
def prepare_projects_for_rendering(projects)
preload_max_member_access_for_collection(Project, projects)
# Call the forks count method on every project, so the BatchLoader would load them all at
# once when the entities are rendered
projects.each(&:forks_count)
projects
end
end
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
class Dashboard::ProjectsController < Dashboard::ApplicationController class Dashboard::ProjectsController < Dashboard::ApplicationController
include ParamsBackwardCompatibility include ParamsBackwardCompatibility
include RendersMemberAccess include RendersMemberAccess
include RendersProjectsList
include SortingHelper include SortingHelper
include SortingPreference include SortingPreference
include FiltersEvents include FiltersEvents
......
...@@ -4,6 +4,7 @@ class Explore::ProjectsController < Explore::ApplicationController ...@@ -4,6 +4,7 @@ class Explore::ProjectsController < Explore::ApplicationController
include PageLimiter include PageLimiter
include ParamsBackwardCompatibility include ParamsBackwardCompatibility
include RendersMemberAccess include RendersMemberAccess
include RendersProjectsList
include SortingHelper include SortingHelper
include SortingPreference include SortingPreference
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
class Projects::ForksController < Projects::ApplicationController class Projects::ForksController < Projects::ApplicationController
include ContinueParams include ContinueParams
include RendersMemberAccess include RendersMemberAccess
include RendersProjectsList
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
# Authorize # Authorize
......
...@@ -13,10 +13,15 @@ class RootController < Dashboard::ProjectsController ...@@ -13,10 +13,15 @@ class RootController < Dashboard::ProjectsController
before_action :redirect_unlogged_user, if: -> { current_user.nil? } before_action :redirect_unlogged_user, if: -> { current_user.nil? }
before_action :redirect_logged_user, if: -> { current_user.present? } before_action :redirect_logged_user, if: -> { current_user.present? }
# We only need to load the projects when the user is logged in but did not
# configure a dashboard. In which case we render projects. We can do that straight
# from the #index action.
skip_before_action :projects
def index def index
# n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/40260 # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/40260
Gitlab::GitalyClient.allow_n_plus_1_calls do Gitlab::GitalyClient.allow_n_plus_1_calls do
projects
super super
end end
end end
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
class UsersController < ApplicationController class UsersController < ApplicationController
include RoutableActions include RoutableActions
include RendersMemberAccess include RendersMemberAccess
include RendersProjectsList
include ControllerWithCrossProjectAccessCheck include ControllerWithCrossProjectAccessCheck
include Gitlab::NoteableMetadata include Gitlab::NoteableMetadata
......
...@@ -2153,7 +2153,13 @@ class Project < ApplicationRecord ...@@ -2153,7 +2153,13 @@ class Project < ApplicationRecord
# rubocop: disable CodeReuse/ServiceClass # rubocop: disable CodeReuse/ServiceClass
def forks_count def forks_count
Projects::ForksCountService.new(self).count BatchLoader.for(self).batch do |projects, loader|
fork_count_per_project = ::Projects::BatchForksCountService.new(projects).refresh_cache_and_retrieve_data
fork_count_per_project.each do |project, count|
loader.call(project, count)
end
end
end end
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
......
...@@ -5,6 +5,21 @@ ...@@ -5,6 +5,21 @@
# because the service use maps to retrieve the project ids # because the service use maps to retrieve the project ids
module Projects module Projects
class BatchForksCountService < Projects::BatchCountService class BatchForksCountService < Projects::BatchCountService
def refresh_cache_and_retrieve_data
count_services = @projects.map { |project| count_service.new(project) }
values = Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
Rails.cache.fetch_multi(*(count_services.map { |ser| ser.cache_key } )) { |key| nil }
end
results_per_service = Hash[count_services.zip(values.values)]
projects_to_refresh = results_per_service.select { |_k, value| value.nil? }
projects_to_refresh = recreate_cache(projects_to_refresh)
results_per_service.update(projects_to_refresh)
results_per_service.transform_keys { |k| k.project }
end
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def global_count def global_count
@global_count ||= begin @global_count ||= begin
...@@ -18,5 +33,13 @@ module Projects ...@@ -18,5 +33,13 @@ module Projects
def count_service def count_service
::Projects::ForksCountService ::Projects::ForksCountService
end end
def recreate_cache(projects_to_refresh)
projects_to_refresh.each_with_object({}) do |(service, _v), hash|
count = global_count[service.project.id].to_i
service.refresh_cache { count }
hash[service] = count
end
end
end end
end end
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
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 < Projects::CountService class ForksCountService < Projects::CountService
attr_reader :project
def cache_key_name def cache_key_name
'forks_count' 'forks_count'
end end
......
---
title: Use BatchLoader for Project.forks_count to limit calls to Redis
merge_request: 35328
author:
type: performance
...@@ -33,7 +33,8 @@ module API ...@@ -33,7 +33,8 @@ module API
project.avatar_url(only_path: false) project.avatar_url(only_path: false)
end end
expose :star_count, :forks_count expose :forks_count
expose :star_count
expose :last_activity_at expose :last_activity_at
expose :namespace, using: 'API::Entities::NamespaceBasic' expose :namespace, using: 'API::Entities::NamespaceBasic'
expose :custom_attributes, using: 'API::Entities::CustomAttribute', if: :with_custom_attributes expose :custom_attributes, using: 'API::Entities::CustomAttribute', if: :with_custom_attributes
......
...@@ -7,6 +7,7 @@ module API ...@@ -7,6 +7,7 @@ module API
SharedGroupWithGroup.represent(group.shared_with_group_links.public_or_visible_to_user(group, options[:current_user])) SharedGroupWithGroup.represent(group.shared_with_group_links.public_or_visible_to_user(group, options[:current_user]))
end end
expose :runners_token, if: lambda { |group, options| options[:user_can_admin_group] } expose :runners_token, if: lambda { |group, options| options[:user_can_admin_group] }
expose :projects, using: Entities::Project do |group, options| expose :projects, using: Entities::Project do |group, options|
projects = GroupProjectsFinder.new( projects = GroupProjectsFinder.new(
group: group, group: group,
......
...@@ -8,6 +8,10 @@ module API ...@@ -8,6 +8,10 @@ module API
def prepare_relation(projects_relation, options = {}) def prepare_relation(projects_relation, options = {})
projects_relation = preload_relation(projects_relation, options) projects_relation = preload_relation(projects_relation, options)
execute_batch_counting(projects_relation) execute_batch_counting(projects_relation)
# Call the forks count method on every project, so the BatchLoader would load them all at
# once when the entities are rendered
projects_relation.each(&:forks_count)
projects_relation projects_relation
end end
...@@ -19,16 +23,11 @@ module API ...@@ -19,16 +23,11 @@ module API
projects_relation projects_relation
end end
def batch_forks_counting(projects_relation)
::Projects::BatchForksCountService.new(forks_counting_projects(projects_relation)).refresh_cache
end
def batch_open_issues_counting(projects_relation) def batch_open_issues_counting(projects_relation)
::Projects::BatchOpenIssuesCountService.new(projects_relation).refresh_cache ::Projects::BatchOpenIssuesCountService.new(projects_relation).refresh_cache
end end
def execute_batch_counting(projects_relation) def execute_batch_counting(projects_relation)
batch_forks_counting(projects_relation)
batch_open_issues_counting(projects_relation) batch_open_issues_counting(projects_relation)
end end
end end
......
...@@ -4116,7 +4116,7 @@ RSpec.describe Project do ...@@ -4116,7 +4116,7 @@ RSpec.describe Project do
it 'returns the number of forks' do it 'returns the number of forks' do
project = build(:project) project = build(:project)
expect_any_instance_of(Projects::ForksCountService).to receive(:count).and_return(1) expect_any_instance_of(::Projects::BatchForksCountService).to receive(:refresh_cache_and_retrieve_data).and_return({ project => 1 })
expect(project.forks_count).to eq(1) expect(project.forks_count).to eq(1)
end end
......
...@@ -10,6 +10,7 @@ RSpec.describe Projects::ForkService do ...@@ -10,6 +10,7 @@ RSpec.describe Projects::ForkService do
expect(from_project.forks_count).to be_zero expect(from_project.forks_count).to be_zero
fork_project(from_project, to_user) fork_project(from_project, to_user)
BatchLoader::Executor.clear_current
expect(from_project.forks_count).to eq(1) expect(from_project.forks_count).to eq(1)
end end
...@@ -405,6 +406,7 @@ RSpec.describe Projects::ForkService do ...@@ -405,6 +406,7 @@ RSpec.describe Projects::ForkService do
expect(fork_from_project.forks_count).to be_zero expect(fork_from_project.forks_count).to be_zero
subject.execute(fork_to_project) subject.execute(fork_to_project)
BatchLoader::Executor.clear_current
expect(fork_from_project.forks_count).to eq(1) expect(fork_from_project.forks_count).to eq(1)
end end
......
...@@ -53,6 +53,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin ...@@ -53,6 +53,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
expect(source.forks_count).to eq(1) expect(source.forks_count).to eq(1)
subject.execute subject.execute
BatchLoader::Executor.clear_current
expect(source.forks_count).to be_zero expect(source.forks_count).to be_zero
end end
...@@ -146,6 +147,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin ...@@ -146,6 +147,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
expect(project.forks_count).to eq(2) expect(project.forks_count).to eq(2)
subject.execute subject.execute
BatchLoader::Executor.clear_current
expect(project.forks_count).to be_zero expect(project.forks_count).to be_zero
end end
...@@ -212,6 +214,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin ...@@ -212,6 +214,7 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
expect(forked_project.forks_count).to eq(1) expect(forked_project.forks_count).to eq(1)
subject.execute subject.execute
BatchLoader::Executor.clear_current
expect(project.forks_count).to eq(1) expect(project.forks_count).to eq(1)
expect(forked_project.forks_count).to eq(0) expect(forked_project.forks_count).to eq(0)
......
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