Fix explore projects controller n+1 bullet report (!36874) · Merge requests · GitLab.org / GitLab

Terri Chu requested to merge tchu/fix-explore-projects-controller-n+1 into master Jul 14, 2020

What does this MR do?

bullet gem reported an n+1 issue on the /explore endpoint via the performance bar in gdk

USE eager loading detected: Project => [:project_feature] Add to your finder: :includes => [:project_feature]

 Project => [:project_feature]
  Add to your finder: :includes => [:project_feature]
Call stack
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/policies/project_policy.rb:659:in `feature_available?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/policies/project_policy.rb:174:in `block (2 levels) in <class:ProjectPolicy>'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/condition.rb:23:in `instance_eval'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/condition.rb:23:in `compute'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/condition.rb:44:in `block in pass?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/base.rb:303:in `cache'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/condition.rb:44:in `pass?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/rule.rb:81:in `pass?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/step.rb:81:in `pass?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:101:in `block in run'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:180:in `block in steps_by_score'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:149:in `loop'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:149:in `steps_by_score'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:82:in `run'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/runner.rb:60:in `pass?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/base.rb:234:in `block in allowed?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/base.rb:234:in `all?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/base.rb:234:in `allowed?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/declarative_policy/base.rb:226:in `can?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/models/ability.rb:72:in `allowed?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:200:in `can?'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/shared/projects/_project.html.haml:15:in `_app_views_shared_projects__project_html_haml___3501666621216227162_70206485117640'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/shared/projects/_list.html.haml:41:in `block in _app_views_shared_projects__list_html_haml__1883918250090759976_70206238178260'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/shared/projects/_list.html.haml:39:in `each_with_index'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/shared/projects/_list.html.haml:39:in `_app_views_shared_projects__list_html_haml__1883918250090759976_70206238178260'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/explore/projects/_projects.html.haml:2:in `_app_views_explore_projects__projects_html_haml___1259853146202165984_70206240426960'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/views/explore/projects/index.html.haml:13:in `_app_views_explore_projects_index_html_haml__4561333759503841090_70206490913620'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:134:in `render'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/ee/lib/gitlab/ip_address_state.rb:10:in `with'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/ee/app/controllers/ee/application_controller.rb:44:in `set_current_ip_address'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:491:in `set_current_admin'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/session.rb:11:in `with_session'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:482:in `set_session_storage'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/i18n.rb:55:in `with_locale'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/i18n.rb:61:in `with_user_locale'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:476:in `set_locale'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/error_tracking.rb:50:in `with_context'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:541:in `sentry_context'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:469:in `block in set_current_context'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/application_context.rb:52:in `block in use'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/application_context.rb:52:in `use'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/application_context.rb:20:in `with_context'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/app/controllers/application_controller.rb:462:in `set_current_context'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/elasticsearch_rack_middleware.rb:24:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/redis_rack_middleware.rb:22:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/rails_queue_duration.rb:29:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/rack_middleware.rb:17:in `block in call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/transaction.rb:54:in `run'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/rack_middleware.rb:17:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/request_profiler/middleware.rb:17:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/query_limiting/middleware.rb:17:in `block in call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/query_limiting/transaction.rb:39:in `run'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/query_limiting/middleware.rb:16:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/ee/lib/gitlab/database/load_balancing/rack_middleware.rb:39:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/ee/lib/gitlab/jira/middleware.rb:19:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/go.rb:20:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/etag_caching/middleware.rb:13:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/multipart.rb:140:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/read_only/controller.rb:51:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/read_only.rb:18:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/same_site_cookies.rb:27:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/basic_health_check.rb:25:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/request_context.rb:23:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/config/initializers/fix_local_cache_middleware.rb:9:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/static.rb:11:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/webpack/dev_server_middleware.rb:27:in `perform_request'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/metrics/requests_rack_middleware.rb:60:in `call'
  /Users/terrichu/Projects/gitlab/gitlab-development-kit/gitlab/lib/gitlab/middleware/release_env.rb:12:in `call'

add :project_feature to the preload_associations method

Note: project_feature is not an EE attribute so it went into the CE controller. project_feature is preloaded in the same manner on the Dashboard::ProjectsController

Database

Worst case is that project_feature is called for each project in the list. This doesn't appear to happen on GitLab.com when I go to the explore page, I looked into how/when that call gets made and believe it is called when the pipeline icon is presented to the user. This could affect self managed instances more often and I think would be a performance improvement.

Before

SQL

SELECT
    "project_features".*
FROM
    "project_features"
WHERE
    "project_features"."project_id" = 278964
LIMIT 1

Explain Plan

 Limit  (cost=0.43..3.45 rows=1 width=60) (actual time=7.208..7.209 rows=1 loops=1)
   Buffers: shared read=4
   I/O Timings: read=7.139
   ->  Index Scan using index_project_features_on_project_id on public.project_features  (cost=0.43..3.45 rows=1 width=60) (actual time=7.206..7.206 rows=1 loops=1)
         Index Cond: (project_features.project_id = 278964)
         Buffers: shared read=4
         I/O Timings: read=7.139

After

SQL

SELECT
    “project_features”. *
FROM
    “project_features”
WHERE
    “project_features”.“project_id” IN (29, 28, 27, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 278964, 13083)

Explain Plan

 Index Scan using index_project_features_on_project_id on public.project_features  (cost=0.43..69.50 rows=23 width=60) (actual time=11.021..29.663 rows=16 loops=1)
   Index Cond: (project_features.project_id = ANY ('{29,28,27,24,23,22,21,20,19,18,17,16,15,14,13,12,11,10,9,8,7,278964,13083}'::integer[]))
   Buffers: shared hit=72 read=16 dirtied=1
   I/O Timings: read=28.984

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

[-] Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
[-] Tested in all supported browsers
[-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

[-] Label as security and @ mention @gitlab-com/gl-security/appsec
[-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
[-] Security reports checked/validated by a reviewer from the AppSec team

Edited Jul 16, 2020 by Terri Chu

Fix explore projects controller n+1 bullet report