Read about our upcoming Code of Conduct on this issue

Commit be6a1265 authored by Stan Hu's avatar Stan Hu
Browse files

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
......@@ -579,9 +579,10 @@ def reload_diff(current_user = nil)
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 @@ def real_size
@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 @@ def highlight_cache
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 @@
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 @@ def set_compare(merge_request)
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