From 6cd7f679d065e08f58d6dc9e2debf4f1a9cbcbe1 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 14 Mar 2018 16:03:10 +0000 Subject: [PATCH] Only cache highlight results for latest MR diffs Previously, we kept them all in the cache. We don't need the highlight results for older diffs - if someone does view that (which is rare), we can do the highlighting on the fly. --- app/models/merge_request.rb | 5 +-- .../merge_request_diff_cache_service.rb | 11 +++++- ...usage-from-merge-request-diffs-caching.yml | 5 +++ .../file_collection/merge_request_diff.rb | 12 ++++--- .../merge_request_diff_spec.rb | 2 +- spec/models/merge_request_spec.rb | 2 +- .../merge_request_diff_cache_service_spec.rb | 36 ++++++++++++++----- 7 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c2bae379a94..149ef7ec429 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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( diff --git a/app/services/merge_requests/merge_request_diff_cache_service.rb b/app/services/merge_requests/merge_request_diff_cache_service.rb index 2945a7fd4e4..10aa9ae609c 100644 --- a/app/services/merge_requests/merge_request_diff_cache_service.rb +++ b/app/services/merge_requests/merge_request_diff_cache_service.rb @@ -1,8 +1,17 @@ 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 diff --git a/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml b/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml new file mode 100644 index 00000000000..8fdca6eec83 --- /dev/null +++ b/changelogs/unreleased/44191-reduce-redis-usage-from-merge-request-diffs-caching.yml @@ -0,0 +1,5 @@ +--- +title: Stop caching highlighted diffs in Redis unnecessarily +merge_request: 17746 +author: +type: performance diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb index ff68bc7303a..9c1f85c70d6 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb @@ -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) @@ -70,10 +78,6 @@ module Gitlab 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 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 a067c42b75b..f48ee8924e8 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 @@ -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) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 7986aa31e16..4e783acbd8b 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -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 diff --git a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb index bb46e1dd9ab..33cba7225e3 100644 --- a/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb +++ b/spec/services/merge_requests/merge_request_diff_cache_service_spec.rb @@ -1,19 +1,39 @@ 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).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).and_call_original - subject.execute(merge_request) + subject.execute(merge_request, new_diff) end end end -- GitLab