Merge branch 'ee-43140-reduce-logs-tree-load' into 'master'

Bulk-render commit titles in the tree view to improve performance: EE version

See merge request gitlab-org/gitlab-ee!7234

app/controllers/projects/refs_controller.rb

 class Projects::RefsController < Projects::ApplicationController
   include ExtractsPath
   include TreeHelper
-  include PathLocksHelper
 
   before_action :require_non_empty_project
   before_action :validate_ref_id
@@ -37,58 +36,47 @@ class Projects::RefsController < Projects::ApplicationController
   end
 
   def logs_tree
-    @offset = if params[:offset].present?
-                params[:offset].to_i
-              else
-                0
-              end
+    summary = ::Gitlab::TreeSummary.new(
+      @commit,
+      @project,
+      path: @path,
+      offset: params[:offset],
+      limit: 25
+    )
 
-    @limit = 25
-
-    @path = params[:path]
-
-    contents = []
-    contents.push(*tree.trees)
-    contents.push(*tree.blobs)
-    contents.push(*tree.submodules)
-
-    # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
-    @logs = Gitlab::GitalyClient.allow_n_plus_1_calls do
-      contents[@offset, @limit].to_a.map do |content|
-        file = @path ? File.join(@path, content.name) : content.name
-        last_commit = @repo.last_commit_for_path(@commit.id, file)
-        commit_path = project_commit_path(@project, last_commit) if last_commit
-        path_lock = @project.find_path_lock(file)
-
-        {
-          file_name: content.name,
-          commit: last_commit,
-          type: content.type,
-          commit_path: commit_path,
-          lock_label: path_lock && text_label_for_lock(path_lock, file)
-        }
-      end
-    end
-
-    offset = @offset + @limit
-
-    if contents.size > offset
-      @more_log_url = logs_file_project_ref_path(@project, @ref, @path || '', offset: offset)
-    end
+    @logs, commits = summary.summarize
+    @more_log_url = more_url(summary.next_offset) if summary.more?
 
     respond_to do |format|
       format.html { render_404 }
       format.json do
-        response.headers["More-Logs-Url"] = @more_log_url
-
+        response.headers["More-Logs-Url"] = @more_log_url if summary.more?
         render json: @logs
       end
-      format.js
+
+      # The commit titles must be rendered and redacted before being shown.
+      # Doing it here allows us to apply performance optimizations that avoid
+      # N+1 problems
+      format.js do
+        prerender_commit_full_titles!(commits)
+      end
     end
   end
 
   private
 
+  def more_url(offset)
+    logs_file_project_ref_path(@project, @ref, @path, offset: offset)
+  end
+
+  def prerender_commit_full_titles!(commits)
+    # Preload commit authors as they are used in rendering
+    commits.each(&:lazy_author)
+
+    renderer = Banzai::ObjectRenderer.new(user: current_user, default_project: @project)
+    renderer.render(commits, :full_title)
+  end
+
   def validate_ref_id
     return not_found! if params[:id].present? && params[:id] !~ Gitlab::PathRegex.git_reference_regex
   end

app/models/commit.rb

@@ -22,6 +22,7 @@ class Commit
   attr_accessor :project, :author
   attr_accessor :redacted_description_html
   attr_accessor :redacted_title_html
+  attr_accessor :redacted_full_title_html
   attr_reader :gpg_commit
 
   DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]

app/views/projects/tree/_tree_commit_column.html.haml

 %span.str-truncated
-  = link_to_markdown commit.full_title, project_commit_path(@project, commit.id), class: "tree-commit-link"
+  = link_to_html commit.redacted_full_title_html, project_commit_path(@project, commit.id), class: 'tree-commit-link'

changelogs/unreleased/43140-reduce-logs-tree-load.yml

+---
+title: Bulk-render commit titles in the tree view to improve performance
+merge_request: 21500
+author:
+type: performance

ee/lib/ee/gitlab/tree_summary.rb

+module EE
+  module Gitlab
+    module TreeSummary
+      extend ::Gitlab::Utils::Override
+
+      include ::PathLocksHelper
+
+      override :summarize
+      def summarize
+        summary, commits = super
+        summary.tap { |summary| fill_path_locks!(summary) }
+
+        [summary, commits]
+      end
+
+      private
+
+      # FIXME: Loading the path locks from the database is an N+1 problem
+      # https://gitlab.com/gitlab-org/gitlab-ee/issues/7481
+      def fill_path_locks!(entries)
+        entries.each do |entry|
+          path = entry_path(entry)
+          path_lock = project.find_path_lock(path)
+
+          entry[:lock_label] = path_lock && text_label_for_lock(path_lock, path)
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/tree_summary_spec.rb

+require 'spec_helper'
+
+describe Gitlab::TreeSummary do
+  let(:project) { create(:project, :custom_repo, files: { 'a.txt' => '' }) }
+  let(:commit) { project.repository.head_commit }
+  let!(:path_lock) { create(:path_lock, project: project, path: 'a.txt') }
+
+  describe '#summarize (entries)' do
+    subject { described_class.new(commit, project).summarize.first }
+
+    it 'includes path locks in entries' do
+      is_expected.to contain_exactly(
+        a_hash_including(file_name: 'a.txt', lock_label: "Locked by #{path_lock.user.name}")
+      )
+    end
+  end
+end

lib/gitlab/tree_summary.rb

+module Gitlab
+  class TreeSummary
+    prepend ::EE::Gitlab::TreeSummary
+
+    include ::Gitlab::Utils::StrongMemoize
+
+    attr_reader :commit, :project, :path, :offset, :limit
+
+    attr_reader :resolved_commits
+    private :resolved_commits
+
+    def initialize(commit, project, params = {})
+      @commit = commit
+      @project = project
+
+      @path = params.fetch(:path, nil).presence
+      @offset = params.fetch(:offset, 0).to_i
+      @limit = (params.fetch(:limit, 25) || 25).to_i
+
+      # Ensure that if multiple tree entries share the same last commit, they share
+      # a ::Commit instance. This prevents us from rendering the same commit title
+      # multiple times
+      @resolved_commits = {}
+    end
+
+    # Creates a summary of the tree entries for a commit, within the window of
+    # entries defined by the offset and limit parameters. This consists of two
+    # return values:
+    #
+    #     - An Array of Hashes containing the following keys:
+    #         - file_name:   The full path of the tree entry
+    #         - type:        One of :blob, :tree, or :submodule
+    #         - commit:      The last ::Commit to touch this entry in the tree
+    #         - commit_path: URI of the commit in the web interface
+    #     - An Array of the unique ::Commit objects in the first value
+    def summarize
+      summary = contents
+        .map { |content| build_entry(content) }
+        .tap { |summary| fill_last_commits!(summary) }
+
+      [summary, commits]
+    end
+
+    # Does the tree contain more entries after the given offset + limit?
+    def more?
+      all_contents[next_offset].present?
+    end
+
+    # The offset of the next batch of tree entries. If more? returns false, this
+    # batch will be empty
+    def next_offset
+      [all_contents.size + 1, offset + limit].min
+    end
+
+    private
+
+    def contents
+      all_contents[offset, limit]
+    end
+
+    def commits
+      resolved_commits.values
+    end
+
+    def repository
+      project.repository
+    end
+
+    def entry_path(entry)
+      File.join(*[path, entry[:file_name]].compact)
+    end
+
+    def build_entry(entry)
+      { file_name: entry.name, type: entry.type }
+    end
+
+    def fill_last_commits!(entries)
+      # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
+      Gitlab::GitalyClient.allow_n_plus_1_calls do
+        entries.each do |entry|
+          raw_commit = repository.last_commit_for_path(commit.id, entry_path(entry))
+
+          if raw_commit
+            commit = resolve_commit(raw_commit)
+
+            entry[:commit] = commit
+            entry[:commit_path] =  commit_path(commit)
+          end
+        end
+      end
+    end
+
+    def resolve_commit(raw_commit)
+      return nil unless raw_commit.present?
+
+      resolved_commits[raw_commit.id] ||= ::Commit.new(raw_commit, project)
+    end
+
+    def commit_path(commit)
+      Gitlab::Routing.url_helpers.project_commit_path(project, commit)
+    end
+
+    def all_contents
+      strong_memoize(:all_contents) do
+        [
+          *tree.trees,
+          *tree.blobs,
+          *tree.submodules
+        ]
+      end
+    end
+
+    def tree
+      strong_memoize(:tree) { repository.tree(commit.id, path) }
+    end
+  end
+end

spec/features/projects/tree/tree_show_spec.rb

@@ -4,16 +4,30 @@ describe 'Projects tree', :js do
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository) }
 
+  # This commit has a known state on the master branch of gitlab-test
+  let(:test_sha) { '7975be0116940bf2ad4321f79d02a55c5f7779aa' }
+
   before do
     project.add_maintainer(user)
     sign_in(user)
   end
 
   it 'renders tree table without errors' do
-    visit project_tree_path(project, 'master')
+    visit project_tree_path(project, test_sha)
+    wait_for_requests
+
+    expect(page).to have_selector('.tree-item')
+    expect(page).to have_content('add tests for .gitattributes custom highlighting')
+    expect(page).not_to have_selector('.flash-alert')
+    expect(page).not_to have_selector('.label-lfs', text: 'LFS')
+  end
+
+  it 'renders tree table for a subtree without errors' do
+    visit project_tree_path(project, File.join(test_sha, 'files'))
     wait_for_requests
 
     expect(page).to have_selector('.tree-item')
+    expect(page).to have_content('add spaces in whitespace file')
     expect(page).not_to have_selector('.label-lfs', text: 'LFS')
     expect(page).not_to have_selector('.flash-alert')
   end

spec/lib/gitlab/tree_summary_spec.rb

+require 'spec_helper'
+
+describe Gitlab::TreeSummary do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:project) { create(:project, :empty_repo) }
+  let(:repo) { project.repository }
+  let(:commit) { repo.head_commit }
+
+  let(:path) { nil }
+  let(:offset) { nil }
+  let(:limit) { nil }
+
+  subject(:summary) { described_class.new(commit, project, path: path, offset: offset, limit: limit) }
+
+  describe '#initialize' do
+    it 'defaults offset to 0' do
+      expect(summary.offset).to eq(0)
+    end
+
+    it 'defaults limit to 25' do
+      expect(summary.limit).to eq(25)
+    end
+  end
+
+  describe '#summarize' do
+    let(:project) { create(:project, :custom_repo, files: { 'a.txt' => '' }) }
+
+    subject(:summarized) { summary.summarize }
+
+    it 'returns an array of entries, and an array of commits' do
+      expect(summarized).to be_a(Array)
+      expect(summarized.size).to eq(2)
+
+      entries, commits = *summarized
+      aggregate_failures do
+        expect(entries).to contain_exactly(
+          a_hash_including(file_name: 'a.txt', commit: have_attributes(id: commit.id))
+        )
+
+        expect(commits).to match_array(entries.map { |entry| entry[:commit] })
+      end
+    end
+  end
+
+  describe '#summarize (entries)' do
+    let(:limit) { 2 }
+
+    custom_files = {
+      'a.txt' => '',
+      'b.txt' => '',
+      'directory/c.txt' => ''
+    }
+
+    let(:project) { create(:project, :custom_repo, files: custom_files) }
+    let(:commit) { repo.head_commit }
+
+    subject(:entries) { summary.summarize.first }
+
+    it 'summarizes the entries within the window' do
+      is_expected.to contain_exactly(
+        a_hash_including(type: :tree, file_name: 'directory'),
+        a_hash_including(type: :blob, file_name: 'a.txt')
+        # b.txt is excluded by the limit
+      )
+    end
+
+    it 'references the commit and commit path in entries' do
+      entry = entries.first
+      expected_commit_path = Gitlab::Routing.url_helpers.project_commit_path(project, commit)
+
+      expect(entry[:commit]).to be_a(::Commit)
+      expect(entry[:commit_path]).to eq expected_commit_path
+    end
+
+    context 'in a good subdirectory' do
+      let(:path) { 'directory' }
+
+      it 'summarizes the entries in the subdirectory' do
+        is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'c.txt'))
+      end
+    end
+
+    context 'in a non-existent subdirectory' do
+      let(:path) { 'tmp' }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'custom offset and limit' do
+      let(:offset) { 2 }
+
+      it 'returns entries from the offset' do
+        is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'b.txt'))
+      end
+    end
+  end
+
+  describe '#summarize (commits)' do
+    # This is a commit in the master branch of the gitlab-test repository that
+    # satisfies certain assumptions these tests depend on
+    let(:test_commit_sha) { '7975be0116940bf2ad4321f79d02a55c5f7779aa' }
+    let(:whitespace_commit_sha) { '66eceea0db202bb39c4e445e8ca28689645366c5' }
+
+    let(:project) { create(:project, :repository) }
+    let(:commit) { repo.commit(test_commit_sha) }
+    let(:limit) { nil }
+    let(:entries) { summary.summarize.first }
+
+    subject(:commits) do
+      summary.summarize.last
+    end
+
+    it 'returns an Array of ::Commit objects' do
+      is_expected.not_to be_empty
+      is_expected.to all(be_kind_of(::Commit))
+    end
+
+    it 'deduplicates commits when multiple entries reference the same commit' do
+      expect(commits.size).to be < entries.size
+    end
+
+    context 'in a subdirectory' do
+      let(:path) { 'files' }
+
+      it 'returns commits for entries in the subdirectory' do
+        expect(commits).to satisfy_one { |c| c.id == whitespace_commit_sha }
+      end
+    end
+  end
+
+  describe '#more?' do
+    let(:path) { 'tmp/more' }
+
+    where(:num_entries, :offset, :limit, :expected_result) do
+      0 | 0 | 0 | false
+      0 | 0 | 1 | false
+
+      1 | 0 | 0 | true
+      1 | 0 | 1 | false
+      1 | 1 | 0 | false
+      1 | 1 | 1 | false
+
+      2 | 0 | 0 | true
+      2 | 0 | 1 | true
+      2 | 0 | 2 | false
+      2 | 0 | 3 | false
+      2 | 1 | 0 | true
+      2 | 1 | 1 | false
+      2 | 2 | 0 | false
+      2 | 2 | 1 | false
+    end
+
+    with_them do
+      before do
+        create_file('dummy', path: 'other') if num_entries.zero?
+        1.upto(num_entries) { |n| create_file(n, path: path) }
+      end
+
+      subject { summary.more? }
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+
+  describe '#next_offset' do
+    let(:path) { 'tmp/next_offset' }
+
+    where(:num_entries, :offset, :limit, :expected_result) do
+      0 | 0 | 0 | 0
+      0 | 0 | 1 | 1
+      0 | 1 | 0 | 1
+      0 | 1 | 1 | 1
+
+      1 | 0 | 0 | 0
+      1 | 0 | 1 | 1
+      1 | 1 | 0 | 1
+      1 | 1 | 1 | 2
+    end
+
+    with_them do
+      before do
+        create_file('dummy', path: 'other')  if num_entries.zero?
+        1.upto(num_entries) { |n| create_file(n, path: path) }
+      end
+
+      subject { summary.next_offset }
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+
+  def create_file(unique, path:)
+    repo.create_file(
+      project.creator,
+      "#{path}/file-#{unique}.txt",
+      'content',
+      message: "Commit message #{unique}",
+      branch_name: 'master'
+    )
+  end
+end