Denormalize commits count for merge request diffs

For each MR diff an extra 'SELECT COUNT()' is executed
to get number of commits for the diff. Overall time to get counts for
all MR diffs may be quite expensive. To speed up loading of MR info,
information about number of commits is stored in a MR diff's extra column.

Closes #38068

app/models/merge_request_diff.rb

@@ -49,6 +49,7 @@ class MergeRequestDiff < ActiveRecord::Base
     ensure_commit_shas
     save_commits
     save_diffs
+    save
     keep_around_commits
   end
@@ -56,7 +57,6 @@ class MergeRequestDiff < ActiveRecord::Base
     self.start_commit_sha ||= merge_request.target_branch_sha
     self.head_commit_sha  ||= merge_request.source_branch_sha
     self.base_commit_sha  ||= find_base_sha
-    save
   end
   # Override head_commit_sha to keep compatibility with merge request diff
@@ -195,7 +195,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
   def commits_count
-    merge_request_diff_commits.size
+    super || merge_request_diff_commits.size
   end
   private
@@ -264,13 +264,16 @@ class MergeRequestDiff < ActiveRecord::Base
       new_attributes[:state] = :overflow if diff_collection.overflow?
     end
-    update(new_attributes)
+    assign_attributes(new_attributes)
   end
   def save_commits
     MergeRequestDiffCommit.create_bulk(self.id, compare.commits.reverse)
-    merge_request_diff_commits.reload
+    # merge_request_diff_commits.reload is preferred way to reload associated
+    # objects but it returns cached result for some reason in this case
+    commits = merge_request_diff_commits(true)
+    self.commits_count = commits.size
   end
   def repository

changelogs/unreleased/38068-commits-count.yml

+---
+title: Store number of commits in merge_request_diffs table.
+merge_request:
+author:
+type: performance

db/migrate/20180105212544_add_commits_count_to_merge_request_diff.rb

+class AddCommitsCountToMergeRequestDiff < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'AddMergeRequestDiffCommitsCount'.freeze
+  BATCH_SIZE = 5000
+  DELAY_INTERVAL = 5.minutes.to_i
+
+  class MergeRequestDiff < ActiveRecord::Base
+    self.table_name = 'merge_request_diffs'
+
+    include ::EachBatch
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    add_column :merge_request_diffs, :commits_count, :integer
+
+    say 'Populating the MergeRequestDiff `commits_count`'
+
+    queue_background_migration_jobs_by_range_at_intervals(MergeRequestDiff, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    remove_column :merge_request_diffs, :commits_count
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20171230123729) do
+ActiveRecord::Schema.define(version: 20180105212544) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1044,6 +1044,7 @@ ActiveRecord::Schema.define(version: 20171230123729) do
     t.string "real_size"
     t.string "head_commit_sha"
     t.string "start_commit_sha"
+    t.integer "commits_count"
   end
   add_index "merge_request_diffs", ["merge_request_id", "id"], name: "index_merge_request_diffs_on_merge_request_id_and_id", using: :btree

lib/gitlab/background_migration/add_merge_request_diff_commits_count.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+# rubocop:disable Metrics/LineLength
+
+module Gitlab
+  module BackgroundMigration
+    class AddMergeRequestDiffCommitsCount
+      class MergeRequestDiff < ActiveRecord::Base
+        self.table_name = 'merge_request_diffs'
+      end
+
+      def perform(start_id, stop_id)
+        Rails.logger.info("Setting commits_count for merge request diffs: #{start_id} - #{stop_id}")
+
+        update = '
+          commits_count = (
+            SELECT count(*)
+            FROM merge_request_diff_commits
+            WHERE merge_request_diffs.id = merge_request_diff_commits.merge_request_diff_id
+          )'.squish
+
+        MergeRequestDiff.where(id: start_id..stop_id).update_all(update)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/add_merge_request_diff_commits_count_spec.rb

+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::AddMergeRequestDiffCommitsCount, :migration, schema: 20180105212544 do
+  let(:projects_table) { table(:projects) }
+  let(:merge_requests_table) { table(:merge_requests) }
+  let(:merge_request_diffs_table) { table(:merge_request_diffs) }
+  let(:merge_request_diff_commits_table) { table(:merge_request_diff_commits) }
+
+  let(:project) { projects_table.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce') }
+  let(:merge_request) do
+    merge_requests_table.create!(target_project_id: project.id,
+                                 target_branch: 'master',
+                                 source_project_id: project.id,
+                                 source_branch: 'mr name',
+                                 title: 'mr name')
+  end
+
+  def create_diff!(name, commits: 0)
+    mr_diff = merge_request_diffs_table.create!(
+      merge_request_id: merge_request.id)
+
+    commits.times do |i|
+      merge_request_diff_commits_table.create!(
+        merge_request_diff_id: mr_diff.id,
+        relative_order: i, sha: i)
+    end
+
+    mr_diff
+  end
+
+  describe '#perform' do
+    it 'migrates diffs that have no commits' do
+      diff = create_diff!('with_multiple_commits', commits: 0)
+
+      subject.perform(diff.id, diff.id)
+
+      expect(diff.reload.commits_count).to eq(0)
+    end
+
+    it 'migrates multiple diffs to the correct values' do
+      diffs = Array.new(3).map.with_index { |_, i| create_diff!(i, commits: 3) }
+
+      subject.perform(diffs.first.id, diffs.last.id)
+
+      diffs.each do |diff|
+        expect(diff.reload.commits_count).to eq(3)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb

@@ -10,6 +10,11 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :t
     let(:merge_request_diff) { MergeRequest.find(merge_request.id).create_merge_request_diff }
     let(:updated_merge_request_diff) { MergeRequestDiff.find(merge_request_diff.id) }
+    before do
+      allow_any_instance_of(MergeRequestDiff)
+        .to receive(:commits_count=).and_return(nil)
+    end
+
     def diffs_to_hashes(diffs)
       diffs.as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS).map(&:with_indifferent_access)
     end

spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb

 require 'rails_helper'
 describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do
+  # commits_count attribute is added in a next migration
+  before do
+    allow_any_instance_of(MergeRequestDiff)
+      .to receive(:commits_count=).and_return(nil)
+  end
+
   describe '#perform' do
     let(:mr_with_event) { create(:merge_request) }
     let!(:merged_event) { create(:event, :merged, target: mr_with_event) }

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -180,6 +180,7 @@ MergeRequestDiff:
 - real_size
 - head_commit_sha
 - start_commit_sha
+- commits_count
 MergeRequestDiffCommit:
 - merge_request_diff_id
 - relative_order

spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb

@@ -2,6 +2,12 @@ require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb')
 describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do
+  # commits_count attribute is added in a next migration
+  before do
+    allow_any_instance_of(MergeRequestDiff)
+      .to receive(:commits_count=).and_return(nil)
+  end
+
   let!(:mrs) { create_list(:merge_request, 3) }
   it 'correctly schedules background migrations' do