Move to Gitlab::Diff::FileCollection

Instead calling diff_collection.count use diff_collection.size which is cache on the diff_collection

app/controllers/projects/commit_controller.rb

@@ -28,7 +28,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def diff_for_path
-    render_diff_for_path(SafeDiffs::Commit.new(@commit, diff_options: diff_options))
+    render_diff_for_path(@commit.diff_file_collection(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 = SafeDiffs::Commit.new(commit, diff_options: opts)
+    @diffs = commit.diff_file_collection(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(SafeDiffs::Compare.new(@compare, project: @project, diff_options: diff_options))
+    render_diff_for_path(Compare.decorate(@compare, @project).diff_file_collection(diff_options: diff_options))
   end
 
   def create
@@ -51,7 +51,7 @@ class Projects::CompareController < Projects::ApplicationController
         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)
+      @diffs = Compare.decorate(@compare, @project).diff_file_collection(diff_options: diff_options, diff_refs: diff_refs)
 
       @diff_notes_disabled = true
       @grouped_diff_discussions = {}

app/controllers/projects/merge_requests_controller.rb

@@ -85,7 +85,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html { define_discussion_vars }
-      format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
+      format.json do
+        @diffs = @merge_request.diff_file_collection(diff_options) if @merge_request_diff.collected?
+
+        render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") }
+      end
     end
   end
 
@@ -104,7 +108,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     define_commit_vars
 
-    render_diff_for_path(SafeDiffs::MergeRequest.new(merge_request, diff_options: diff_options))
+    render_diff_for_path(@merge_request.diff_file_collection(diff_options))
   end
 
   def commits
@@ -153,10 +157,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @commit = @merge_request.diff_head_commit
     @base_commit = @merge_request.diff_base_commit
     if @merge_request.compare
-      @diffs = SafeDiffs::Compare.new(@merge_request.compare,
+      @diffs = Compare.decorate(@merge_request.compare, @project).diff_file_collection(
-        project:      @merge_request.project,
+        diff_options: diff_options,
-        diff_refs:    @merge_request.diff_refs,
+        diff_refs: @merge_request.diff_refs
-        diff_options: diff_options)
+      )
     end
     @diff_notes_disabled = true
 

app/helpers/diff_helper.rb

@@ -23,7 +23,7 @@ module DiffHelper
   end
 
   def diff_options
-    options = SafeDiffs.default_options.merge(
+    options = Gitlab::Diff::FileCollection.default_options.merge(
       ignore_whitespace_change: hide_whitespace?,
       no_collapse: expand_all_diffs?
     )

app/models/commit.rb

@@ -317,6 +317,10 @@ class Commit
     nil
   end
 
+  def diff_file_collection(diff_options)
+    Gitlab::Diff::FileCollection::Commit.new(self, diff_options: diff_options)
+  end
+
   private
 
   def find_author_by_any_email

app/models/compare.rb

@@ -0,0 +1,23 @@
+class Compare
+  delegate :commits, :same, :head, :base, to: :@compare
+
+  def self.decorate(compare, project)
+    if compare.is_a?(Compare)
+      compare
+    else
+      self.new(compare, project)
+    end
+  end
+
+  def initialize(compare, project)
+    @compare = compare
+    @project = project
+  end
+
+  def diff_file_collection(diff_options:, diff_refs: nil)
+    Gitlab::Diff::FileCollection::Compare.new(@compare,
+      project: @project,
+      diff_options: diff_options,
+      diff_refs: diff_refs)
+  end
+end

app/models/merge_request.rb

@@ -168,6 +168,10 @@ class MergeRequest < ActiveRecord::Base
     merge_request_diff ? merge_request_diff.diffs(*args) : compare.diffs(*args)
   end
 
+  def diff_file_collection(diff_options)
+    Gitlab::Diff::FileCollection::MergeRequest.new(self, diff_options: diff_options)
+  end
+
   def diff_size
     merge_request_diff.size
   end

app/models/safe_diffs.rb

@@ -1,5 +0,0 @@
-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

@@ -1,55 +0,0 @@
-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

@@ -1,10 +0,0 @@
-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

@@ -1,10 +0,0 @@
-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

@@ -1,52 +0,0 @@
-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

@@ -2,7 +2,7 @@ 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
+      merge_request.diff_file_collection(Gitlab::Diff::FileCollection.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.diff_lines.each do |line|
+                - diff_file.highlighted_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/_ci_menu.html.haml

@@ -2,7 +2,7 @@
   = nav_link(path: 'commit#show') do
     = link_to namespace_project_commit_path(@project.namespace, @project, @commit.id) do
       Changes
-      %span.badge= @diffs.count
+      %span.badge= @diffs.size
   = nav_link(path: 'commit#builds') do
     = link_to builds_namespace_project_commit_path(@project.namespace, @project, @commit.id) do
       Builds

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.diff_lines.each do |line|
+  - diff_file.highlighted_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/show/_diffs.html.haml

@@ -1,7 +1,6 @@
 - if @merge_request_diff.collected?
-  - diffs = SafeDiffs::MergeRequest.new(@merge_request, diff_options: diff_options)
+  = render "projects/diffs/diffs", diff_files: @diffs.diff_files,
-  = render "projects/diffs/diffs", diff_files: diffs.diff_files,
+    diff_refs: @diffs.diff_refs, project: @diffs.project
-    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

app/workers/irker_worker.rb

@@ -142,7 +142,7 @@ class IrkerWorker
 
   def files_count(commit)
     files = "#{commit.diffs.real_size} file"
-    files += 's' if commit.diffs.count > 1
+    files += 's' if commit.diffs.size > 1
     files
   end
 

lib/gitlab/diff/file.rb

@@ -63,7 +63,7 @@ module Gitlab
         diff_refs.try(:head_sha)
       end
 
-      attr_writer :diff_lines, :highlighted_diff_lines
+      attr_writer :highlighted_diff_lines
 
       # Array of Gitlab::Diff::Line objects
       def diff_lines

lib/gitlab/diff/file_collection.rb

@@ -0,0 +1,9 @@
+module Gitlab
+  module Diff
+    module FileCollection
+      def self.default_options
+        ::Commit.max_diff_options.merge(ignore_whitespace_change: false, no_collapse: false)
+      end
+    end
+  end
+end

lib/gitlab/diff/file_collection/base.rb

@@ -0,0 +1,30 @@
+module Gitlab
+  module Diff
+    module FileCollection
+
+      class Base
+        attr_reader :project, :diff_options, :diff_view, :diff_refs
+
+        delegate :count, :size, :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
+          @diffs.decorate! { |diff| decorate_diff!(diff) }
+        end
+
+        private
+
+        def decorate_diff!(diff)
+          return diff if diff.is_a?(Gitlab::Diff::File)
+          Gitlab::Diff::File.new(diff, diff_refs: @diff_refs, repository: @project.repository)
+        end
+      end
+    end
+  end
+end

lib/gitlab/diff/file_collection/commit.rb

@@ -0,0 +1,14 @@
+module Gitlab
+  module Diff
+    module FileCollection
+      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
+  end
+end

lib/gitlab/diff/file_collection/compare.rb

@@ -0,0 +1,14 @@
+module Gitlab
+  module Diff
+    module FileCollection
+      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
+  end
+end

lib/gitlab/diff/file_collection/merge_request.rb

@@ -0,0 +1,88 @@
+module Gitlab
+  module Diff
+    module FileCollection
+      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
+
+        def diff_files
+          super.tap { |_| store_highlight_cache }
+        end
+
+        private
+
+        # Extracted method to highlight in the same iteration to the diff_collection. Iteration in the DiffCollections
+        # seems particularly slow on big diffs (event when already populated).
+        def decorate_diff!(diff)
+          highlight! super
+        end
+
+        def highlight!(diff_file)
+          if cacheable?
+            cache_highlight!(diff_file)
+          else
+            highlight_diff_file!(diff_file)
+          end
+        end
+
+        def highlight_diff_file!(diff_file)
+          diff_file.highlighted_diff_lines = Gitlab::Diff::Highlight.new(diff_file, repository: diff_file.repository).highlight
+          diff_file
+        end
+
+        def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
+          diff_file.highlighted_diff_lines = cache_diff_lines.map do |line|
+            Gitlab::Diff::Line.init_from_hash(line)
+          end
+        end
+
+        #
+        # 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 a Hash where the key correspond to the file_path and the values are Arrays of
+        # hashes that represent serialized diff lines.
+        #
+        def cache_highlight!(diff_file)
+          file_path = diff_file.file_path
+
+          if highlight_cache[file_path]
+            highlight_diff_file_from_cache!(diff_file, highlight_cache[file_path])
+          else
+            highlight_diff_file!(diff_file)
+            highlight_cache[file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
+          end
+
+          diff_file
+        end
+
+        def highlight_cache
+          return @highlight_cache if defined?(@highlight_cache)
+
+          @highlight_cache = Rails.cache.read(cache_key) || {}
+          @highlight_cache_was_empty = highlight_cache.empty?
+          @highlight_cache
+        end
+
+        def store_highlight_cache
+          Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty
+        end
+
+        def cacheable?
+          @merge_request.merge_request_diff.present?
+        end
+
+        def cache_key
+          [@merge_request.merge_request_diff, 'highlighted-diff-files', diff_options]
+        end
+      end
+    end
+  end
+end

lib/gitlab/email/message/repository_push.rb

@@ -40,16 +40,18 @@ module Gitlab
 
         def diffs
           return unless compare
-          
+
-          @diffs ||= SafeDiffs::Compare.new(compare, diff_options: { max_files: 30 }, project: project, diff_refs: diff_refs).diff_files
+          @diffs ||= compare.diff_file_collection(diff_options: { max_files: 30 }, diff_refs: diff_refs).diff_files
         end
 
         def diffs_count
-          diffs.count if diffs
+          diffs.size if diffs
         end
 
         def compare
-          @opts[:compare]
+          if @opts[:compare]
+            Compare.decorate(@opts[:compare], project)
+          end
         end
 
         def diff_refs

spec/controllers/projects/commit_controller_spec.rb

@@ -267,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, safe_diffs|
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
-              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(safe_diffs)
+              meth.call(diffs)
             end
 
             diff_for_path(id: commit.id, old_path: existing_path, new_path: existing_path)

spec/controllers/projects/compare_controller_spec.rb

@@ -32,10 +32,11 @@ describe Projects::CompareController do
         w: 1)
 
     expect(response).to be_success
-    expect(assigns(:diffs).diff_files.first).not_to be_nil
+    diff_file = assigns(:diffs).diff_files.first
+    expect(diff_file).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).diff_files.first.diff.diff.split("\n")
+    diff_splits = diff_file.diff.diff.split("\n")
     expect(diff_splits.length).to be <= 2
   end
 
@@ -87,9 +88,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, safe_diffs|
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
-              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(safe_diffs)
+              meth.call(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, safe_diffs|
+              expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
-                expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+                expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
-                meth.call(safe_diffs)
+                meth.call(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, safe_diffs|
+          expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
-            expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+            expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
-            meth.call(safe_diffs)
+            meth.call(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, safe_diffs|
+            expect(controller).to receive(:render_diff_for_path).and_wrap_original do |meth, diffs|
-              expect(safe_diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
+              expect(diffs.diff_files.map(&:new_path)).to contain_exactly(existing_path)
-              meth.call(safe_diffs)
+              meth.call(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' })

spec/lib/gitlab/email/message/repository_push_spec.rb

@@ -62,12 +62,12 @@ describe Gitlab::Email::Message::RepositoryPush do
 
     describe '#diffs_count' do
       subject { message.diffs_count }
-      it { is_expected.to eq compare.diffs.count }
+      it { is_expected.to eq compare.diffs.size }
     end
 
     describe '#compare' do
       subject { message.compare }
-      it { is_expected.to be_an_instance_of Gitlab::Git::Compare }
+      it { is_expected.to be_an_instance_of Compare }
     end
 
     describe '#compare_timeout' do

spec/models/merge_request_spec.rb

@@ -660,6 +660,12 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
 
+    it "executs diff cache service" do
+      expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
+
+      subject.reload_diff
+    end
+
     it "updates diff note positions" do
       old_diff_refs = subject.diff_refs
 

spec/services/merge_requests/merge_request_diff_cache_service_spec.rb

@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe MergeRequests::MergeRequestDiffCacheService do
+
+  let(:subject) { MergeRequests::MergeRequestDiffCacheService.new }
+
+  describe '#execute' do
+    it 'retrieve the diff files to cache the highlighted result' do
+      merge_request = create(:merge_request)
+      cache_key = [merge_request.merge_request_diff, 'highlighted-diff-files', Gitlab::Diff::FileCollection.default_options]
+
+      expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
+      expect(Rails.cache).to receive(:write).with(cache_key, anything)
+
+      subject.execute(merge_request)
+    end
+  end
+end