Commit 49d5c097 authored by Stan Hu's avatar Stan Hu

Merge branch '44191-reduce-redis-usage-from-merge-request-diffs-caching' into 'master'

Resolve "Reduce Redis usage from merge request diffs caching"

Closes #44191

See merge request gitlab-org/gitlab-ce!17746
parents b53c4277 db908826
Pipeline #18970390 passed with stages
in 40 minutes and 52 seconds
......@@ -579,9 +579,10 @@ class MergeRequest < ActiveRecord::Base
return unless open?
old_diff_refs = self.diff_refs
new_diff = create_merge_request_diff
MergeRequests::MergeRequestDiffCacheService.new.execute(self, new_diff)
create_merge_request_diff
MergeRequests::MergeRequestDiffCacheService.new.execute(self)
new_diff_refs = self.diff_refs
update_diff_discussion_positions(
......
module MergeRequests
class MergeRequestDiffCacheService
def execute(merge_request)
def execute(merge_request, new_diff)
# Executing the iteration we cache all the highlighted diff information
merge_request.diffs.diff_files.to_a
# Remove cache for all diffs on this MR. Do not use the association on the
# model, as that will interfere with other actions happening when
# reloading the diff.
MergeRequestDiff.where(merge_request: merge_request).each do |merge_request_diff|
next if merge_request_diff == new_diff
merge_request_diff.diffs.clear_cache!
end
end
end
end
---
title: Stop caching highlighted diffs in Redis unnecessarily
merge_request: 17746
author:
type: performance
......@@ -29,6 +29,14 @@ module Gitlab
@merge_request_diff.real_size
end
def clear_cache!
Rails.cache.delete(cache_key)
end
def cache_key
[@merge_request_diff, 'highlighted-diff-files', diff_options]
end
private
def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
......@@ -64,16 +72,12 @@ module Gitlab
end
def store_highlight_cache
Rails.cache.write(cache_key, highlight_cache) if @highlight_cache_was_empty
Rails.cache.write(cache_key, highlight_cache, expires_in: 1.week) if @highlight_cache_was_empty
end
def cacheable?(diff_file)
@merge_request_diff.present? && diff_file.text? && diff_file.diffable?
end
def cache_key
[@merge_request_diff, 'highlighted-diff-files', diff_options]
end
end
end
end
......
......@@ -12,7 +12,7 @@ describe Gitlab::Diff::FileCollection::MergeRequestDiff do
diff_files
end
it 'does not files marked as undiffable in .gitattributes' do
it 'does not highlight files marked as undiffable in .gitattributes' do
allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(false)
expect_any_instance_of(Gitlab::Diff::File).not_to receive(:highlighted_diff_lines)
......
......@@ -1544,7 +1544,7 @@ describe MergeRequest do
end
it "executes diff cache service" do
expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject, an_instance_of(MergeRequestDiff))
subject.reload_diff
end
......
require 'spec_helper'
describe MergeRequests::MergeRequestDiffCacheService do
describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_store_caching do
let(:subject) { described_class.new }
let(:merge_request) { create(:merge_request) }
describe '#execute' do
it 'retrieves 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::MergeRequestDiff.default_options]
expect(Rails.cache).to receive(:read).with(cache_key).and_return({})
expect(Rails.cache).to receive(:write).with(cache_key, anything)
before do
allow_any_instance_of(Gitlab::Diff::File).to receive(:text?).and_return(true)
allow_any_instance_of(Gitlab::Diff::File).to receive(:diffable?).and_return(true)
end
it 'retrieves the diff files to cache the highlighted result' do
new_diff = merge_request.merge_request_diff
cache_key = new_diff.diffs.cache_key
expect(Rails.cache).to receive(:read).with(cache_key).and_call_original
expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original
subject.execute(merge_request, new_diff)
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.cache_key
subject.execute(merge_request, old_diff)
new_diff = merge_request.create_merge_request_diff
new_cache_key = new_diff.diffs.cache_key
expect(Rails.cache).to receive(:delete).with(old_cache_key).and_call_original
expect(Rails.cache).to receive(:read).with(new_cache_key).and_call_original
expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original
subject.execute(merge_request)
subject.execute(merge_request, new_diff)
end
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment