From 3fa823e3a014fffcbc699d6be6664b8ed9ae86f5 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 11 Nov 2019 16:56:17 -0800
Subject: [PATCH 01/38] Add redis caching of individual diffs in an HSET

HSET allows us to use a Redis hash, so we can store the combined diffs
for a given merge request rev in a single key _and_ access individual
file diffs as needed.
---
 lib/gitlab/diff/highlight_cache.rb           | 69 ++++++++++++++-
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 92 +++++++++++++++++++-
 2 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index e4390771db2c54..2fc48d72b56f35 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -3,6 +3,12 @@
 module Gitlab
   module Diff
     class HighlightCache
+      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
+      #   extract into shared module?
+      #
+      VERSION = 1
+      EXPIRATION = 1.week
+
       delegate :diffable, to: :@diff_collection
       delegate :diff_options, to: :@diff_collection
 
@@ -35,7 +41,54 @@ def write_if_empty
           cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        cache.write(key, cached_content, expires_in: 1.week)
+        if diffable.project.feature_available?(:redis_diff_caching)
+          write_to_redis_hash(cached_content)
+        else
+          cache.write(key, cached_content, expires_in: 1.week)
+        end
+      end
+
+      # Given a hash of:
+      #   { "file/to/cache" =>
+      #   [ { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_19_19",
+      #       rich_text: " <span id=\"LC19\" class=\"line\" lang=\"plaintext\">config/initializers/secret_token.rb</span>\n",
+      #       text: " config/initializers/secret_token.rb",
+      #       type: nil,
+      #       index: 3,
+      #       old_pos: 19,
+      #       new_pos: 19 }
+      #   ] }
+      #
+      #   ...it will write/update a Redis hash (HSET)
+      #
+      def write_to_redis_hash(hash)
+        key = diffable.cache_key
+
+        if key
+          Redis::Cache.with do |redis|
+            redis.multi do |multi|
+              hash.each do |diff_file_id, highlighted_diff_lines_hash|
+                multi.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
+
+                # HSETs have to have their expiration date manually updated
+                #
+                multi.expire(key, EXPIRATION)
+              end
+            end
+          end
+        end
+      end
+
+      def read_entire_redis_hash(key)
+        Redis::Cache.with do |redis|
+          redis.hgetall(key)
+        end
+      end
+
+      def read_single_entry_from_redis_hash(key, diff_file_id)
+        Redis::Cache.with do |redis|
+          redis.hget(key, diff_file_id)
+        end
       end
 
       def clear
@@ -63,6 +116,20 @@ def cached_content
       def cacheable?(diff_file)
         diffable.present? && diff_file.text? && diff_file.diffable?
       end
+
+      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
+      #   extract into shared module?
+      #
+      def cache_key_for(raw_key)
+        "#{cache_key_prefix}:#{raw_key}"
+      end
+
+      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
+      #   extract into shared module?
+      #
+      def cache_key_prefix
+        "#{Redis::Cache::CACHE_NAMESPACE}:#{VERSION}:diff-highlight"
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index bfcfed4231f7a9..3f218e7e4e63ec 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -2,8 +2,40 @@
 
 require 'spec_helper'
 
-describe Gitlab::Diff::HighlightCache do
+describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
   let(:merge_request) { create(:merge_request_with_diffs) }
+  let(:diff_hash) do
+    { ".gitignore-false-false-false" =>
+      [{ line_code: nil, rich_text: nil, text: "@@ -17,3 +17,4 @@ rerun.txt", type: "match", index: 0, old_pos: 17, new_pos: 17 },
+       { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_17_17",
+        rich_text: " <span id=\"LC17\" class=\"line\" lang=\"plaintext\">pickle-email-*.html</span>\n",
+        text: " pickle-email-*.html",
+        type: nil,
+        index: 1,
+        old_pos: 17,
+        new_pos: 17 },
+       { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_18_18",
+        rich_text: " <span id=\"LC18\" class=\"line\" lang=\"plaintext\">.project</span>\n",
+        text: " .project",
+        type: nil,
+        index: 2,
+        old_pos: 18,
+        new_pos: 18 },
+       { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_19_19",
+        rich_text: " <span id=\"LC19\" class=\"line\" lang=\"plaintext\">config/initializers/secret_token.rb</span>\n",
+        text: " config/initializers/secret_token.rb",
+        type: nil,
+        index: 3,
+        old_pos: 19,
+        new_pos: 19 },
+       { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_20_20",
+        rich_text: "+<span id=\"LC20\" class=\"line\" lang=\"plaintext\">.DS_Store</span>",
+        text: "+.DS_Store",
+        type: "new",
+        index: 4,
+        old_pos: 20,
+        new_pos: 20 }] }
+  end
 
   subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
 
@@ -48,9 +80,22 @@
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
+    context 'when :redis_diff_caching is enabled' do
+      before do
+        expect(cache.diffable.project)
+          .to receive(:feature_available?)
+          .with(:redis_diff_caching).and_return(true)
+      end
+
+      it 'submits a single write action to the redis cache when invoked multiple times' do
+        expect(cache).to receive(:write_to_redis_hash).once
+
+        2.times { cache.write_if_empty }
+      end
+    end
+
     it 'submits a single writing to the cache' do
-      cache.write_if_empty
-      cache.write_if_empty
+      2.times { cache.write_if_empty }
 
       expect(backend).to have_received(:write).with(cache.key,
                                                     hash_including('CHANGELOG-false-false-false'),
@@ -58,6 +103,47 @@
     end
   end
 
+  describe '#write_to_redis_hash' do
+    let(:backend) { Rails.cache }
+
+    it 'creates or updates a Redis hash' do
+      expect { cache.write_to_redis_hash(diff_hash) }
+        .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache.diffable.cache_key) } }
+    end
+  end
+
+  describe '#read_entire_redis_hash' do
+    let(:backend) { Rails.cache }
+    let(:cache_key) { cache.diffable.cache_key }
+
+    before do
+      cache.write_to_redis_hash(diff_hash)
+    end
+
+    it 'returns the entire contents of a Redis hash as JSON' do
+      result = cache.read_entire_redis_hash(cache_key)
+
+      expect(result.values.first).to eq(diff_hash.values.first.to_json)
+    end
+  end
+
+  describe '#read_single_entry_from_redis_hash' do
+    let(:backend) { Rails.cache }
+    let(:cache_key) { cache.diffable.cache_key }
+
+    before do
+      cache.write_to_redis_hash(diff_hash)
+    end
+
+    it 'returns highlighted diff content for a single file as JSON' do
+      diff_hash.each do |file_path, value|
+        found = cache.read_single_entry_from_redis_hash(cache_key, file_path)
+
+        expect(found).to eq(value.to_json)
+      end
+    end
+  end
+
   describe '#clear' do
     let(:backend) { double('backend').as_null_object }
 
-- 
GitLab


From 8ad8252c315a7c31aa01968ba0cbaa3e565d76d1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 11 Nov 2019 18:14:44 -0800
Subject: [PATCH 02/38] Remove unused code

---
 lib/gitlab/diff/highlight_cache.rb | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 2fc48d72b56f35..cdc54239a70905 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -3,10 +3,6 @@
 module Gitlab
   module Diff
     class HighlightCache
-      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
-      #   extract into shared module?
-      #
-      VERSION = 1
       EXPIRATION = 1.week
 
       delegate :diffable, to: :@diff_collection
@@ -116,20 +112,6 @@ def cached_content
       def cacheable?(diff_file)
         diffable.present? && diff_file.text? && diff_file.diffable?
       end
-
-      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
-      #   extract into shared module?
-      #
-      def cache_key_for(raw_key)
-        "#{cache_key_prefix}:#{raw_key}"
-      end
-
-      # TODO: copied from lib/gitlab/discussions_diff/highlight_cache.rb
-      #   extract into shared module?
-      #
-      def cache_key_prefix
-        "#{Redis::Cache::CACHE_NAMESPACE}:#{VERSION}:diff-highlight"
-      end
     end
   end
 end
-- 
GitLab


From d56b973a196c3506ec95198d63bbea031af48e9b Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 11 Nov 2019 20:51:00 -0800
Subject: [PATCH 03/38] Adds missing param to feature_available? call

---
 lib/gitlab/diff/highlight_cache.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index cdc54239a70905..6e1c351ceb3a6b 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -37,7 +37,7 @@ def write_if_empty
           cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        if diffable.project.feature_available?(:redis_diff_caching)
+        if diffable.project.feature_available?(:redis_diff_caching, current_user)
           write_to_redis_hash(cached_content)
         else
           cache.write(key, cached_content, expires_in: 1.week)
-- 
GitLab


From dc6522b8ee29e1fa73727974247f04ae2c97b3ca Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 11 Nov 2019 22:40:12 -0800
Subject: [PATCH 04/38] Use blanket Feature.enabled? check

---
 lib/gitlab/diff/highlight_cache.rb           |  2 +-
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 21 ++++++++++++--------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 6e1c351ceb3a6b..6e269ddb8d31f9 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -37,7 +37,7 @@ def write_if_empty
           cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        if diffable.project.feature_available?(:redis_diff_caching, current_user)
+        if Feature.enabled?(:redis_diff_caching)
           write_to_redis_hash(cached_content)
         else
           cache.write(key, cached_content, expires_in: 1.week)
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 3f218e7e4e63ec..9d9c60d0773d14 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -82,9 +82,7 @@
 
     context 'when :redis_diff_caching is enabled' do
       before do
-        expect(cache.diffable.project)
-          .to receive(:feature_available?)
-          .with(:redis_diff_caching).and_return(true)
+        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(true)
       end
 
       it 'submits a single write action to the redis cache when invoked multiple times' do
@@ -94,12 +92,19 @@
       end
     end
 
-    it 'submits a single writing to the cache' do
-      2.times { cache.write_if_empty }
+    context 'when :redis_diff_caching is not enabled' do
+      before do
+        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(false)
+      end
 
-      expect(backend).to have_received(:write).with(cache.key,
-                                                    hash_including('CHANGELOG-false-false-false'),
-                                                    expires_in: 1.week).once
+      it 'submits a single writing to the cache' do
+        2.times { cache.write_if_empty }
+
+        expect(backend)
+          .to have_received(:write)
+          .with(cache.key, hash_including('CHANGELOG-false-false-false'), expires_in: 1.week)
+          .once
+      end
     end
   end
 
-- 
GitLab


From 0c00fa10dde83c3b93cfcc1fb60e98971ab59d5b Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 11 Nov 2019 23:28:49 -0800
Subject: [PATCH 05/38] Cache redis loading and streamline method calls

---
 lib/gitlab/diff/highlight_cache.rb           | 41 ++++++++++++--------
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 22 +++++++----
 2 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 6e269ddb8d31f9..dc233a1710e4eb 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -11,6 +11,7 @@ class HighlightCache
       def initialize(diff_collection, backend: Rails.cache)
         @backend = backend
         @diff_collection = diff_collection
+        @redis_key = diffable.cache_key if Feature.enabled?(:redis_diff_caching)
       end
 
       # - Reads from cache
@@ -58,32 +59,30 @@ def write_if_empty
       #   ...it will write/update a Redis hash (HSET)
       #
       def write_to_redis_hash(hash)
-        key = diffable.cache_key
-
-        if key
-          Redis::Cache.with do |redis|
-            redis.multi do |multi|
-              hash.each do |diff_file_id, highlighted_diff_lines_hash|
-                multi.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
-
-                # HSETs have to have their expiration date manually updated
-                #
-                multi.expire(key, EXPIRATION)
-              end
+        return unless @redis_key
+
+        Redis::Cache.with do |redis|
+          redis.multi do |multi|
+            hash.each do |diff_file_id, highlighted_diff_lines_hash|
+              multi.hset(@redis_key, diff_file_id, highlighted_diff_lines_hash.to_json)
+
+              # HSETs have to have their expiration date manually updated
+              #
+              multi.expire(@redis_key, EXPIRATION)
             end
           end
         end
       end
 
-      def read_entire_redis_hash(key)
+      def read_entire_redis_hash
         Redis::Cache.with do |redis|
-          redis.hgetall(key)
+          redis.hgetall(@redis_key)
         end
       end
 
-      def read_single_entry_from_redis_hash(key, diff_file_id)
+      def read_single_entry_from_redis_hash(diff_file_id)
         Redis::Cache.with do |redis|
-          redis.hget(key, diff_file_id)
+          redis.hget(@redis_key, diff_file_id)
         end
       end
 
@@ -106,7 +105,15 @@ def cache
       end
 
       def cached_content
-        @cached_content ||= cache.read(key) || {}
+        @cached_content ||= populate_cached_content || {}
+      end
+
+      def populate_cached_content
+        if Feature.enabled?(:redis_diff_caching)
+          read_entire_redis_hash
+        else
+          cache.read(key)
+        end
       end
 
       def cacheable?(diff_file)
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 9d9c60d0773d14..378a43bb76a538 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -53,6 +53,7 @@
                              fallback_diff_refs: diffs.fallback_diff_refs)
     end
 
+
     it 'does not calculate highlighting when reading from cache' do
       cache.write_if_empty
       cache.decorate(diff_file)
@@ -69,11 +70,18 @@
       expect(diff_file.highlighted_diff_lines.size).to be > 5
     end
 
-    it 'submits a single reading from the cache' do
-      cache.decorate(diff_file)
-      cache.decorate(diff_file)
+    context 'when :redis_diff_caching is not enabled' do
+      before do
+        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(false)
+      end
+
+      it 'submits a single reading from the cache' do
+        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
+
+        2.times { cache.decorate(diff_file) }
 
-      expect(backend).to have_received(:read).with(cache.key).once
+        expect(backend).to have_received(:read).with(cache.key).once
+      end
     end
   end
 
@@ -94,7 +102,7 @@
 
     context 'when :redis_diff_caching is not enabled' do
       before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(false)
+        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
       end
 
       it 'submits a single writing to the cache' do
@@ -126,7 +134,7 @@
     end
 
     it 'returns the entire contents of a Redis hash as JSON' do
-      result = cache.read_entire_redis_hash(cache_key)
+      result = cache.read_entire_redis_hash
 
       expect(result.values.first).to eq(diff_hash.values.first.to_json)
     end
@@ -142,7 +150,7 @@
 
     it 'returns highlighted diff content for a single file as JSON' do
       diff_hash.each do |file_path, value|
-        found = cache.read_single_entry_from_redis_hash(cache_key, file_path)
+        found = cache.read_single_entry_from_redis_hash(file_path)
 
         expect(found).to eq(value.to_json)
       end
-- 
GitLab


From cdf2d77dea3a970fe61a4c1d37aed0496d5a0518 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 12 Nov 2019 07:44:20 -0800
Subject: [PATCH 06/38] Make :robot: :cop: happy :donut:

---
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 378a43bb76a538..48e171611ddc5e 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -53,7 +53,6 @@
                              fallback_diff_refs: diffs.fallback_diff_refs)
     end
 
-
     it 'does not calculate highlighting when reading from cache' do
       cache.write_if_empty
       cache.decorate(diff_file)
-- 
GitLab


From 73b7f8b05bcc4a573096be43392037221cc5b42f Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 12 Nov 2019 16:35:39 -0800
Subject: [PATCH 07/38] Update to use stringified version of existing key

---
 lib/gitlab/diff/highlight_cache.rb           | 8 +++-----
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index dc233a1710e4eb..3c1cbcc4c46b6b 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -59,12 +59,10 @@ def write_if_empty
       #   ...it will write/update a Redis hash (HSET)
       #
       def write_to_redis_hash(hash)
-        return unless @redis_key
-
         Redis::Cache.with do |redis|
           redis.multi do |multi|
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
-              multi.hset(@redis_key, diff_file_id, highlighted_diff_lines_hash.to_json)
+              multi.hset(key.to_s, diff_file_id, highlighted_diff_lines_hash.to_json)
 
               # HSETs have to have their expiration date manually updated
               #
@@ -76,13 +74,13 @@ def write_to_redis_hash(hash)
 
       def read_entire_redis_hash
         Redis::Cache.with do |redis|
-          redis.hgetall(@redis_key)
+          redis.hgetall(key.to_s)
         end
       end
 
       def read_single_entry_from_redis_hash(diff_file_id)
         Redis::Cache.with do |redis|
-          redis.hget(@redis_key, diff_file_id)
+          redis.hget(key.to_s, diff_file_id)
         end
       end
 
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 48e171611ddc5e..6d0e8f0cfb15d9 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -120,7 +120,7 @@
 
     it 'creates or updates a Redis hash' do
       expect { cache.write_to_redis_hash(diff_hash) }
-        .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache.diffable.cache_key) } }
+        .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache.key.to_s) } }
     end
   end
 
-- 
GitLab


From f6ae155fd02fbedf0b655ff6bb7846c775151c7b Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 12 Nov 2019 16:36:17 -0800
Subject: [PATCH 08/38] Cache redis access and transform JSON for parity

---
 lib/gitlab/diff/highlight_cache.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 3c1cbcc4c46b6b..1ae1cbcc89a8df 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -103,14 +103,14 @@ def cache
       end
 
       def cached_content
-        @cached_content ||= populate_cached_content || {}
+        @cached_content ||= populate_cached_content
       end
 
       def populate_cached_content
         if Feature.enabled?(:redis_diff_caching)
-          read_entire_redis_hash
+          read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
         else
-          cache.read(key)
+          cache.read(key) || {}
         end
       end
 
-- 
GitLab


From cc1442bb031b5ac8383f113e331c93067c86af92 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 13 Nov 2019 12:25:10 -0800
Subject: [PATCH 09/38] Use cache key from diffable for Redis

---
 lib/gitlab/diff/highlight_cache.rb           | 8 ++++----
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 1ae1cbcc89a8df..21a29559be09a4 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -11,7 +11,7 @@ class HighlightCache
       def initialize(diff_collection, backend: Rails.cache)
         @backend = backend
         @diff_collection = diff_collection
-        @redis_key = diffable.cache_key if Feature.enabled?(:redis_diff_caching)
+        @redis_key = "highlighted-diff-files:#{diffable.cache_key}" if Feature.enabled?(:redis_diff_caching)
       end
 
       # - Reads from cache
@@ -62,7 +62,7 @@ def write_to_redis_hash(hash)
         Redis::Cache.with do |redis|
           redis.multi do |multi|
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
-              multi.hset(key.to_s, diff_file_id, highlighted_diff_lines_hash.to_json)
+              multi.hset(@redis_key, diff_file_id, highlighted_diff_lines_hash.to_json)
 
               # HSETs have to have their expiration date manually updated
               #
@@ -74,13 +74,13 @@ def write_to_redis_hash(hash)
 
       def read_entire_redis_hash
         Redis::Cache.with do |redis|
-          redis.hgetall(key.to_s)
+          redis.hgetall(@redis_key )
         end
       end
 
       def read_single_entry_from_redis_hash(diff_file_id)
         Redis::Cache.with do |redis|
-          redis.hget(key.to_s, diff_file_id)
+          redis.hget(@redis_key, diff_file_id)
         end
       end
 
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 6d0e8f0cfb15d9..b4aa5e65155bf9 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -117,10 +117,11 @@
 
   describe '#write_to_redis_hash' do
     let(:backend) { Rails.cache }
+    let(:cache_key) { cache.diffable.cache_key }
 
     it 'creates or updates a Redis hash' do
       expect { cache.write_to_redis_hash(diff_hash) }
-        .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache.key.to_s) } }
+        .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache_key) } }
     end
   end
 
-- 
GitLab


From d4e9269907bcb05b7c1cdfb4dda59ef413211c36 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 13 Nov 2019 12:42:59 -0800
Subject: [PATCH 10/38] Migrate HighlightCache -> DeprecatedHighlightCache

Originally I had hoped to feature-flag the changes to support redis hash
caching, as they seemed small, but of course those changes expanded and
expanded until it became clear it really needed to be extracted into a
new class with a parallel API to keep it clean. h/t to @oswaldo for
pointing out when we'd crossed that threshold!
---
 lib/gitlab/diff/deprecated_highlight_cache.rb | 68 ++++++++++++++++++
 .../file_collection/merge_request_diff.rb     |  3 +-
 lib/gitlab/diff/highlight_cache.rb            | 26 ++-----
 .../diff/deprecated_highlight_cache_spec.rb   | 70 +++++++++++++++++++
 spec/lib/gitlab/diff/highlight_cache_spec.rb  | 42 ++---------
 spec/services/notes/create_service_spec.rb    |  2 +-
 6 files changed, 151 insertions(+), 60 deletions(-)
 create mode 100644 lib/gitlab/diff/deprecated_highlight_cache.rb
 create mode 100644 spec/lib/gitlab/diff/deprecated_highlight_cache_spec.rb

diff --git a/lib/gitlab/diff/deprecated_highlight_cache.rb b/lib/gitlab/diff/deprecated_highlight_cache.rb
new file mode 100644
index 00000000000000..473476869730ae
--- /dev/null
+++ b/lib/gitlab/diff/deprecated_highlight_cache.rb
@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+#
+module Gitlab
+  module Diff
+    class DeprecatedHighlightCache
+      delegate :diffable, to: :@diff_collection
+      delegate :diff_options, to: :@diff_collection
+
+      def initialize(diff_collection, backend: Rails.cache)
+        @backend = backend
+        @diff_collection = diff_collection
+      end
+
+      # - Reads from cache
+      # - Assigns DiffFile#highlighted_diff_lines for cached files
+      def decorate(diff_file)
+        if content = read_file(diff_file)
+          diff_file.highlighted_diff_lines = content.map do |line|
+            Gitlab::Diff::Line.init_from_hash(line)
+          end
+        end
+      end
+
+      # It populates a Hash in order to submit a single write to the memory
+      # cache. This avoids excessive IO generated by N+1's (1 writing for
+      # each highlighted line or file).
+      def write_if_empty
+        return if cached_content.present?
+
+        @diff_collection.diff_files.each do |diff_file|
+          next unless cacheable?(diff_file)
+
+          diff_file_id = diff_file.file_identifier
+
+          cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
+        end
+
+        cache.write(key, cached_content, expires_in: 1.week)
+      end
+
+      def clear
+        cache.delete(key)
+      end
+
+      def key
+        [diffable, 'highlighted-diff-files', Gitlab::Diff::Line::SERIALIZE_KEYS, diff_options]
+      end
+
+      private
+
+      def read_file(diff_file)
+        cached_content[diff_file.file_identifier]
+      end
+
+      def cache
+        @backend
+      end
+
+      def cached_content
+        @cached_content ||= cache.read(key) || {}
+      end
+
+      def cacheable?(diff_file)
+        diffable.present? && diff_file.text? && diff_file.diffable?
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index c4288ca640894a..fabb5b272474a4 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -33,7 +33,8 @@ def real_size
         private
 
         def cache
-          @cache ||= Gitlab::Diff::HighlightCache.new(self)
+          # if Feature.enabled?(:redis_diff_caching)
+          @cache ||= Gitlab::Diff::DeprecatedHighlightCache.new(self)
         end
       end
     end
diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 21a29559be09a4..993561d9c3457c 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -11,7 +11,7 @@ class HighlightCache
       def initialize(diff_collection, backend: Rails.cache)
         @backend = backend
         @diff_collection = diff_collection
-        @redis_key = "highlighted-diff-files:#{diffable.cache_key}" if Feature.enabled?(:redis_diff_caching)
+        @redis_key = "highlighted-diff-files:#{diffable.cache_key}"
       end
 
       # - Reads from cache
@@ -38,11 +38,7 @@ def write_if_empty
           cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        if Feature.enabled?(:redis_diff_caching)
-          write_to_redis_hash(cached_content)
-        else
-          cache.write(key, cached_content, expires_in: 1.week)
-        end
+        write_to_redis_hash(cached_content)
       end
 
       # Given a hash of:
@@ -85,11 +81,9 @@ def read_single_entry_from_redis_hash(diff_file_id)
       end
 
       def clear
-        cache.delete(key)
-      end
-
-      def key
-        [diffable, 'highlighted-diff-files', Gitlab::Diff::Line::SERIALIZE_KEYS, diff_options]
+        Redis::Cache.with do |redis|
+          redis.del(@redis_key)
+        end
       end
 
       private
@@ -98,20 +92,12 @@ def read_file(diff_file)
         cached_content[diff_file.file_identifier]
       end
 
-      def cache
-        @backend
-      end
-
       def cached_content
         @cached_content ||= populate_cached_content
       end
 
       def populate_cached_content
-        if Feature.enabled?(:redis_diff_caching)
-          read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
-        else
-          cache.read(key) || {}
-        end
+        read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
       end
 
       def cacheable?(diff_file)
diff --git a/spec/lib/gitlab/diff/deprecated_highlight_cache_spec.rb b/spec/lib/gitlab/diff/deprecated_highlight_cache_spec.rb
new file mode 100644
index 00000000000000..7e46632ea776f5
--- /dev/null
+++ b/spec/lib/gitlab/diff/deprecated_highlight_cache_spec.rb
@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Diff::DeprecatedHighlightCache do
+  let(:merge_request) { create(:merge_request_with_diffs) }
+
+  subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
+
+  describe '#decorate' do
+    let(:backend) { double('backend').as_null_object }
+
+    # Manually creates a Diff::File object to avoid triggering the cache on
+    # the FileCollection::MergeRequestDiff
+    let(:diff_file) do
+      diffs = merge_request.diffs
+      raw_diff = diffs.diffable.raw_diffs(diffs.diff_options.merge(paths: ['CHANGELOG'])).first
+      Gitlab::Diff::File.new(raw_diff,
+                             repository: diffs.project.repository,
+                             diff_refs: diffs.diff_refs,
+                             fallback_diff_refs: diffs.fallback_diff_refs)
+    end
+
+    it 'does not calculate highlighting when reading from cache' do
+      cache.write_if_empty
+      cache.decorate(diff_file)
+
+      expect_any_instance_of(Gitlab::Diff::Highlight).not_to receive(:highlight)
+
+      diff_file.highlighted_diff_lines
+    end
+
+    it 'assigns highlighted diff lines to the DiffFile' do
+      cache.write_if_empty
+      cache.decorate(diff_file)
+
+      expect(diff_file.highlighted_diff_lines.size).to be > 5
+    end
+
+    it 'submits a single reading from the cache' do
+      cache.decorate(diff_file)
+      cache.decorate(diff_file)
+
+      expect(backend).to have_received(:read).with(cache.key).once
+    end
+  end
+
+  describe '#write_if_empty' do
+    let(:backend) { double('backend', read: {}).as_null_object }
+
+    it 'submits a single writing to the cache' do
+      cache.write_if_empty
+      cache.write_if_empty
+
+      expect(backend).to have_received(:write).with(cache.key,
+                                                    hash_including('CHANGELOG-false-false-false'),
+                                                    expires_in: 1.week).once
+    end
+  end
+
+  describe '#clear' do
+    let(:backend) { double('backend').as_null_object }
+
+    it 'clears cache' do
+      cache.clear
+
+      expect(backend).to have_received(:delete).with(cache.key)
+    end
+  end
+end
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index b4aa5e65155bf9..d42834068456f7 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -37,6 +37,8 @@
         new_pos: 20 }] }
   end
 
+  let(:cache_key) { cache.instance_variable_get(:@redis_key) }
+
   subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
 
   describe '#decorate' do
@@ -68,56 +70,22 @@
 
       expect(diff_file.highlighted_diff_lines.size).to be > 5
     end
-
-    context 'when :redis_diff_caching is not enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(false)
-      end
-
-      it 'submits a single reading from the cache' do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
-
-        2.times { cache.decorate(diff_file) }
-
-        expect(backend).to have_received(:read).with(cache.key).once
-      end
-    end
   end
 
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
     context 'when :redis_diff_caching is enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(true)
-      end
-
       it 'submits a single write action to the redis cache when invoked multiple times' do
         expect(cache).to receive(:write_to_redis_hash).once
 
         2.times { cache.write_if_empty }
       end
     end
-
-    context 'when :redis_diff_caching is not enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
-      end
-
-      it 'submits a single writing to the cache' do
-        2.times { cache.write_if_empty }
-
-        expect(backend)
-          .to have_received(:write)
-          .with(cache.key, hash_including('CHANGELOG-false-false-false'), expires_in: 1.week)
-          .once
-      end
-    end
   end
 
   describe '#write_to_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     it 'creates or updates a Redis hash' do
       expect { cache.write_to_redis_hash(diff_hash) }
@@ -127,7 +95,6 @@
 
   describe '#read_entire_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     before do
       cache.write_to_redis_hash(diff_hash)
@@ -142,7 +109,6 @@
 
   describe '#read_single_entry_from_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     before do
       cache.write_to_redis_hash(diff_hash)
@@ -161,9 +127,9 @@
     let(:backend) { double('backend').as_null_object }
 
     it 'clears cache' do
-      cache.clear
+      expect_any_instance_of(Redis).to receive(:del).with(cache_key)
 
-      expect(backend).to have_received(:delete).with(cache.key)
+      cache.clear
     end
   end
 end
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index cd4ea9c401d33c..bf3d663ec806cb 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -88,7 +88,7 @@
       end
 
       it 'clears noteable diff cache when it was unfolded for the note position' do
-        expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
+        expect_any_instance_of(Gitlab::Diff::DeprecatedHighlightCache).to receive(:clear)
 
         described_class.new(project_with_repo, user, new_opts).execute
       end
-- 
GitLab


From c3fd01907c49800d6b6db64f6e37650de83a583c Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 20 Nov 2019 03:37:32 -0800
Subject: [PATCH 11/38] Rewrite HighlightCache to use redis HSETs

---
 lib/gitlab/diff/highlight_cache.rb           | 47 +++++++++++---------
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 28 ++++++------
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 993561d9c3457c..2429238f8ba412 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -5,13 +5,14 @@ module Diff
     class HighlightCache
       EXPIRATION = 1.week
 
-      delegate :diffable, to: :@diff_collection
+      delegate :diffable,     to: :@diff_collection
       delegate :diff_options, to: :@diff_collection
 
       def initialize(diff_collection, backend: Rails.cache)
-        @backend = backend
-        @diff_collection = diff_collection
-        @redis_key = "highlighted-diff-files:#{diffable.cache_key}"
+        @backend          = backend
+        @diff_collection  = diff_collection
+        @redis_key        = "highlighted-diff-files:#{diffable.cache_key}"
+        @file_identifiers = @diff_collection.diff_files.collect(&:file_identifier)
       end
 
       # - Reads from cache
@@ -24,21 +25,19 @@ def decorate(diff_file)
         end
       end
 
-      # It populates a Hash in order to submit a single write to the memory
-      # cache. This avoids excessive IO generated by N+1's (1 writing for
-      # each highlighted line or file).
       def write_if_empty
-        return if cached_content.present?
+        diff_files = @diff_collection.diff_files
+        cached_diff_files = read_cache
+        uncached_files = diff_files.select { |file| cached_diff_files[file.file_identifier].nil? }
 
-        @diff_collection.diff_files.each do |diff_file|
+        new_cache_content = {}
+        uncached_files.each do |diff_file|
           next unless cacheable?(diff_file)
 
-          diff_file_id = diff_file.file_identifier
-
-          cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
+          new_cache_content[diff_file.file_identifier] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        write_to_redis_hash(cached_content)
+        write_to_redis_hash(new_cache_content)
       end
 
       # Given a hash of:
@@ -68,12 +67,6 @@ def write_to_redis_hash(hash)
         end
       end
 
-      def read_entire_redis_hash
-        Redis::Cache.with do |redis|
-          redis.hgetall(@redis_key )
-        end
-      end
-
       def read_single_entry_from_redis_hash(diff_file_id)
         Redis::Cache.with do |redis|
           redis.hget(@redis_key, diff_file_id)
@@ -93,11 +86,21 @@ def read_file(diff_file)
       end
 
       def cached_content
-        @cached_content ||= populate_cached_content
+        @cached_content ||= read_cache
       end
 
-      def populate_cached_content
-        read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
+      def read_cache
+        results = []
+
+        Redis::Cache.with do |redis|
+          results = redis.hmget(@redis_key, @file_identifiers)
+        end
+
+        results.map! do |result|
+          JSON.parse(result, symbolize_names: true) unless result.nil?
+        end
+
+        @file_identifiers.zip(results).to_h
       end
 
       def cacheable?(diff_file)
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index d42834068456f7..b2ff20cf8c785b 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -76,7 +76,7 @@
     let(:backend) { double('backend', read: {}).as_null_object }
 
     context 'when :redis_diff_caching is enabled' do
-      it 'submits a single write action to the redis cache when invoked multiple times' do
+      xit 'submits a single write action to the redis cache when invoked multiple times' do
         expect(cache).to receive(:write_to_redis_hash).once
 
         2.times { cache.write_if_empty }
@@ -93,19 +93,19 @@
     end
   end
 
-  describe '#read_entire_redis_hash' do
-    let(:backend) { Rails.cache }
-
-    before do
-      cache.write_to_redis_hash(diff_hash)
-    end
-
-    it 'returns the entire contents of a Redis hash as JSON' do
-      result = cache.read_entire_redis_hash
-
-      expect(result.values.first).to eq(diff_hash.values.first.to_json)
-    end
-  end
+  # describe '#read_entire_redis_hash' do
+  #   let(:backend) { Rails.cache }
+  #
+  #   before do
+  #     cache.write_to_redis_hash(diff_hash)
+  #   end
+  #
+  #   it 'returns the entire contents of a Redis hash as JSON' do
+  #     result = cache.read_entire_redis_hash
+  #
+  #     expect(result.values.first).to eq(diff_hash.values.first.to_json)
+  #   end
+  # end
 
   describe '#read_single_entry_from_redis_hash' do
     let(:backend) { Rails.cache }
-- 
GitLab


From 450c0d9043bf703b19baad520f4ae76fb5c53ab4 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 15:03:04 -0800
Subject: [PATCH 12/38] Extract file_identifiers to avoid looping

---
 lib/gitlab/diff/highlight_cache.rb | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 2429238f8ba412..f1b0ca4c56367a 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -12,7 +12,6 @@ def initialize(diff_collection, backend: Rails.cache)
         @backend          = backend
         @diff_collection  = diff_collection
         @redis_key        = "highlighted-diff-files:#{diffable.cache_key}"
-        @file_identifiers = @diff_collection.diff_files.collect(&:file_identifier)
       end
 
       # - Reads from cache
@@ -81,6 +80,10 @@ def clear
 
       private
 
+      def file_identifiers
+        @file_identifiers ||= @diff_collection.diff_files.collect(&:file_identifier)
+      end
+
       def read_file(diff_file)
         cached_content[diff_file.file_identifier]
       end
@@ -93,14 +96,14 @@ def read_cache
         results = []
 
         Redis::Cache.with do |redis|
-          results = redis.hmget(@redis_key, @file_identifiers)
+          results = redis.hmget(@redis_key, file_identifiers)
         end
 
         results.map! do |result|
           JSON.parse(result, symbolize_names: true) unless result.nil?
         end
 
-        @file_identifiers.zip(results).to_h
+        file_identifiers.zip(results).to_h
       end
 
       def cacheable?(diff_file)
-- 
GitLab


From 21e4c379ca2fb6423aa81d0508a928af5df8a75e Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 16:19:39 -0800
Subject: [PATCH 13/38] Key redis hash with file path

---
 lib/gitlab/diff/highlight_cache.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index f1b0ca4c56367a..0a3c656a56953c 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -27,13 +27,13 @@ def decorate(diff_file)
       def write_if_empty
         diff_files = @diff_collection.diff_files
         cached_diff_files = read_cache
-        uncached_files = diff_files.select { |file| cached_diff_files[file.file_identifier].nil? }
+        uncached_files = diff_files.select { |file| cached_diff_files[file.new_path].nil? }
 
         new_cache_content = {}
         uncached_files.each do |diff_file|
           next unless cacheable?(diff_file)
 
-          new_cache_content[diff_file.file_identifier] = diff_file.highlighted_diff_lines.map(&:to_hash)
+          new_cache_content[diff_file.new_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
         write_to_redis_hash(new_cache_content)
@@ -80,12 +80,12 @@ def clear
 
       private
 
-      def file_identifiers
-        @file_identifiers ||= @diff_collection.diff_files.collect(&:file_identifier)
+      def file_paths
+        @file_paths ||= @diff_collection.diffs.collect(&:new_path)
       end
 
       def read_file(diff_file)
-        cached_content[diff_file.file_identifier]
+        cached_content[diff_file.new_path]
       end
 
       def cached_content
@@ -96,14 +96,14 @@ def read_cache
         results = []
 
         Redis::Cache.with do |redis|
-          results = redis.hmget(@redis_key, file_identifiers)
+          results = redis.hmget(@redis_key, file_paths)
         end
 
         results.map! do |result|
           JSON.parse(result, symbolize_names: true) unless result.nil?
         end
 
-        file_identifiers.zip(results).to_h
+        file_paths.zip(results).to_h
       end
 
       def cacheable?(diff_file)
-- 
GitLab


From 918dd172a437aa07811135f5ad927a157c08e880 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 16:20:03 -0800
Subject: [PATCH 14/38] Feature flag use of new HighlightCache class

---
 lib/gitlab/diff/file_collection/merge_request_diff.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index fabb5b272474a4..a4c8910c2eb255 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -33,8 +33,11 @@ def real_size
         private
 
         def cache
-          # if Feature.enabled?(:redis_diff_caching)
-          @cache ||= Gitlab::Diff::DeprecatedHighlightCache.new(self)
+          @cache ||= if Feature.enabled?(:redis_diff_caching)
+                       Gitlab::Diff::HighlightCache.new(self)
+                     else
+                       Gitlab::Diff::DeprecatedHighlightCache.new(self)
+                     end
         end
       end
     end
-- 
GitLab


From c20a080f400397e374aee87c50995d7df998c19c Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 17:24:01 -0800
Subject: [PATCH 15/38] Guard against empty file_path

---
 lib/gitlab/diff/highlight_cache.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 0a3c656a56953c..cabeaa2fa67a0f 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -93,6 +93,8 @@ def cached_content
       end
 
       def read_cache
+        return {} unless file_paths.any?
+
         results = []
 
         Redis::Cache.with do |redis|
-- 
GitLab


From baedc0995ebafde6985b5253ef76a5a4840114ea Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 18:19:29 -0800
Subject: [PATCH 16/38] expose redis key

---
 lib/gitlab/diff/highlight_cache.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index cabeaa2fa67a0f..573b1f5f2cc727 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -78,6 +78,10 @@ def clear
         end
       end
 
+      def key
+        @redis_key
+      end
+
       private
 
       def file_paths
-- 
GitLab


From 7fe9058c348217d5c38b03a32520b759f143d788 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 21 Nov 2019 18:20:19 -0800
Subject: [PATCH 17/38] Scope tests against DeprecatedHighlightCache

---
 .../merge_request_diff_spec.rb                | 16 +++++++---
 .../reload_diffs_service_spec.rb              | 31 ++++++++++++++++---
 spec/services/notes/create_service_spec.rb    | 24 ++++++++++++--
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
index d89be6fef4e138..8a2d833c0189d1 100644
--- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
+++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
@@ -29,13 +29,19 @@
     let(:diffable) { merge_request.merge_request_diff }
   end
 
-  it 'uses a different cache key if diff line keys change' do
-    mr_diff = described_class.new(merge_request.merge_request_diff, diff_options: nil)
-    key = mr_diff.cache_key
+  context 'using Gitlab::Diff::DeprecatedHighlightCache' do
+    before do
+      stub_feature_flags(redis_diff_caching: false)
+    end
+
+    it 'uses a different cache key if diff line keys change' do
+      mr_diff = described_class.new(merge_request.merge_request_diff, diff_options: nil)
+      key = mr_diff.cache_key
 
-    stub_const('Gitlab::Diff::Line::SERIALIZE_KEYS', [:foo])
+      stub_const('Gitlab::Diff::Line::SERIALIZE_KEYS', [:foo])
 
-    expect(mr_diff.cache_key).not_to eq(key)
+      expect(mr_diff.cache_key).not_to eq(key)
+    end
   end
 
   it_behaves_like 'diff statistics' do
diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb
index cc21348ab11f71..1a1e7b29ac4228 100644
--- a/spec/services/merge_requests/reload_diffs_service_spec.rb
+++ b/spec/services/merge_requests/reload_diffs_service_spec.rb
@@ -33,13 +33,34 @@
     end
 
     context 'cache clearing' do
-      it 'clears the cache for older diffs on the merge request' do
-        old_diff = merge_request.merge_request_diff
-        old_cache_key = old_diff.diffs_collection.cache_key
+      context 'using Gitlab::Diff::DeprecatedHighlightCache' do
+        before do
+          stub_feature_flags(redis_diff_caching: false)
+        end
 
-        expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
+        it 'clears the cache for older diffs on the merge request' do
+          old_diff = merge_request.merge_request_diff
+          old_cache_key = old_diff.diffs_collection.cache_key
 
-        subject.execute
+          expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
+
+          subject.execute
+        end
+      end
+
+      context 'using Gitlab::Diff::HighlightCache' do
+        before do
+          stub_feature_flags(redis_diff_caching: true)
+        end
+
+        it 'clears the cache for older diffs on the merge request' do
+          old_diff = merge_request.merge_request_diff
+          old_cache_key = old_diff.diffs_collection.cache_key
+
+          expect_any_instance_of(Redis).to receive(:del).with(old_cache_key).and_call_original
+
+          subject.execute
+        end
       end
 
       it 'avoids N+1 queries', :request_store do
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index bf3d663ec806cb..fe98ec3e1f1911 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -87,10 +87,28 @@
           .to receive(:unfolded_diff?) { true }
       end
 
-      it 'clears noteable diff cache when it was unfolded for the note position' do
-        expect_any_instance_of(Gitlab::Diff::DeprecatedHighlightCache).to receive(:clear)
+      context 'using Gitlab::Diff::DeprecatedHighlightCache' do
+        before do
+          stub_feature_flags(redis_diff_caching: false)
+        end
+
+        it 'clears noteable diff cache when it was unfolded for the note position' do
+          expect_any_instance_of(Gitlab::Diff::DeprecatedHighlightCache).to receive(:clear)
+
+          described_class.new(project_with_repo, user, new_opts).execute
+        end
+      end
 
-        described_class.new(project_with_repo, user, new_opts).execute
+      context 'using Gitlab::Diff::HighlightCache' do
+        before do
+          stub_feature_flags(redis_diff_caching: true)
+        end
+
+        it 'clears noteable diff cache when it was unfolded for the note position' do
+          expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
+
+          described_class.new(project_with_repo, user, new_opts).execute
+        end
       end
 
       it 'does not clear cache when note is not the first of the discussion' do
-- 
GitLab


From 5d983135811493f93c1fb04bd73a603227aeb8cb Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 22 Nov 2019 07:59:34 -0800
Subject: [PATCH 18/38] Use a more-specific name for feature flag

---
 lib/gitlab/diff/file_collection/merge_request_diff.rb         | 2 +-
 .../gitlab/diff/file_collection/merge_request_diff_spec.rb    | 2 +-
 spec/lib/gitlab/diff/highlight_cache_spec.rb                  | 2 +-
 spec/services/merge_requests/reload_diffs_service_spec.rb     | 4 ++--
 spec/services/notes/create_service_spec.rb                    | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index a4c8910c2eb255..3d106a2bcc152b 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -33,7 +33,7 @@ def real_size
         private
 
         def cache
-          @cache ||= if Feature.enabled?(:redis_diff_caching)
+          @cache ||= if Feature.enabled?(:hset_redis_diff_caching)
                        Gitlab::Diff::HighlightCache.new(self)
                      else
                        Gitlab::Diff::DeprecatedHighlightCache.new(self)
diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
index 8a2d833c0189d1..7f207d5d2ee130 100644
--- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
+++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb
@@ -31,7 +31,7 @@
 
   context 'using Gitlab::Diff::DeprecatedHighlightCache' do
     before do
-      stub_feature_flags(redis_diff_caching: false)
+      stub_feature_flags(hset_redis_diff_caching: false)
     end
 
     it 'uses a different cache key if diff line keys change' do
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index b2ff20cf8c785b..292e8e9cfb6f81 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -75,7 +75,7 @@
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
-    context 'when :redis_diff_caching is enabled' do
+    context 'when :hset_redis_diff_caching is enabled' do
       xit 'submits a single write action to the redis cache when invoked multiple times' do
         expect(cache).to receive(:write_to_redis_hash).once
 
diff --git a/spec/services/merge_requests/reload_diffs_service_spec.rb b/spec/services/merge_requests/reload_diffs_service_spec.rb
index 1a1e7b29ac4228..c450fc0a7dceef 100644
--- a/spec/services/merge_requests/reload_diffs_service_spec.rb
+++ b/spec/services/merge_requests/reload_diffs_service_spec.rb
@@ -35,7 +35,7 @@
     context 'cache clearing' do
       context 'using Gitlab::Diff::DeprecatedHighlightCache' do
         before do
-          stub_feature_flags(redis_diff_caching: false)
+          stub_feature_flags(hset_redis_diff_caching: false)
         end
 
         it 'clears the cache for older diffs on the merge request' do
@@ -50,7 +50,7 @@
 
       context 'using Gitlab::Diff::HighlightCache' do
         before do
-          stub_feature_flags(redis_diff_caching: true)
+          stub_feature_flags(hset_redis_diff_caching: true)
         end
 
         it 'clears the cache for older diffs on the merge request' do
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index fe98ec3e1f1911..79ea8f48a589a8 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -89,7 +89,7 @@
 
       context 'using Gitlab::Diff::DeprecatedHighlightCache' do
         before do
-          stub_feature_flags(redis_diff_caching: false)
+          stub_feature_flags(hset_redis_diff_caching: false)
         end
 
         it 'clears noteable diff cache when it was unfolded for the note position' do
@@ -101,7 +101,7 @@
 
       context 'using Gitlab::Diff::HighlightCache' do
         before do
-          stub_feature_flags(redis_diff_caching: true)
+          stub_feature_flags(hset_redis_diff_caching: true)
         end
 
         it 'clears noteable diff cache when it was unfolded for the note position' do
-- 
GitLab


From 76f3f5b8bb1969924d481dec693a0473a0569463 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 22 Nov 2019 08:06:33 -0800
Subject: [PATCH 19/38] prefer file_path over new_path, as new_path might be
 nil

---
 lib/gitlab/diff/highlight_cache.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 573b1f5f2cc727..797bad2a1641f6 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -27,13 +27,13 @@ def decorate(diff_file)
       def write_if_empty
         diff_files = @diff_collection.diff_files
         cached_diff_files = read_cache
-        uncached_files = diff_files.select { |file| cached_diff_files[file.new_path].nil? }
+        uncached_files = diff_files.select { |file| cached_diff_files[file.file_path].nil? }
 
         new_cache_content = {}
         uncached_files.each do |diff_file|
           next unless cacheable?(diff_file)
 
-          new_cache_content[diff_file.new_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
+          new_cache_content[diff_file.file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
         write_to_redis_hash(new_cache_content)
@@ -85,11 +85,11 @@ def key
       private
 
       def file_paths
-        @file_paths ||= @diff_collection.diffs.collect(&:new_path)
+        @file_paths ||= @diff_collection.diffs.collect(&:file_path)
       end
 
       def read_file(diff_file)
-        cached_content[diff_file.new_path]
+        cached_content[diff_file.file_path]
       end
 
       def cached_content
-- 
GitLab


From 2413538d160fe897c7f852f8b53d75c0dd2d0108 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 22 Nov 2019 08:09:36 -0800
Subject: [PATCH 20/38] Move cache expiration setting outside iteration

---
 lib/gitlab/diff/highlight_cache.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 797bad2a1641f6..34ab4be1e4dc69 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -57,11 +57,11 @@ def write_to_redis_hash(hash)
           redis.multi do |multi|
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
               multi.hset(@redis_key, diff_file_id, highlighted_diff_lines_hash.to_json)
-
-              # HSETs have to have their expiration date manually updated
-              #
-              multi.expire(@redis_key, EXPIRATION)
             end
+
+            # HSETs have to have their expiration date manually updated
+            #
+            multi.expire(@redis_key, EXPIRATION)
           end
         end
       end
-- 
GitLab


From 10d88d8fef8d1dd408e5fcebda923208641dff8b Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 22 Nov 2019 08:44:18 -0800
Subject: [PATCH 21/38] Return early if there are no uncached files

---
 lib/gitlab/diff/highlight_cache.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 34ab4be1e4dc69..932456df564998 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -29,6 +29,8 @@ def write_if_empty
         cached_diff_files = read_cache
         uncached_files = diff_files.select { |file| cached_diff_files[file.file_path].nil? }
 
+        return if uncached_files.empty?
+
         new_cache_content = {}
         uncached_files.each do |diff_file|
           next unless cacheable?(diff_file)
-- 
GitLab


From a3f08d7d0bc416f3047c9e777bb42601bab29780 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 22 Nov 2019 09:10:41 -0800
Subject: [PATCH 22/38] Use a composite, versioned cache key

---
 lib/gitlab/diff/highlight_cache.rb           | 13 ++++++-------
 spec/lib/gitlab/diff/highlight_cache_spec.rb |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 932456df564998..42f0fcef88da56 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -11,7 +11,6 @@ class HighlightCache
       def initialize(diff_collection, backend: Rails.cache)
         @backend          = backend
         @diff_collection  = diff_collection
-        @redis_key        = "highlighted-diff-files:#{diffable.cache_key}"
       end
 
       # - Reads from cache
@@ -58,30 +57,30 @@ def write_to_redis_hash(hash)
         Redis::Cache.with do |redis|
           redis.multi do |multi|
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
-              multi.hset(@redis_key, diff_file_id, highlighted_diff_lines_hash.to_json)
+              multi.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
             end
 
             # HSETs have to have their expiration date manually updated
             #
-            multi.expire(@redis_key, EXPIRATION)
+            multi.expire(key, EXPIRATION)
           end
         end
       end
 
       def read_single_entry_from_redis_hash(diff_file_id)
         Redis::Cache.with do |redis|
-          redis.hget(@redis_key, diff_file_id)
+          redis.hget(key, diff_file_id)
         end
       end
 
       def clear
         Redis::Cache.with do |redis|
-          redis.del(@redis_key)
+          redis.del(key)
         end
       end
 
       def key
-        @redis_key
+        [diffable.head_commit_sha, VERSION, 'highlighted-diff-files', diff_options].join(":")
       end
 
       private
@@ -104,7 +103,7 @@ def read_cache
         results = []
 
         Redis::Cache.with do |redis|
-          results = redis.hmget(@redis_key, file_paths)
+          results = redis.hmget(key, file_paths)
         end
 
         results.map! do |result|
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 292e8e9cfb6f81..00aa22d8218faf 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -37,7 +37,7 @@
         new_pos: 20 }] }
   end
 
-  let(:cache_key) { cache.instance_variable_get(:@redis_key) }
+  let(:cache_key) { cache.key }
 
   subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
 
-- 
GitLab


From 0b6f53103d22eb251d8fb77410b64f3b7376c0a9 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 25 Nov 2019 12:59:50 -0800
Subject: [PATCH 23/38] Proper formatting of redis key

---
 lib/gitlab/diff/highlight_cache.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 42f0fcef88da56..13add1880644e9 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -80,7 +80,7 @@ def clear
       end
 
       def key
-        [diffable.head_commit_sha, VERSION, 'highlighted-diff-files', diff_options].join(":")
+        ['highlighted-diff-files', diffable, VERSION, diff_options].join(":")
       end
 
       private
-- 
GitLab


From 9b10875276328f0c9b681b4834f80a42e3cc696e Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 25 Nov 2019 13:00:11 -0800
Subject: [PATCH 24/38] Memoize redis key generation

---
 lib/gitlab/diff/highlight_cache.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 13add1880644e9..28ef74a104d735 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -80,7 +80,7 @@ def clear
       end
 
       def key
-        ['highlighted-diff-files', diffable, VERSION, diff_options].join(":")
+        @redis_key ||= ['highlighted-diff-files', diffable, VERSION, diff_options].join(":")
       end
 
       private
-- 
GitLab


From db1e309a54124f188bebba77fb818ad195fa23e1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 25 Nov 2019 13:39:10 -0800
Subject: [PATCH 25/38] Explicitly call #cache_key on diffable

---
 lib/gitlab/diff/highlight_cache.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 28ef74a104d735..eaa1a0ea15c324 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -80,7 +80,7 @@ def clear
       end
 
       def key
-        @redis_key ||= ['highlighted-diff-files', diffable, VERSION, diff_options].join(":")
+        @redis_key ||= ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":")
       end
 
       private
-- 
GitLab


From 79b963b22bfbd1cd25be8f464945d153b5b540b2 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 09:00:08 -0800
Subject: [PATCH 26/38] Cleanup formating of tests

---
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 22 +++-----------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 00aa22d8218faf..dc220938e6628f 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -75,12 +75,10 @@
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
-    context 'when :hset_redis_diff_caching is enabled' do
-      xit 'submits a single write action to the redis cache when invoked multiple times' do
-        expect(cache).to receive(:write_to_redis_hash).once
+    xit 'submits a single write action to the redis cache when invoked multiple times' do
+      expect(cache).to receive(:write_to_redis_hash).once
 
-        2.times { cache.write_if_empty }
-      end
+      2.times { cache.write_if_empty }
     end
   end
 
@@ -93,20 +91,6 @@
     end
   end
 
-  # describe '#read_entire_redis_hash' do
-  #   let(:backend) { Rails.cache }
-  #
-  #   before do
-  #     cache.write_to_redis_hash(diff_hash)
-  #   end
-  #
-  #   it 'returns the entire contents of a Redis hash as JSON' do
-  #     result = cache.read_entire_redis_hash
-  #
-  #     expect(result.values.first).to eq(diff_hash.values.first.to_json)
-  #   end
-  # end
-
   describe '#read_single_entry_from_redis_hash' do
     let(:backend) { Rails.cache }
 
-- 
GitLab


From 50dd9513a6bc69d751ff132e3849364f8798eaa9 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 09:03:22 -0800
Subject: [PATCH 27/38] Restrict feature flag by project

---
 lib/gitlab/diff/file_collection/merge_request_diff.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index 3d106a2bcc152b..c67c2159c3c041 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -33,7 +33,7 @@ def real_size
         private
 
         def cache
-          @cache ||= if Feature.enabled?(:hset_redis_diff_caching)
+          @cache ||= if Feature.enabled?(:hset_redis_diff_caching, project)
                        Gitlab::Diff::HighlightCache.new(self)
                      else
                        Gitlab::Diff::DeprecatedHighlightCache.new(self)
-- 
GitLab


From 33536df96a9965684f1dee3b3ca1bfb9fe152a0f Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 09:18:49 -0800
Subject: [PATCH 28/38] Update comment on #write_if_empty

---
 lib/gitlab/diff/highlight_cache.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index eaa1a0ea15c324..fae74659ea7931 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -15,6 +15,7 @@ def initialize(diff_collection, backend: Rails.cache)
 
       # - Reads from cache
       # - Assigns DiffFile#highlighted_diff_lines for cached files
+      #
       def decorate(diff_file)
         if content = read_file(diff_file)
           diff_file.highlighted_diff_lines = content.map do |line|
@@ -23,6 +24,11 @@ def decorate(diff_file)
         end
       end
 
+      # For every file that isn't already contained in the redis hash, store the
+      #   result of #highlighted_diff_lines, then submit the uncached content
+      #   to #write_to_redis_hash to submit a single write. This avoids excessive
+      #   IO generated by N+1's (1 writing for each highlighted line or file).
+      #
       def write_if_empty
         diff_files = @diff_collection.diff_files
         cached_diff_files = read_cache
-- 
GitLab


From f226967c1487dfff2efbb020812275e30094935d Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 09:35:51 -0800
Subject: [PATCH 29/38] Use local VERSION constant instead of Rails global

---
 lib/gitlab/diff/highlight_cache.rb | 1 +
 lib/gitlab/diff/line.rb            | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index fae74659ea7931..4633b0dc003d55 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -4,6 +4,7 @@ module Gitlab
   module Diff
     class HighlightCache
       EXPIRATION = 1.week
+      VERSION = 1
 
       delegate :diffable,     to: :@diff_collection
       delegate :diff_options, to: :@diff_collection
diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb
index 001748afb4166a..28ea1921f90e0c 100644
--- a/lib/gitlab/diff/line.rb
+++ b/lib/gitlab/diff/line.rb
@@ -3,6 +3,9 @@
 module Gitlab
   module Diff
     class Line
+      # When SERIALIZE_KEYS is updated, to reset the redis cache entries you'll
+      #   need to bump the VERSION constant on Gitlab::Diff::HighlightCache
+      #
       SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze
 
       attr_reader :line_code, :type, :old_pos, :new_pos
-- 
GitLab


From 82e3c537da3c2d0cf6f91958c47e5f3d79135838 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 11:21:52 -0800
Subject: [PATCH 30/38] Clarify spec by adding better test for expectations

---
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index dc220938e6628f..8ed8034e273516 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -75,8 +75,10 @@
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
-    xit 'submits a single write action to the redis cache when invoked multiple times' do
-      expect(cache).to receive(:write_to_redis_hash).once
+    it 'filters the key/value list of entries to be caches for each invocation' do
+      expect(cache).to receive(:write_to_redis_hash)
+        .once.with(hash_including(".gitignore")).and_call_original
+      expect(cache).to receive(:write_to_redis_hash).once.with({}).and_call_original
 
       2.times { cache.write_if_empty }
     end
-- 
GitLab


From 0c2fedc0ffbad1e41c70520e6eea2d86d47b7e2f Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 11:34:59 -0800
Subject: [PATCH 31/38] Remove unused method for retreiving single entries from
 cache

---
 lib/gitlab/diff/highlight_cache.rb           |  6 ------
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 16 ----------------
 2 files changed, 22 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 4633b0dc003d55..4e755fc358fa22 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -74,12 +74,6 @@ def write_to_redis_hash(hash)
         end
       end
 
-      def read_single_entry_from_redis_hash(diff_file_id)
-        Redis::Cache.with do |redis|
-          redis.hget(key, diff_file_id)
-        end
-      end
-
       def clear
         Redis::Cache.with do |redis|
           redis.del(key)
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 8ed8034e273516..8e0af4b31c3b44 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -93,22 +93,6 @@
     end
   end
 
-  describe '#read_single_entry_from_redis_hash' do
-    let(:backend) { Rails.cache }
-
-    before do
-      cache.write_to_redis_hash(diff_hash)
-    end
-
-    it 'returns highlighted diff content for a single file as JSON' do
-      diff_hash.each do |file_path, value|
-        found = cache.read_single_entry_from_redis_hash(file_path)
-
-        expect(found).to eq(value.to_json)
-      end
-    end
-  end
-
   describe '#clear' do
     let(:backend) { double('backend').as_null_object }
 
-- 
GitLab


From b82131becf9e7e36e489ad227258865896ab132b Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 11:44:18 -0800
Subject: [PATCH 32/38] More #write_to_redis_hash to private

---
 lib/gitlab/diff/highlight_cache.rb           | 24 ++++++++++----------
 spec/lib/gitlab/diff/highlight_cache_spec.rb |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 4e755fc358fa22..15d15349989306 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -47,6 +47,18 @@ def write_if_empty
         write_to_redis_hash(new_cache_content)
       end
 
+      def clear
+        Redis::Cache.with do |redis|
+          redis.del(key)
+        end
+      end
+
+      def key
+        @redis_key ||= ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":")
+      end
+
+      private
+
       # Given a hash of:
       #   { "file/to/cache" =>
       #   [ { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_19_19",
@@ -74,18 +86,6 @@ def write_to_redis_hash(hash)
         end
       end
 
-      def clear
-        Redis::Cache.with do |redis|
-          redis.del(key)
-        end
-      end
-
-      def key
-        @redis_key ||= ['highlighted-diff-files', diffable.cache_key, VERSION, diff_options].join(":")
-      end
-
-      private
-
       def file_paths
         @file_paths ||= @diff_collection.diffs.collect(&:file_path)
       end
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index 8e0af4b31c3b44..bf8359fe83cf73 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -88,7 +88,7 @@
     let(:backend) { Rails.cache }
 
     it 'creates or updates a Redis hash' do
-      expect { cache.write_to_redis_hash(diff_hash) }
+      expect { cache.send(:write_to_redis_hash, diff_hash) }
         .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache_key) } }
     end
   end
-- 
GitLab


From 4cd99b395eb49fea31bc95d43abf7351ef815424 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 12:53:28 -0800
Subject: [PATCH 33/38] Extract #uncached_files logic

---
 lib/gitlab/diff/highlight_cache.rb | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 15d15349989306..f3e8c38c2a9de7 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -31,10 +31,6 @@ def decorate(diff_file)
       #   IO generated by N+1's (1 writing for each highlighted line or file).
       #
       def write_if_empty
-        diff_files = @diff_collection.diff_files
-        cached_diff_files = read_cache
-        uncached_files = diff_files.select { |file| cached_diff_files[file.file_path].nil? }
-
         return if uncached_files.empty?
 
         new_cache_content = {}
@@ -59,6 +55,13 @@ def key
 
       private
 
+      def uncached_files
+        diff_files = @diff_collection.diff_files
+        cached_diff_files = read_cache
+
+        diff_files.select { |file| cached_diff_files[file.file_path].nil? }
+      end
+
       # Given a hash of:
       #   { "file/to/cache" =>
       #   [ { line_code: "a5cc2925ca8258af241be7e5b0381edf30266302_19_19",
-- 
GitLab


From 93676c829e251a4aaf157796b5ff45366074badb Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 26 Nov 2019 13:48:50 -0800
Subject: [PATCH 34/38] Test uncached files are added during #write_if_empty

---
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index bf8359fe83cf73..d369b1b2bf0317 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -73,7 +73,7 @@
   end
 
   describe '#write_if_empty' do
-    let(:backend) { double('backend', read: {}).as_null_object }
+    let(:backend) { Rails.cache }
 
     it 'filters the key/value list of entries to be caches for each invocation' do
       expect(cache).to receive(:write_to_redis_hash)
@@ -82,6 +82,19 @@
 
       2.times { cache.write_if_empty }
     end
+
+    context 'different diff_collections for the same diffable' do
+      before do
+        cache.write_if_empty
+      end
+
+      it 'writes an uncached files in the collection to the same redis hash' do
+        Gitlab::Redis::Cache.with { |r| r.hdel(cache_key, "files/whitespace") }
+
+        expect { cache.write_if_empty }
+          .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache_key) } }
+      end
+    end
   end
 
   describe '#write_to_redis_hash' do
-- 
GitLab


From c8e4f59a0fe627bdc1d8c2c22a87cf975a69d267 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 27 Nov 2019 07:34:38 -0800
Subject: [PATCH 35/38] Remove unrequired backend parameter

---
 lib/gitlab/diff/highlight_cache.rb           |  3 +--
 spec/lib/gitlab/diff/highlight_cache_spec.rb | 10 +---------
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index f3e8c38c2a9de7..d425229d363add 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -9,8 +9,7 @@ class HighlightCache
       delegate :diffable,     to: :@diff_collection
       delegate :diff_options, to: :@diff_collection
 
-      def initialize(diff_collection, backend: Rails.cache)
-        @backend          = backend
+      def initialize(diff_collection)
         @diff_collection  = diff_collection
       end
 
diff --git a/spec/lib/gitlab/diff/highlight_cache_spec.rb b/spec/lib/gitlab/diff/highlight_cache_spec.rb
index d369b1b2bf0317..97ebe6ae0e4a83 100644
--- a/spec/lib/gitlab/diff/highlight_cache_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_cache_spec.rb
@@ -39,11 +39,9 @@
 
   let(:cache_key) { cache.key }
 
-  subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
+  subject(:cache) { described_class.new(merge_request.diffs) }
 
   describe '#decorate' do
-    let(:backend) { double('backend').as_null_object }
-
     # Manually creates a Diff::File object to avoid triggering the cache on
     # the FileCollection::MergeRequestDiff
     let(:diff_file) do
@@ -73,8 +71,6 @@
   end
 
   describe '#write_if_empty' do
-    let(:backend) { Rails.cache }
-
     it 'filters the key/value list of entries to be caches for each invocation' do
       expect(cache).to receive(:write_to_redis_hash)
         .once.with(hash_including(".gitignore")).and_call_original
@@ -98,8 +94,6 @@
   end
 
   describe '#write_to_redis_hash' do
-    let(:backend) { Rails.cache }
-
     it 'creates or updates a Redis hash' do
       expect { cache.send(:write_to_redis_hash, diff_hash) }
         .to change { Gitlab::Redis::Cache.with { |r| r.hgetall(cache_key) } }
@@ -107,8 +101,6 @@
   end
 
   describe '#clear' do
-    let(:backend) { double('backend').as_null_object }
-
     it 'clears cache' do
       expect_any_instance_of(Redis).to receive(:del).with(cache_key)
 
-- 
GitLab


From 9f651dc1694a163de02db4eaa60240626482b0c9 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 27 Nov 2019 08:05:45 -0800
Subject: [PATCH 36/38] Remove temporary value

---
 lib/gitlab/diff/highlight_cache.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index d425229d363add..837d234a6b828d 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -56,9 +56,8 @@ def key
 
       def uncached_files
         diff_files = @diff_collection.diff_files
-        cached_diff_files = read_cache
 
-        diff_files.select { |file| cached_diff_files[file.file_path].nil? }
+        diff_files.select { |file| read_cache[file.file_path].nil? }
       end
 
       # Given a hash of:
-- 
GitLab


From 4a98138f7b903c43e7c656bf6892167b58c0f1b0 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 27 Nov 2019 08:06:06 -0800
Subject: [PATCH 37/38] Namespace cleanup for Gitlab::Redis::Cache

---
 lib/gitlab/diff/highlight_cache.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index 837d234a6b828d..db3bf1879d3db1 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -10,7 +10,7 @@ class HighlightCache
       delegate :diff_options, to: :@diff_collection
 
       def initialize(diff_collection)
-        @diff_collection  = diff_collection
+        @diff_collection = diff_collection
       end
 
       # - Reads from cache
@@ -43,7 +43,7 @@ def write_if_empty
       end
 
       def clear
-        Redis::Cache.with do |redis|
+        Gitlab::Redis::Cache.with do |redis|
           redis.del(key)
         end
       end
@@ -71,10 +71,10 @@ def uncached_files
       #       new_pos: 19 }
       #   ] }
       #
-      #   ...it will write/update a Redis hash (HSET)
+      #   ...it will write/update a Gitlab::Redis hash (HSET)
       #
       def write_to_redis_hash(hash)
-        Redis::Cache.with do |redis|
+        Gitlab::Redis::Cache.with do |redis|
           redis.multi do |multi|
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
               multi.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
@@ -104,7 +104,7 @@ def read_cache
 
         results = []
 
-        Redis::Cache.with do |redis|
+        Gitlab::Redis::Cache.with do |redis|
           results = redis.hmget(key, file_paths)
         end
 
-- 
GitLab


From 51b8a760b49dedc00b06ff3b02ecc2a23fbfa4c3 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 3 Dec 2019 06:32:33 -0800
Subject: [PATCH 38/38] Migrate from #multi to #pipelined for performance

Under the hood, #multi uses #pipelined, and we don't care about the
minor differences that using the #multi transaction approach gets us, so
we pick up about 0.1 sec of performance on large-ish diff collections.
This is a minor improvement, but as this is more or less a straight
drop-in replacement, and we're already workign directly with redis-rb,
it seems worthwhile.
---
 lib/gitlab/diff/highlight_cache.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/diff/highlight_cache.rb b/lib/gitlab/diff/highlight_cache.rb
index db3bf1879d3db1..12a1db70b36682 100644
--- a/lib/gitlab/diff/highlight_cache.rb
+++ b/lib/gitlab/diff/highlight_cache.rb
@@ -75,14 +75,14 @@ def uncached_files
       #
       def write_to_redis_hash(hash)
         Gitlab::Redis::Cache.with do |redis|
-          redis.multi do |multi|
+          redis.pipelined do
             hash.each do |diff_file_id, highlighted_diff_lines_hash|
-              multi.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
+              redis.hset(key, diff_file_id, highlighted_diff_lines_hash.to_json)
             end
 
             # HSETs have to have their expiration date manually updated
             #
-            multi.expire(key, EXPIRATION)
+            redis.expire(key, EXPIRATION)
           end
         end
       end
-- 
GitLab