Commit dd660402 authored by Rémy Coutable's avatar Rémy Coutable

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

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

Closes #43140

See merge request gitlab-org/gitlab-ce!21500
parents 2ab67aeb eea1b885
...@@ -36,54 +36,47 @@ class Projects::RefsController < Projects::ApplicationController ...@@ -36,54 +36,47 @@ class Projects::RefsController < Projects::ApplicationController
end end
def logs_tree def logs_tree
@offset = if params[:offset].present? summary = ::Gitlab::TreeSummary.new(
params[:offset].to_i @commit,
else @project,
0 path: @path,
end 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, commits = summary.summarize
@logs = Gitlab::GitalyClient.allow_n_plus_1_calls do @more_log_url = more_url(summary.next_offset) if summary.more?
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
{
file_name: content.name,
commit: last_commit,
type: content.type,
commit_path: commit_path
}
end
end
offset = (@offset + @limit)
if contents.size > offset
@more_log_url = logs_file_project_ref_path(@project, @ref, @path || '', offset: offset)
end
respond_to do |format| respond_to do |format|
format.html { render_404 } format.html { render_404 }
format.json do 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 render json: @logs
end 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
end end
private 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 def validate_ref_id
return not_found! if params[:id].present? && params[:id] !~ Gitlab::PathRegex.git_reference_regex return not_found! if params[:id].present? && params[:id] !~ Gitlab::PathRegex.git_reference_regex
end end
......
...@@ -22,6 +22,7 @@ class Commit ...@@ -22,6 +22,7 @@ class Commit
attr_accessor :project, :author attr_accessor :project, :author
attr_accessor :redacted_description_html attr_accessor :redacted_description_html
attr_accessor :redacted_title_html attr_accessor :redacted_title_html
attr_accessor :redacted_full_title_html
attr_reader :gpg_commit attr_reader :gpg_commit
DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]
......
%span.str-truncated %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'
---
title: Bulk-render commit titles in the tree view to improve performance
merge_request: 21500
author:
type: performance
module Gitlab
class 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
...@@ -4,16 +4,30 @@ describe 'Projects tree', :js do ...@@ -4,16 +4,30 @@ describe 'Projects tree', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
# This commit has a known state on the master branch of gitlab-test
let(:test_sha) { '7975be0116940bf2ad4321f79d02a55c5f7779aa' }
before do before do
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
end end
it 'renders tree table without errors' do 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 wait_for_requests
expect(page).to have_selector('.tree-item') 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('.label-lfs', text: 'LFS')
expect(page).not_to have_selector('.flash-alert') expect(page).not_to have_selector('.flash-alert')
end end
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment