Merge branch 'lm-improve-n-1-pl-serializer' into 'master'

Improve n+1 in pipeline serializer for triggered pipelines

See merge request gitlab-org/gitlab!42421

app/models/ci/pipeline.rb

@@ -42,6 +42,7 @@ module Ci
     has_many :stages, -> { order(position: :asc) }, inverse_of: :pipeline
     has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :latest_statuses_ordered_by_stage, -> { latest.order(:stage_idx, :stage) }, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
+    has_many :latest_statuses, -> { latest }, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :processables, class_name: 'Ci::Processable', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :bridges, class_name: 'Ci::Bridge', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
@@ -577,11 +578,11 @@ module Ci
     end
 
     def retried
-      @retried ||= (statuses.order(id: :desc) - statuses.latest)
+      @retried ||= (statuses.order(id: :desc) - latest_statuses)
     end
 
     def coverage
-      coverage_array = statuses.latest.map(&:coverage).compact
+      coverage_array = latest_statuses.map(&:coverage).compact
       if coverage_array.size >= 1
         '%.2f' % (coverage_array.reduce(:+) / coverage_array.size)
       end

app/serializers/pipeline_serializer.rb

@@ -47,6 +47,7 @@ class PipelineSerializer < BaseSerializer
       :retryable_builds,
       :scheduled_actions,
       :stages,
+      :latest_statuses,
       :trigger_requests,
       :user,
       {
@@ -62,7 +63,14 @@ class PipelineSerializer < BaseSerializer
         pending_builds: :project,
         project: [:route, { namespace: :route }],
         triggered_by_pipeline: [{ project: [:route, { namespace: :route }] }, :user],
-        triggered_pipelines: [{ project: [:route, { namespace: :route }] }, :user, :source_job]
+        triggered_pipelines: [
+          {
+            project: [:route, { namespace: :route }]
+          },
+          :source_job,
+          :latest_statuses,
+          :user
+        ]
       }
     ]
   end

app/services/ci/process_pipeline_service.rb

@@ -31,14 +31,14 @@ module Ci
     # rubocop: disable CodeReuse/ActiveRecord
     def update_retried
       # find the latest builds for each name
-      latest_statuses = pipeline.statuses.latest
+      latest_statuses = pipeline.latest_statuses
         .group(:name)
         .having('count(*) > 1')
         .pluck(Arel.sql('MAX(id)'), 'name')
 
       # mark builds that are retried
       if latest_statuses.any?
-        pipeline.statuses.latest
+        pipeline.latest_statuses
           .where(name: latest_statuses.map(&:second))
           .where.not(id: latest_statuses.map(&:first))
           .update_all(retried: true)

app/views/notify/autodevops_disabled_email.html.haml

@@ -46,4 +46,4 @@
             %td{ style: "font-family: 'Menlo','Liberation Mono','Consolas','DejaVu Sans Mono','Ubuntu Mono','Courier New','andale mono','lucida console',monospace; font-size: 14px; line-height: 1.4; vertical-align: baseline; padding:0 8px;" }
               API
 
-= render 'notify/failed_builds', pipeline: @pipeline, failed: @pipeline.statuses.latest.failed
+= render 'notify/failed_builds', pipeline: @pipeline, failed: @pipeline.latest_statuses.failed

app/views/notify/autodevops_disabled_email.text.erb

@@ -7,7 +7,7 @@ The Auto DevOps pipeline failed for pipeline <%= @pipeline.iid %> (<%= pipeline_
 <% else -%>
   Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API
 <% end -%>
-<% failed = @pipeline.statuses.latest.failed -%>
+<% failed = @pipeline.latest_statuses.failed -%>
 had <%= failed.size %> failed <%= 'build'.pluralize(failed.size) %>.
 
 <% failed.each do |build| -%>

app/views/notify/pipeline_failed_email.html.haml

@@ -108,4 +108,4 @@
             %td{ style: "font-family:'Menlo','Liberation Mono','Consolas','DejaVu Sans Mono','Ubuntu Mono','Courier New','andale mono','lucida console',monospace;font-size:14px;line-height:1.4;vertical-align:baseline;padding:0 5px;" }
               API
 
-= render 'notify/failed_builds', pipeline: @pipeline, failed: @pipeline.statuses.latest.failed
+= render 'notify/failed_builds', pipeline: @pipeline, failed: @pipeline.latest_statuses.failed

app/views/notify/pipeline_failed_email.text.erb

@@ -27,7 +27,7 @@ Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%
 <% else -%>
 Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API
 <% end -%>
-<% failed = @pipeline.statuses.latest.failed -%>
+<% failed = @pipeline.latest_statuses.failed -%>
 had <%= failed.size %> failed <%= 'build'.pluralize(failed.size) %>.
 
 <% failed.each do |build| -%>

changelogs/unreleased/lm-improve-n-1-pl-serializer.yml

+---
+title: Improve n+1 in pipeline serializer for triggered pipelines
+merge_request: 42421
+author:
+type: performance

spec/lib/gitlab/import_export/all_models.yml

@@ -242,6 +242,7 @@ ci_pipelines:
 - latest_builds_report_results
 - messages
 - pipeline_artifacts
+- latest_statuses
 ci_refs:
 - project
 - ci_pipelines

spec/models/ci/pipeline_spec.rb

@@ -2436,7 +2436,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   end
 
   describe '#retry_failed' do
-    let(:latest_status) { pipeline.statuses.latest.pluck(:status) }
+    let(:latest_status) { pipeline.latest_statuses.pluck(:status) }
 
     before do
       stub_not_protect_default_branch

spec/serializers/pipeline_serializer_spec.rb

@@ -155,7 +155,7 @@ RSpec.describe PipelineSerializer do
 
         it 'verifies number of queries', :request_store do
           recorded = ActiveRecord::QueryRecorder.new { subject }
-          expected_queries = Gitlab.ee? ? 43 : 40
+          expected_queries = Gitlab.ee? ? 39 : 36
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)
@@ -176,7 +176,7 @@ RSpec.describe PipelineSerializer do
           # pipeline. With the same ref this check is cached but if refs are
           # different then there is an extra query per ref
           # https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
-          expected_queries = Gitlab.ee? ? 49 : 46
+          expected_queries = Gitlab.ee? ? 42 : 39
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)
@@ -199,11 +199,10 @@ RSpec.describe PipelineSerializer do
         it 'verifies number of queries', :request_store do
           recorded = ActiveRecord::QueryRecorder.new { subject }
 
-          # 99 queries by default + 2 related to preloading
-          # :source_pipeline and :source_job
           # Existing numbers are high and require performance optimization
+          # Ongoing issue:
           # https://gitlab.com/gitlab-org/gitlab/-/issues/225156
-          expected_queries = Gitlab.ee? ? 95 : 86
+          expected_queries = Gitlab.ee? ? 85 : 76
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)

spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb

@@ -43,12 +43,12 @@ RSpec.shared_context 'Pipeline Processing Service Tests With Yaml' do
       {
         pipeline: pipeline.status,
         stages: pipeline.stages.pluck(:name, :status).to_h,
-        jobs: pipeline.statuses.latest.pluck(:name, :status).to_h
+        jobs: pipeline.latest_statuses.pluck(:name, :status).to_h
       }
     end
 
     def event_on_jobs(event, job_names)
-      statuses = pipeline.statuses.latest.by_name(job_names).to_a
+      statuses = pipeline.latest_statuses.by_name(job_names).to_a
       expect(statuses.count).to eq(job_names.count) # ensure that we have the same counts
 
       statuses.each { |status| status.public_send("#{event}!") }