From 409f2f4dd2888f88df2293466c37c768b94068e5 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Wed, 17 Oct 2018 20:03:15 -0400 Subject: Improve performance of tree rendering in repositories with lots of items Rails is slow to generate paths dynamically especially when called hundreds/thousands of times. Also, rendering many partials hundreds of times can be slower. This change reduces the number of partials rendered and introduces two fast path methods to speed up path generation. --- app/helpers/icons_helper.rb | 2 + app/helpers/tree_helper.rb | 20 ++++++++-- app/views/projects/tree/_blob_item.html.haml | 12 ------ app/views/projects/tree/_spinner.html.haml | 3 -- app/views/projects/tree/_submodule_item.html.haml | 6 --- app/views/projects/tree/_tree_item.html.haml | 9 ----- app/views/projects/tree/_tree_row.html.haml | 33 +++++++++++++--- changelogs/unreleased/fast_project_blob_path.yml | 5 +++ spec/helpers/tree_helper_spec.rb | 45 +++++++++++++++++++++- .../projects/tree/_blob_item.html.haml_spec.rb | 40 ------------------- .../projects/tree/_tree_row.html.haml_spec.rb | 37 ++++++++++++++++++ 11 files changed, 132 insertions(+), 80 deletions(-) delete mode 100644 app/views/projects/tree/_blob_item.html.haml delete mode 100644 app/views/projects/tree/_spinner.html.haml delete mode 100644 app/views/projects/tree/_submodule_item.html.haml delete mode 100644 app/views/projects/tree/_tree_item.html.haml create mode 100644 changelogs/unreleased/fast_project_blob_path.yml delete mode 100644 spec/views/projects/tree/_blob_item.html.haml_spec.rb create mode 100644 spec/views/projects/tree/_tree_row.html.haml_spec.rb diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb index 037004327b9..56ffd145f65 100644 --- a/app/helpers/icons_helper.rb +++ b/app/helpers/icons_helper.rb @@ -109,6 +109,8 @@ module IconsHelper def file_type_icon_class(type, mode, name) if type == 'folder' icon_class = 'folder' + elsif type == 'archive' + icon_class = 'archive' elsif mode == '120000' icon_class = 'share' else diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index 6d2da5699fb..78a11616d4c 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -31,11 +31,21 @@ module TreeHelper # mode - File unix mode # name - File name def tree_icon(type, mode, name) - icon("#{file_type_icon_class(type, mode, name)} fw") + icon([file_type_icon_class(type, mode, name), 'fw']) end - def tree_hex_class(content) - "file_#{hexdigest(content.name)}" + # Using Rails `*_path` methods can be slow, especially when generating + # many paths, as with a repository tree that has thousands of items. + def fast_project_blob_path(project, blob_path) + Addressable::URI.escape( + File.join(relative_url_root, project.path_with_namespace, 'blob', blob_path) + ) + end + + def fast_project_tree_path(project, tree_path) + Addressable::URI.escape( + File.join(relative_url_root, project.path_with_namespace, 'tree', tree_path) + ) end # Simple shortcut to File.join @@ -142,4 +152,8 @@ module TreeHelper def selected_branch @branch_name || tree_edit_branch end + + def relative_url_root + Gitlab.config.gitlab.relative_url_root.presence || '/' + end end diff --git a/app/views/projects/tree/_blob_item.html.haml b/app/views/projects/tree/_blob_item.html.haml deleted file mode 100644 index f79f3af36d4..00000000000 --- a/app/views/projects/tree/_blob_item.html.haml +++ /dev/null @@ -1,12 +0,0 @@ -- is_lfs_blob = @lfs_blob_ids.include?(blob_item.id) -%tr{ class: "tree-item #{tree_hex_class(blob_item)}" } - %td.tree-item-file-name - = tree_icon(type, blob_item.mode, blob_item.name) - - file_name = blob_item.name - = link_to project_blob_path(@project, tree_join(@id || @commit.id, blob_item.name)), class: 'str-truncated', title: file_name do - %span= file_name - - if is_lfs_blob - %span.badge.label-lfs.prepend-left-5 LFS - %td.d-none.d-sm-table-cell.tree-commit - %td.tree-time-ago.cgray.text-right - = render 'projects/tree/spinner' diff --git a/app/views/projects/tree/_spinner.html.haml b/app/views/projects/tree/_spinner.html.haml deleted file mode 100644 index b47ad0f41e4..00000000000 --- a/app/views/projects/tree/_spinner.html.haml +++ /dev/null @@ -1,3 +0,0 @@ -%span.log_loading.hide - %i.fa.fa-spinner.fa-spin - Loading commit data... diff --git a/app/views/projects/tree/_submodule_item.html.haml b/app/views/projects/tree/_submodule_item.html.haml deleted file mode 100644 index e563c8c4036..00000000000 --- a/app/views/projects/tree/_submodule_item.html.haml +++ /dev/null @@ -1,6 +0,0 @@ -%tr.tree-item - %td.tree-item-file-name - %i.fa.fa-archive.fa-fw - = submodule_link(submodule_item, @ref) - %td - %td.d-none.d-sm-table-cell diff --git a/app/views/projects/tree/_tree_item.html.haml b/app/views/projects/tree/_tree_item.html.haml deleted file mode 100644 index ce0cd95b468..00000000000 --- a/app/views/projects/tree/_tree_item.html.haml +++ /dev/null @@ -1,9 +0,0 @@ -%tr{ class: "tree-item #{tree_hex_class(tree_item)}" } - %td.tree-item-file-name - = tree_icon(type, tree_item.mode, tree_item.name) - - path = flatten_tree(@path, tree_item) - = link_to project_tree_path(@project, tree_join(@id || @commit.id, path)), class: 'str-truncated', title: path do - %span= path - %td.d-none.d-sm-table-cell.tree-commit - %td.tree-time-ago.text-right - = render 'projects/tree/spinner' diff --git a/app/views/projects/tree/_tree_row.html.haml b/app/views/projects/tree/_tree_row.html.haml index 0a5c6f048f7..8a27ea66523 100644 --- a/app/views/projects/tree/_tree_row.html.haml +++ b/app/views/projects/tree/_tree_row.html.haml @@ -1,6 +1,27 @@ -- if tree_row.type == :tree - = render partial: 'projects/tree/tree_item', object: tree_row, as: 'tree_item', locals: { type: 'folder' } -- elsif tree_row.type == :blob - = render partial: 'projects/tree/blob_item', object: tree_row, as: 'blob_item', locals: { type: 'file' } -- elsif tree_row.type == :commit - = render partial: 'projects/tree/submodule_item', object: tree_row, as: 'submodule_item' +- tree_row_name = tree_row.name +- tree_row_type = tree_row.type + +%tr{ class: "tree-item file_#{hexdigest(tree_row_name)}" } + %td.tree-item-file-name + - if tree_row_type == :tree + = tree_icon('folder', tree_row.mode, tree_row.name) + - path = flatten_tree(@path, tree_row) + %a.str-truncated{ href: fast_project_tree_path(@project, tree_join(@id || @commit.id, path)), title: path } + %span= path + + - elsif tree_row_type == :blob + = tree_icon('file', tree_row.mode, tree_row_name) + %a.str-truncated{ href: fast_project_blob_path(@project, tree_join(@id || @commit.id, tree_row_name)), title: tree_row_name } + %span= tree_row_name + - if @lfs_blob_ids.include?(tree_row.id) + %span.badge.label-lfs.prepend-left-5 LFS + + - elsif tree_row_type == :commit + = tree_icon('archive', tree_row.mode, tree_row.name) + = submodule_link(tree_row, @ref) + + %td.d-none.d-sm-table-cell.tree-commit + %td.tree-time-ago.text-right + %span.log_loading.hide + %i.fa.fa-spinner.fa-spin + Loading commit data... diff --git a/changelogs/unreleased/fast_project_blob_path.yml b/changelogs/unreleased/fast_project_blob_path.yml new file mode 100644 index 00000000000..b56c9d9cf59 --- /dev/null +++ b/changelogs/unreleased/fast_project_blob_path.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of tree rendering in repositories with lots of items +merge_request: +author: +type: performance diff --git a/spec/helpers/tree_helper_spec.rb b/spec/helpers/tree_helper_spec.rb index ffdf6561a53..ab4566e261b 100644 --- a/spec/helpers/tree_helper_spec.rb +++ b/spec/helpers/tree_helper_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe TreeHelper do let(:project) { create(:project, :repository) } let(:repository) { project.repository } - let(:sha) { 'ce369011c189f62c815f5971d096b26759bab0d1' } + let(:sha) { 'c1c67abbaf91f624347bb3ae96eabe3a1b742478' } describe '.render_tree' do before do @@ -32,6 +32,49 @@ describe TreeHelper do end end + describe '.fast_project_blob_path' do + it 'generates the same path as project_blob_path' do + blob_path = repository.tree(sha, 'with space').entries.first.path + fast_path = fast_project_blob_path(project, blob_path) + std_path = project_blob_path(project, blob_path) + + expect(fast_path).to eq(std_path) + end + + it 'generates the same path with encoded file names' do + tree = repository.tree(sha, 'encoding') + blob_path = tree.entries.find { |entry| entry.path == 'encoding/ใƒ†ใ‚นใƒˆ.txt' }.path + fast_path = fast_project_blob_path(project, blob_path) + std_path = project_blob_path(project, blob_path) + + expect(fast_path).to eq(std_path) + end + + it 'respects a configured relative URL' do + allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root') + blob_path = repository.tree(sha, '').entries.first.path + fast_path = fast_project_blob_path(project, blob_path) + + expect(fast_path).to start_with('/gitlab/root') + end + end + + describe '.fast_project_tree_path' do + let(:tree_path) { repository.tree(sha, 'with space').path } + let(:fast_path) { fast_project_tree_path(project, tree_path) } + let(:std_path) { project_tree_path(project, tree_path) } + + it 'generates the same path as project_tree_path' do + expect(fast_path).to eq(std_path) + end + + it 'respects a configured relative URL' do + allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root') + + expect(fast_path).to start_with('/gitlab/root') + end + end + describe 'flatten_tree' do let(:tree) { repository.tree(sha, 'files') } let(:root_path) { 'files' } diff --git a/spec/views/projects/tree/_blob_item.html.haml_spec.rb b/spec/views/projects/tree/_blob_item.html.haml_spec.rb deleted file mode 100644 index 6a477c712ff..00000000000 --- a/spec/views/projects/tree/_blob_item.html.haml_spec.rb +++ /dev/null @@ -1,40 +0,0 @@ -require 'spec_helper' - -describe 'projects/tree/_blob_item' do - let(:project) { create(:project, :repository) } - let(:repository) { project.repository } - let(:blob_item) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID, 'files/ruby').first } - - before do - assign(:project, project) - assign(:repository, repository) - assign(:id, File.join('master', '')) - assign(:lfs_blob_ids, []) - end - - it 'renders blob item' do - render_partial(blob_item) - - expect(rendered).to have_content(blob_item.name) - expect(rendered).not_to have_selector('.label-lfs', text: 'LFS') - end - - describe 'LFS blob' do - before do - assign(:lfs_blob_ids, [blob_item].map(&:id)) - - render_partial(blob_item) - end - - it 'renders LFS badge' do - expect(rendered).to have_selector('.label-lfs', text: 'LFS') - end - end - - def render_partial(blob_item) - render partial: 'projects/tree/blob_item', locals: { - blob_item: blob_item, - type: 'blob' - } - end -end diff --git a/spec/views/projects/tree/_tree_row.html.haml_spec.rb b/spec/views/projects/tree/_tree_row.html.haml_spec.rb new file mode 100644 index 00000000000..3353b7665e2 --- /dev/null +++ b/spec/views/projects/tree/_tree_row.html.haml_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe 'projects/tree/_tree_row' do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:blob_item) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID, 'files/ruby').first } + + before do + assign(:project, project) + assign(:repository, repository) + assign(:id, File.join('master', '')) + assign(:lfs_blob_ids, []) + end + + it 'renders blob item' do + render_partial(blob_item) + + expect(rendered).to have_content(blob_item.name) + expect(rendered).not_to have_selector('.label-lfs', text: 'LFS') + end + + describe 'LFS blob' do + before do + assign(:lfs_blob_ids, [blob_item].map(&:id)) + + render_partial(blob_item) + end + + it 'renders LFS badge' do + expect(rendered).to have_selector('.label-lfs', text: 'LFS') + end + end + + def render_partial(items) + render partial: 'projects/tree/tree_row', collection: [items].flatten + end +end -- cgit v1.2.1