Merge branch '217572-make-blame-view-faster' into 'master'

Speed up the BlameController: view part

See merge request gitlab-org/gitlab!33283

app/controllers/projects/blame_controller.rb

@@ -20,6 +20,7 @@ class Projects::BlameController < Projects::ApplicationController
     environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
 
-    @blame_groups = Gitlab::Blame.new(@blob, @commit).groups
+    @blame = Gitlab::Blame.new(@blob, @commit)
+    @blame = Gitlab::View::Presenter::Factory.new(@blame, project: @project, path: @path).fabricate!
   end
 end

app/presenters/gitlab/blame_presenter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class BlamePresenter < Gitlab::View::Presenter::Simple
+    include ActionView::Helpers::UrlHelper
+    include ActionView::Helpers::TranslationHelper
+    include ActionView::Context
+    include AvatarsHelper
+    include BlameHelper
+    include CommitsHelper
+    include ApplicationHelper
+    include TreeHelper
+    include IconsHelper
+
+    presents :blame
+
+    CommitData = Struct.new(
+      :author_avatar,
+      :age_map_class,
+      :commit_link,
+      :commit_author_link,
+      :project_blame_link,
+      :time_ago_tooltip)
+
+    def initialize(subject, **attributes)
+      super
+
+      @commits = {}
+      precalculate_data_by_commit!
+    end
+
+    def groups
+      @groups ||= blame.groups
+    end
+
+    def commit_data(commit)
+      @commits[commit.id] ||= get_commit_data(commit)
+    end
+
+    private
+
+    # Huge source files with a high churn rate (e.g. 'locale/gitlab.pot') could have
+    # 10x times more blame groups than unique commits across all the groups.
+    # That means we could cache per-commit data we need
+    # to avoid recalculating it multiple times.
+    # For such files, it could significantly improve the performance of the Blame.
+    def precalculate_data_by_commit!
+      groups.each { |group| commit_data(group[:commit]) }
+    end
+
+    def get_commit_data(commit)
+      CommitData.new.tap do |data|
+        data.author_avatar = author_avatar(commit, size: 36, has_tooltip: false)
+        data.age_map_class = age_map_class(commit.committed_date, project_duration)
+        data.commit_link = link_to commit.title, project_commit_path(project, commit.id), class: "cdark", title: commit.title
+        data.commit_author_link = commit_author_link(commit, avatar: false)
+        data.project_blame_link = project_blame_link(commit)
+        data.time_ago_tooltip = time_ago_with_tooltip(commit.committed_date)
+      end
+    end
+
+    def project_blame_link(commit)
+      previous_commit_id = commit.parent_id
+      return unless previous_commit_id
+
+      link_to project_blame_path(project, tree_join(previous_commit_id, path)),
+        title: _('View blame prior to this change'),
+        aria: { label: _('View blame prior to this change') },
+        data: { toggle: 'tooltip', placement: 'right', container: 'body' } do
+          versions_sprite_icon
+        end
+    end
+
+    def project_duration
+      @project_duration ||= age_map_duration(groups, project)
+    end
+
+    def versions_sprite_icon
+      @versions_sprite_icon ||= sprite_icon('doc-versions', size: 16, css_class: 'doc-versions align-text-bottom')
+    end
+  end
+end

app/views/projects/blame/_blame_group.html.haml

+%tr
+  %td.blame-commit{ class: commit_data.age_map_class }
+    .commit
+      = commit_data.author_avatar
+      .commit-row-title
+        %span.item-title.str-truncated-100
+          = commit_data.commit_link
+        %span
+          = commit_data.project_blame_link
+         
+      .light
+        = commit_data.commit_author_link
+        = _('committed')
+        #{commit_data.time_ago_tooltip}
+  %td.line-numbers
+    - line_count = blame_group[:lines].count
+    - (current_line...(current_line + line_count)).each do |i|
+      %a.diff-line-num{ href: "#L#{i}", id: "L#{i}", 'data-line-number' => i }
+        = link_icon
+        = i
+      \
+  %td.lines
+    %pre.code.highlight
+      %code
+        - blame_group[:lines].each do |line|
+          #{line}

app/views/projects/blame/show.html.haml

-- project_duration = age_map_duration(@blame_groups, @project)
 - page_title "Blame", @blob.path, @ref
+- link_icon = icon("link")
 
 #blob-content-holder.tree-holder
   = render "projects/blob/breadcrumb", blob: @blob, blame: true
@@ -11,38 +11,13 @@
     .table-responsive.file-content.blame.code.js-syntax-highlight
       %table
         - current_line = 1
-        - @blame_groups.each do |blame_group|
-          %tr
-            - commit = blame_group[:commit]
-            %td.blame-commit{ class: age_map_class(commit.committed_date, project_duration) }
-              .commit
-                = author_avatar(commit, size: 36, has_tooltip: false)
-                .commit-row-title
-                  %span.item-title.str-truncated-100
-                    = link_to commit.title, project_commit_path(@project, commit.id), class: "cdark", title: commit.title
-                  %span
-                    - previous_commit_id = commit.parent_id
-                    - if previous_commit_id
-                      = link_to project_blame_path(@project, tree_join(previous_commit_id, @path)),
-                                title: _('View blame prior to this change'),
-                                aria: { label: _('View blame prior to this change') },
-                                data: { toggle: 'tooltip', placement: 'right', container: 'body' } do
-                        = sprite_icon('doc-versions', size: 16, css_class: 'doc-versions align-text-bottom')
-                   
-                .light
-                  = commit_author_link(commit, avatar: false)
-                  committed
-                  #{time_ago_with_tooltip(commit.committed_date)}
-            %td.line-numbers
-              - line_count = blame_group[:lines].count
-              - (current_line...(current_line + line_count)).each do |i|
-                %a.diff-line-num{ href: "#L#{i}", id: "L#{i}", 'data-line-number' => i }
-                  = icon("link")
-                  = i
-                \
-              - current_line += line_count
-            %td.lines
-              %pre.code.highlight
-                %code
-                  - blame_group[:lines].each do |line|
-                    #{line}
+        - @blame.groups.each do |blame_group|
+          - commit_data = @blame.commit_data(blame_group[:commit])
+
+          = render 'blame_group',
+            blame_group: blame_group,
+            current_line: current_line,
+            link_icon: link_icon,
+            commit_data: commit_data
+
+          - current_line += blame_group[:lines].count

locale/gitlab.pot

@@ -26235,6 +26235,9 @@ msgstr ""
 msgid "commit %{commit_id}"
 msgstr ""
 
+msgid "committed"
+msgstr ""
+
 msgid "confidentiality|You are going to turn off the confidentiality. This means <strong>everyone</strong> will be able to see and leave a comment on this issue."
 msgstr ""
 

spec/presenters/gitlab/blame_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BlamePresenter do
+  let(:project) { create(:project, :repository) }
+  let(:path) { 'files/ruby/popen.rb' }
+  let(:commit) { project.commit('master') }
+  let(:blob) { project.repository.blob_at(commit.id, path) }
+  let(:blame) { Gitlab::Blame.new(blob, commit) }
+
+  subject { described_class.new(blame, project: project, path: path) }
+
+  it 'precalculates necessary data on init' do
+    expect_any_instance_of(described_class)
+      .to receive(:precalculate_data_by_commit!)
+      .and_call_original
+
+    subject
+  end
+
+  describe '#groups' do
+    it 'delegates #groups call to the blame' do
+      expect(blame).to receive(:groups).and_call_original
+
+      subject.groups
+    end
+  end
+
+  describe '#commit_data' do
+    it 'has the data necessary to render the view' do
+      commit = blame.groups.first[:commit]
+      data = subject.commit_data(commit)
+
+      aggregate_failures do
+        expect(data.author_avatar.to_s).to include('src="https://www.gravatar.com/')
+        expect(data.age_map_class).to include('blame-commit-age-')
+        expect(data.commit_link.to_s).to include '913c66a37b4a45b9769037c55c2d238bd0942d2e">Files, encoding and much more</a>'
+        expect(data.commit_author_link.to_s).to include('<a class="commit-author-link" href=')
+        expect(data.project_blame_link.to_s).to include('<a title="View blame prior to this change"')
+        expect(data.time_ago_tooltip.to_s).to include('data-container="body">Feb 27, 2014</time>')
+      end
+    end
+  end
+end