Reduce CommitIsAncestor RPCs with environments

When there are many environments, loading diffs, blobs, and comparisons
can be slow because we load many environments and call CommitIsAncestor
on each of them to determine whether each environment is relevant to a
given commit SHA. This is a bit of a waste because in most cases, the
controller just needs the latest environment for the given commit.

This commit adds a `find_latest` flag to `EnvironmentsFinder` to look
for only one environment that matches the criteria. In the common case,
we will only need to look for one or two environments instead of
iterating through all of them.

Closes https://gitlab.com/gitlab-org/gitlab/issues/29562

app/controllers/projects/blame_controller.rb

@@ -17,6 +17,7 @@ class Projects::BlameController < Projects::ApplicationController
     end
 
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
+    environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
 
     @blame_groups = Gitlab::Blame.new(@blob, @commit).groups

app/controllers/projects/blob_controller.rb

@@ -205,6 +205,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def show_html
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
+    environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
     @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path)
 

app/controllers/projects/commit_controller.rb

@@ -151,7 +151,7 @@ class Projects::CommitController < Projects::ApplicationController
     @diffs = commit.diffs(opts)
     @notes_count = commit.notes.count
 
-    @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit).execute.last
+    @environment = EnvironmentsFinder.new(@project, current_user, commit: @commit, find_latest: true).execute.last
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

app/controllers/projects/compare_controller.rb

@@ -101,6 +101,7 @@ class Projects::CompareController < Projects::ApplicationController
   def define_environment
     if compare
       environment_params = @repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit }
+      environment_params[:find_latest] = true
       @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
     end
   end

app/controllers/projects/merge_requests/creations_controller.rb

@@ -52,7 +52,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
 
     @diff_notes_disabled = true
 
-    @environment = @merge_request.environments_for(current_user).last
+    @environment = @merge_request.environments_for(current_user, latest: true).last
 
     render json: { html: view_to_html_string('projects/merge_requests/creations/_diffs', diffs: @diffs, environment: @environment) }
   end

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -51,7 +51,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def render_diffs
     diffs = @compare.diffs(diff_options)
-    @environment = @merge_request.environments_for(current_user).last
+    @environment = @merge_request.environments_for(current_user, latest: true).last
 
     diffs.unfold_diff_files(note_positions.unfoldable)
     diffs.write_cache

app/finders/environments_finder.rb

@@ -25,25 +25,13 @@ class EnvironmentsFinder
       .select(:environment_id)
 
     environments = project.environments.available
-      .where(id: environment_ids).order_by_last_deployed_at.to_a
+      .where(id: environment_ids)
 
-    environments.select! do |environment|
-      Ability.allowed?(current_user, :read_environment, environment)
-    end
-
-    if ref && commit
-      environments.select! do |environment|
-        environment.includes_commit?(commit)
-      end
-    end
-
-    if ref && params[:recently_updated]
-      environments.select! do |environment|
-        environment.recently_updated_on_branch?(ref)
-      end
+    if params[:find_latest]
+      find_one(environments.order_by_last_deployed_at_desc)
+    else
+      find_all(environments.order_by_last_deployed_at.to_a)
     end
-
-    environments
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -62,6 +50,24 @@ class EnvironmentsFinder
 
   private
 
+  def find_one(environments)
+    [environments.find { |environment| valid_environment?(environment) }].compact
+  end
+
+  def find_all(environments)
+    environments.select { |environment| valid_environment?(environment) }
+  end
+
+  def valid_environment?(environment)
+    # Go in order of cost: SQL calls are cheaper than Gitaly calls
+    return false unless Ability.allowed?(current_user, :read_environment, environment)
+
+    return false if ref && params[:recently_updated] && !environment.recently_updated_on_branch?(ref)
+    return false if ref && commit && !environment.includes_commit?(commit)
+
+    true
+  end
+
   def ref
     params[:ref].try(:to_s)
   end

app/models/environment.rb

@@ -48,13 +48,14 @@ class Environment < ApplicationRecord
 
   scope :available, -> { with_state(:available) }
   scope :stopped, -> { with_state(:stopped) }
+
   scope :order_by_last_deployed_at, -> do
-    max_deployment_id_sql =
-      Deployment.select(Deployment.arel_table[:id].maximum)
-      .where(Deployment.arel_table[:environment_id].eq(arel_table[:id]))
-      .to_sql
     order(Gitlab::Database.nulls_first_order("(#{max_deployment_id_sql})", 'ASC'))
   end
+  scope :order_by_last_deployed_at_desc, -> do
+    order(Gitlab::Database.nulls_last_order("(#{max_deployment_id_sql})", 'DESC'))
+  end
+
   scope :in_review_folder, -> { where(environment_type: "review") }
   scope :for_name, -> (name) { where(name: name) }
   scope :preload_cluster, -> { preload(last_deployment: :cluster) }
@@ -90,6 +91,12 @@ class Environment < ApplicationRecord
     end
   end
 
+  def self.max_deployment_id_sql
+    Deployment.select(Deployment.arel_table[:id].maximum)
+    .where(Deployment.arel_table[:environment_id].eq(arel_table[:id]))
+    .to_sql
+  end
+
   def self.pluck_names
     pluck(:name)
   end

app/models/merge_request.rb

@@ -1122,22 +1122,18 @@ class MergeRequest < ApplicationRecord
     actual_head_pipeline.success?
   end
 
-  def environments_for(current_user)
+  def environments_for(current_user, latest: false)
     return [] unless diff_head_commit
 
-    @environments ||= Hash.new do |h, current_user|
-      envs = EnvironmentsFinder.new(target_project, current_user,
-        ref: target_branch, commit: diff_head_commit, with_tags: true).execute
+    envs = EnvironmentsFinder.new(target_project, current_user,
+      ref: target_branch, commit: diff_head_commit, with_tags: true, find_latest: latest).execute
 
-      if source_project
-        envs.concat EnvironmentsFinder.new(source_project, current_user,
-          ref: source_branch, commit: diff_head_commit).execute
-      end
-
-      h[current_user] = envs.uniq
+    if source_project
+      envs.concat EnvironmentsFinder.new(source_project, current_user,
+        ref: source_branch, commit: diff_head_commit, find_latest: latest).execute
     end
 
-    @environments[current_user]
+    envs.uniq
   end
 
   ##

changelogs/unreleased/sh-optimize-commit-is-ancestor-env.yml

+---
+title: Reduce CommitIsAncestor RPCs with environments
+merge_request: 21778
+author:
+type: performance

spec/finders/environments_finder_spec.rb

@@ -13,17 +13,22 @@ describe EnvironmentsFinder do
     end
 
     context 'tagged deployment' do
+      let(:environment_two) { create(:environment, project: project) }
+      # Environments need to include commits, so rewind two commits to fit
+      let(:commit) { project.commit('HEAD~2') }
+
       before do
-        create(:deployment, :success, environment: environment, ref: 'v1.1.0', tag: true, sha: project.commit.id)
+        create(:deployment, :success, environment: environment, ref: 'v1.0.0', tag: true, sha: project.commit.id)
+        create(:deployment, :success, environment: environment_two, ref: 'v1.1.0', tag: true, sha: project.commit('HEAD~1').id)
       end
 
       it 'returns environment when with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit, with_tags: true).execute)
-          .to contain_exactly(environment)
+        expect(described_class.new(project, user, ref: 'master', commit: commit, with_tags: true).execute)
+          .to contain_exactly(environment, environment_two)
       end
 
       it 'does not return environment when no with_tags is set' do
-        expect(described_class.new(project, user, ref: 'master', commit: project.commit).execute)
+        expect(described_class.new(project, user, ref: 'master', commit: commit).execute)
           .to be_empty
       end
 
@@ -31,6 +36,21 @@ describe EnvironmentsFinder do
         expect(described_class.new(project, user, ref: 'master', commit: project.commit('feature')).execute)
           .to be_empty
       end
+
+      it 'returns environment when with_tags is set' do
+        expect(described_class.new(project, user, ref: 'master', commit: commit, with_tags: true).execute)
+          .to contain_exactly(environment, environment_two)
+      end
+
+      # We expect two Gitaly calls: FindCommit, CommitIsAncestor
+      # This tests to ensure we don't call one CommitIsAncestor per environment
+      it 'only calls Gitaly twice when multiple environments are present', :request_store do
+        expect do
+          result = described_class.new(project, user, ref: 'master', commit: commit, with_tags: true, find_latest: true).execute
+
+          expect(result).to contain_exactly(environment_two)
+        end.to change { Gitlab::GitalyClient.get_request_count }.by(2)
+      end
     end
 
     context 'branch deployment' do

spec/models/environment_spec.rb

@@ -36,9 +36,13 @@ describe Environment, :use_clean_rails_memory_store_caching do
     let!(:deployment2) { create(:deployment, environment: environment2) }
     let!(:deployment3) { create(:deployment, environment: environment1) }
 
-    it 'returns the environments in order of having been last deployed' do
+    it 'returns the environments in ascending order of having been last deployed' do
       expect(project.environments.order_by_last_deployed_at.to_a).to eq([environment3, environment2, environment1])
     end
+
+    it 'returns the environments in descending order of having been last deployed' do
+      expect(project.environments.order_by_last_deployed_at_desc.to_a).to eq([environment1, environment2, environment3])
+    end
   end
 
   describe 'state machine' do

spec/models/merge_request_spec.rb

@@ -2322,6 +2322,10 @@ describe MergeRequest do
     let(:project)       { create(:project, :repository) }
     let(:user)          { project.creator }
     let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:source_branch) { merge_request.source_branch }
+    let(:target_branch) { merge_request.target_branch }
+    let(:source_oid) { project.commit(source_branch).id }
+    let(:target_oid) { project.commit(target_branch).id }
 
     before do
       merge_request.source_project.add_maintainer(user)
@@ -2332,13 +2336,21 @@ describe MergeRequest do
       let(:environments) { create_list(:environment, 3, project: project) }
 
       before do
-        create(:deployment, :success, environment: environments.first, ref: 'master', sha: project.commit('master').id)
-        create(:deployment, :success, environment: environments.second, ref: 'feature', sha: project.commit('feature').id)
+        create(:deployment, :success, environment: environments.first, ref: source_branch, sha: source_oid)
+        create(:deployment, :success, environment: environments.second, ref: target_branch, sha: target_oid)
       end
 
       it 'selects deployed environments' do
         expect(merge_request.environments_for(user)).to contain_exactly(environments.first)
       end
+
+      it 'selects latest deployed environment' do
+        latest_environment = create(:environment, project: project)
+        create(:deployment, :success, environment: latest_environment, ref: source_branch, sha: source_oid)
+
+        expect(merge_request.environments_for(user)).to eq([environments.first, latest_environment])
+        expect(merge_request.environments_for(user, latest: true)).to contain_exactly(latest_environment)
+      end
     end
 
     context 'with environments on source project' do