Cache highlighted diff lines for merge requests

Introducing the concept of SafeDiffs which relates 
diffs with UI highlighting.

CHANGELOG

@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - The Repository class is now instrumented
   - Cache the commit author in RequestStore to avoid extra lookups in PostReceive
   - Expand commit message width in repo view (ClemMakesApps)
+  - Cache highlighted diff lines for merge requests
   - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
   - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
   - Optimize maximum user access level lookup in loading of notes

app/controllers/concerns/diff_for_path.rb

 module DiffForPath
   extend ActiveSupport::Concern
 
-  def render_diff_for_path(diffs, diff_refs, project)
-    diff_file = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository).find do |diff|
+  def render_diff_for_path(diffs)
+    diff_file = diffs.diff_files.find do |diff|
       diff.old_path == params[:old_path] && diff.new_path == params[:new_path]
     end
 
@@ -14,7 +14,7 @@ module DiffForPath
     locals = {
       diff_file: diff_file,
       diff_commit: diff_commit,
-      diff_refs: diff_refs,
+      diff_refs: diffs.diff_refs,
       blob: blob,
       project: project
     }

app/controllers/projects/commit_controller.rb

@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def diff_for_path
-    render_diff_for_path(@diffs, @commit.diff_refs, @project)
+    render_diff_for_path(SafeDiffs::Commit.new(@commit, diff_options: diff_options))
   end
 
   def builds
@@ -110,7 +110,7 @@ class Projects::CommitController < Projects::ApplicationController
     opts = diff_options
     opts[:ignore_whitespace_change] = true if params[:format] == 'diff'
 
-    @diffs = commit.diffs(opts)
+    @diffs = SafeDiffs::Commit.new(commit, diff_options: opts)
     @notes_count = commit.notes.count
   end
 

app/controllers/projects/compare_controller.rb

@@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController
   def diff_for_path
     return render_404 unless @compare
 
-    render_diff_for_path(@diffs, @diff_refs, @project)
+    render_diff_for_path(SafeDiffs::Compare.new(@compare, project: @project, diff_options: diff_options))
   end
 
   def create
@@ -46,12 +46,12 @@ class Projects::CompareController < Projects::ApplicationController
       @commit = @project.commit(@head_ref)
       @base_commit = @project.merge_base_commit(@start_ref, @head_ref)
 
-      @diffs = @compare.diffs(diff_options)
-      @diff_refs = Gitlab::Diff::DiffRefs.new(
+      diff_refs = Gitlab::Diff::DiffRefs.new(
         base_sha: @base_commit.try(:sha),
         start_sha: @start_commit.try(:sha),
         head_sha: @commit.try(:sha)
       )
+      @diffs = SafeDiffs::Compare.new(@compare, project: @project, diff_options: diff_options, diff_refs: diff_refs)
 
       @diff_notes_disabled = true
       @grouped_diff_discussions = {}

app/controllers/projects/merge_requests_controller.rb

@@ -103,9 +103,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
 
     define_commit_vars
-    diffs = @merge_request.diffs(diff_options)
 
-    render_diff_for_path(diffs, @merge_request.diff_refs, @merge_request.project)
+    render_diff_for_path(SafeDiffs::MergeRequest.new(merge_request, diff_options: diff_options))
   end
 
   def commits
@@ -153,7 +152,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @commits = @merge_request.compare_commits.reverse
     @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
-    @diffs = @merge_request.compare.diffs(diff_options) if @merge_request.compare
+    if @merge_request.compare
+      @diffs = SafeDiffs::Compare.new(@merge_request.compare,
+        project:      @merge_request.project,
+        diff_refs:    @merge_request.diff_refs,
+        diff_options: diff_options)
+    end
     @diff_notes_disabled = true
 
     @pipeline = @merge_request.pipeline

app/helpers/commits_helper.rb

@@ -206,10 +206,10 @@ module CommitsHelper
     end
   end
 
-  def view_file_btn(commit_sha, diff, project)
+  def view_file_btn(commit_sha, diff_new_path, project)
     link_to(
       namespace_project_blob_path(project.namespace, project,
-                                  tree_join(commit_sha, diff.new_path)),
+                                  tree_join(commit_sha, diff_new_path)),
       class: 'btn view-file js-view-file btn-file-option'
     ) do
       raw('View file @') + content_tag(:span, commit_sha[0..6],

app/helpers/diff_helper.rb

@@ -23,18 +23,17 @@ module DiffHelper
   end
 
   def diff_options
-    options = { ignore_whitespace_change: hide_whitespace?, no_collapse: expand_all_diffs? }
+    options = SafeDiffs.default_options.merge(
+      ignore_whitespace_change: hide_whitespace?,
+      no_collapse: expand_all_diffs?
+    )
 
     if action_name == 'diff_for_path'
       options[:no_collapse] = true
       options[:paths] = params.values_at(:old_path, :new_path)
     end
 
-    Commit.max_diff_options.merge(options)
-  end
-
-  def safe_diff_files(diffs, diff_refs: nil, repository: nil)
-    diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
+    options
   end
 
   def unfold_bottom_class(bottom)

app/models/merge_request.rb

@@ -313,6 +313,8 @@ class MergeRequest < ActiveRecord::Base
 
     merge_request_diff.reload_content
 
+    MergeRequests::MergeRequestDiffCacheService.new.execute(self)
+
     new_diff_refs = self.diff_refs
 
     update_diff_notes_positions(

app/models/safe_diffs.rb

+module SafeDiffs
+  def self.default_options
+    ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
+  end
+end

app/models/safe_diffs/base.rb

+module SafeDiffs
+  class Base
+    attr_reader :project, :diff_options, :diff_view, :diff_refs
+
+    delegate :count, :real_size, to: :diff_files
+
+    def initialize(diffs, project:, diff_options:, diff_refs: nil)
+      @diffs = diffs
+      @project = project
+      @diff_options = diff_options
+      @diff_refs = diff_refs
+    end
+
+    def diff_files
+      @diff_files ||= begin
+        diffs = @diffs.decorate! do |diff|
+          Gitlab::Diff::File.new(diff, diff_refs: @diff_refs, repository: @project.repository)
+        end
+
+        highlight!(diffs)
+        diffs
+      end
+    end
+
+    private
+
+    def highlight!(diff_files)
+      if cacheable?
+        cache_highlight!(diff_files)
+      else
+        diff_files.each { |diff_file| highlight_diff_file!(diff_file) }
+      end
+    end
+
+    def cacheable?
+      false
+    end
+
+    def cache_highlight!
+      raise NotImplementedError
+    end
+
+    def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
+      diff_file.diff_lines = cache_diff_lines.map do |line|
+        Gitlab::Diff::Line.init_from_hash(line)
+      end
+    end
+
+    def highlight_diff_file!(diff_file)
+      diff_file.diff_lines = Gitlab::Diff::Highlight.new(diff_file, repository: diff_file.repository).highlight
+      diff_file.highlighted_diff_lines = diff_file.diff_lines # To be used on parallel diff
+      diff_file
+    end
+  end
+end

app/models/safe_diffs/commit.rb

+module SafeDiffs
+  class Commit < Base
+    def initialize(commit, diff_options:)
+      super(commit.diffs(diff_options),
+        project: commit.project,
+        diff_options: diff_options,
+        diff_refs: commit.diff_refs)
+    end
+  end
+end

app/models/safe_diffs/compare.rb

+module SafeDiffs
+  class Compare < Base
+    def initialize(compare, project:, diff_options:, diff_refs: nil)
+      super(compare.diffs(diff_options),
+        project: project,
+        diff_options: diff_options,
+        diff_refs: diff_refs)
+    end
+  end
+end

app/models/safe_diffs/merge_request.rb

+module SafeDiffs
+  class MergeRequest < Base
+    def initialize(merge_request, diff_options:)
+      @merge_request = merge_request
+
+      super(merge_request.diffs(diff_options),
+        project: merge_request.project,
+        diff_options: diff_options,
+        diff_refs: merge_request.diff_refs)
+    end
+
+    private
+
+    #
+    # If we find the highlighted diff files lines on the cache we replace existing diff_files lines (no highlighted)
+    # for the highlighted ones, so we just skip their execution.
+    # If the highlighted diff files lines are not cached we calculate and cache them.
+    #
+    # The content of the cache is and Hash where the key correspond to the file_path and the values are Arrays of
+    # hashes than represent serialized diff lines.
+    #
+    def cache_highlight!(diff_files)
+      highlighted_cache = Rails.cache.read(cache_key) || {}
+      highlighted_cache_was_empty = highlighted_cache.empty?
+
+      diff_files.each do |diff_file|
+        file_path = diff_file.file_path
+
+        if highlighted_cache[file_path]
+          highlight_diff_file_from_cache!(diff_file, highlighted_cache[file_path])
+        else
+          highlight_diff_file!(diff_file)
+          highlighted_cache[file_path] = diff_file.diff_lines.map(&:to_hash)
+        end
+      end
+
+      if highlighted_cache_was_empty
+        Rails.cache.write(cache_key, highlighted_cache)
+      end
+
+      diff_files
+    end
+
+    def cacheable?
+      @merge_request.merge_request_diff.present?
+    end
+
+    def cache_key
+      [@merge_request.merge_request_diff, 'highlighted-safe-diff-files', diff_options]
+    end
+  end
+end

app/services/merge_requests/merge_request_diff_cache_service.rb

+module MergeRequests
+  class MergeRequestDiffCacheService
+    def execute(merge_request)
+      # Executing the iteration we cache all the highlighted diff information
+      SafeDiffs::MergeRequest.new(merge_request, diff_options: SafeDiffs.default_options).diff_files.to_a
+    end
+  end
+end

app/views/notify/repository_push_email.html.haml

@@ -75,7 +75,7 @@
             - blob = diff_file.blob
             - if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
               %table.code.white
-                - diff_file.highlighted_diff_lines.each do |line|
+                - diff_file.diff_lines.each do |line|
                   = render "projects/diffs/line", line: line, diff_file: diff_file, plain: true
             - else
               No preview for this file type

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

@@ -7,7 +7,7 @@
   = render "ci_menu"
 - else
   %div.block-connector
-= render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @commit.diff_refs
+= render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @diffs.diff_refs
 = render "projects/notes/notes_with_form"
 - if can_collaborate_with_project?
   - %w(revert cherry-pick).each do |type|

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

@@ -8,7 +8,7 @@
 
   - if @commits.present?
     = render "projects/commits/commit_list"
-    = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @diff_refs
+    = render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @diffs.diff_refs
   - else
     .light-well
       .center

app/views/projects/diffs/_diffs.html.haml

@@ -2,8 +2,6 @@
 - if diff_view == 'parallel'
   - fluid_layout true
 
-- diff_files = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository)
-
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons
     - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? }

app/views/projects/diffs/_file.html.haml

@@ -15,6 +15,6 @@
                 from_merge_request_id: @merge_request.id,
                 skip_visible_check: true)
 
-        = view_file_btn(diff_commit.id, diff_file, project)
+        = view_file_btn(diff_commit.id, diff_file.new_path, project)
 
-  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project
+  = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, blob: blob, project: project

app/views/projects/diffs/_text_file.html.haml

@@ -5,7 +5,7 @@
 
 %table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
   - last_line = 0
-  - diff_file.highlighted_diff_lines.each do |line|
+  - diff_file.diff_lines.each do |line|
     - last_line = line.new_pos
     = render "projects/diffs/line", line: line, diff_file: diff_file
 

app/views/projects/merge_requests/_new_submit.html.haml

@@ -42,7 +42,7 @@
           %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
           %p To preserve performance the line changes are not shown.
       - else
-        = render "projects/diffs/diffs", diffs: @diffs, project: @project, diff_refs: @merge_request.diff_refs, show_whitespace_toggle: false
+        = render "projects/diffs/diffs", diff_files: @diffs.diff_files, project: @diffs.project, diff_refs: @merge_request.diff_refs, show_whitespace_toggle: false
     - if @pipeline
       #builds.builds.tab-pane
         = render "projects/merge_requests/show/builds"

app/views/projects/merge_requests/show/_diffs.html.haml

 - if @merge_request_diff.collected?
-  = render "projects/diffs/diffs", diffs: @merge_request.diffs(diff_options),
-    project: @merge_request.project, diff_refs: @merge_request.diff_refs
+  - diffs = SafeDiffs::MergeRequest.new(@merge_request, diff_options: diff_options)
+  = render "projects/diffs/diffs", diff_files: diffs.diff_files,
+    diff_refs: diffs.diff_refs, project: diffs.project
 - elsif @merge_request_diff.empty?
   .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch}
 - else

lib/gitlab/diff/file.rb

@@ -63,15 +63,18 @@ module Gitlab
         diff_refs.try(:head_sha)
       end
 
+      attr_writer :diff_lines, :highlighted_diff_lines
+
       # Array of Gitlab::Diff::Line objects
       def diff_lines
-        @lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
+        @diff_lines ||= Gitlab::Diff::Parser.new.parse(raw_diff.each_line).to_a
       end
 
       def highlighted_diff_lines
         @highlighted_diff_lines ||= Gitlab::Diff::Highlight.new(self, repository: self.repository).highlight
       end
 
+      # Array[<Hash>] with right/left keys that contains Gitlab::Diff::Line objects which text is hightlighted
       def parallel_diff_lines
         @parallel_diff_lines ||= Gitlab::Diff::ParallelDiff.new(self).parallelize
       end

lib/gitlab/diff/highlight.rb

@@ -40,8 +40,6 @@ module Gitlab
       def highlight_line(diff_line)
         return unless diff_file && diff_file.diff_refs
 
-        line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
-
         rich_line =
           if diff_line.unchanged? || diff_line.added?
             new_lines[diff_line.new_pos - 1]
@@ -51,7 +49,10 @@ module Gitlab
 
         # Only update text if line is found. This will prevent
         # issues with submodules given the line only exists in diff content.
-        "#{line_prefix}#{rich_line}".html_safe if rich_line
+        if rich_line
+          line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' '
+          "#{line_prefix}#{rich_line}".html_safe
+        end
       end
 
       def inline_diffs

lib/gitlab/diff/line.rb

@@ -9,6 +9,20 @@ module Gitlab
         @old_pos, @new_pos = old_pos, new_pos
       end
 
+      def self.init_from_hash(hash)
+        new(hash[:text], hash[:type], hash[:index], hash[:old_pos], hash[:new_pos])
+      end
+
+      def serialize_keys
+        @serialize_keys ||= %i(text type index old_pos new_pos)
+      end
+
+      def to_hash
+        hash = {}
+        serialize_keys.each { |key| hash[key] = send(key) }
+        hash
+      end
+
       def old_line
         old_pos unless added? || meta?
       end

lib/gitlab/email/message/repository_push.rb

@@ -41,7 +41,7 @@ module Gitlab
         def diffs
           return unless compare
           
-          @diffs ||= safe_diff_files(compare.diffs(max_files: 30), diff_refs: diff_refs, repository: project.repository)
+          @diffs ||= SafeDiffs::Compare.new(compare, diff_options: { max_files: 30 }, project: project, diff_refs: diff_refs).diff_files
         end
 
         def diffs_count

spec/controllers/projects/commit_controller_spec.rb

@@ -83,7 +83,7 @@ describe Projects::CommitController do
       let(:format) { :diff }
 
       it "should really only be a git diff" do
-        go(id: commit.id, format: format)
+        go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format)
 
         expect(response.body).to start_with("diff --git")
       end
@@ -92,8 +92,9 @@ describe Projects::CommitController do
         go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1)
 
         expect(response.body).to start_with("diff --git")
-        # without whitespace option, there are more than 2 diff_splits
-        diff_splits = assigns(:diffs).first.diff.split("\n")
+
+        # without whitespace option, there are more than 2 diff_splits for other formats
+        diff_splits = assigns(:diffs).diff_files.first.diff.diff.split("\n")
         expect(diff_splits.length).to be <= 2
       end
     end
@@ -266,9 +267,9 @@ describe Projects::CommitController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)

spec/controllers/projects/compare_controller_spec.rb

@@ -19,7 +19,7 @@ describe Projects::CompareController do
         to: ref_to)
 
     expect(response).to be_success
-    expect(assigns(:diffs).first).not_to be_nil
+    expect(assigns(:diffs).diff_files.first).not_to be_nil
     expect(assigns(:commits).length).to be >= 1
   end
 
@@ -32,10 +32,10 @@ describe Projects::CompareController do
         w: 1)
 
     expect(response).to be_success
-    expect(assigns(:diffs).first).not_to be_nil
+    expect(assigns(:diffs).diff_files.first).not_to be_nil
     expect(assigns(:commits).length).to be >= 1
     # without whitespace option, there are more than 2 diff_splits
-    diff_splits = assigns(:diffs).first.diff.split("\n")
+    diff_splits = assigns(:diffs).diff_files.first.diff.diff.split("\n")
     expect(diff_splits.length).to be <= 2
   end
 
@@ -48,7 +48,7 @@ describe Projects::CompareController do
           to: ref_to)
 
       expect(response).to be_success
-      expect(assigns(:diffs).to_a).to eq([])
+      expect(assigns(:diffs).diff_files.to_a).to eq([])
       expect(assigns(:commits)).to eq([])
     end
 
@@ -87,9 +87,9 @@ describe Projects::CompareController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(from: ref_from, to: ref_to, old_path: existing_path, new_path: existing_path)

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -392,9 +392,9 @@ describe Projects::MergeRequestsController do
             end
 
             it 'only renders the diffs for the path given' do
-              expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-                expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-                meth.call(diffs, diff_refs, project)
+              expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+                expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+                meth.call(safe_diffs)
               end
 
               diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
@@ -455,9 +455,9 @@ describe Projects::MergeRequestsController do
         end
 
         it 'only renders the diffs for the path given' do
-          expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-            expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-            meth.call(diffs, diff_refs, project)
+          expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+            expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+            meth.call(safe_diffs)
           end
 
           diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_branch: 'feature', target_branch: 'master' })
@@ -477,9 +477,9 @@ describe Projects::MergeRequestsController do
           end
 
           it 'only renders the diffs for the path given' do
-            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs, diff_refs, project|
-              expect(diffs.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(diffs, diff_refs, project)
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, safe_diffs|
+              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              meth.call(safe_diffs)
             end
 
             diff_for_path(old_path: existing_path, new_path: existing_path, merge_request: { source_project: other_project, source_branch: 'feature', target_branch: 'master' })