Merge branch 'fast_project_blob_path' into 'master'

Improve performance of tree rendering in repositories with a lot of items

See merge request gitlab-org/gitlab-ce!16511

10 files changed, 98 insertions, 46 deletions

diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb
index 910c9e9446f..b0f63de2fb8 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/_tree_row.html.haml_spec.rb
index 6a477c712ff..3353b7665e2 100644
--- a/spec/views/projects/tree/_blob_item.html.haml_spec.rb
+++ b/spec/views/projects/tree/_tree_row.html.haml_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe 'projects/tree/_blob_item' do
+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 }
@@ -31,10 +31,7 @@ describe 'projects/tree/_blob_item' do
     end
   end
 
-  def render_partial(blob_item)
-    render partial: 'projects/tree/blob_item', locals: {
-      blob_item: blob_item,
-      type: 'blob'
-    }
+  def render_partial(items)
+    render partial: 'projects/tree/tree_row', collection: [items].flatten
   end
 end