From 6cd7f679d065e08f58d6dc9e2debf4f1a9cbcbe1 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 14 Mar 2018 16:03:10 +0000 Subject: [PATCH 1/2] 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 - subject.execute(merge_request) + 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, new_diff) end end end From db908826656b78e099ab5e1d99d4524ad0751d13 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 14 Mar 2018 16:05:06 +0000 Subject: [PATCH 2/2] Only cache MR diffs for one week This may lead to some being evicted and having to be cached again, but many MRs get closed or updated in that time anyway. --- lib/gitlab/diff/file_collection/merge_request_diff.rb | 2 +- .../merge_requests/merge_request_diff_cache_service_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb index 9c1f85c70d6..c358ae428cf 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb @@ -72,7 +72,7 @@ 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) 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 33cba7225e3..57b6165cfb0 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 @@ -15,7 +15,7 @@ describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_st 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 + expect(Rails.cache).to receive(:write).with(cache_key, anything, anything).and_call_original subject.execute(merge_request, new_diff) end @@ -31,7 +31,7 @@ describe MergeRequests::MergeRequestDiffCacheService, :use_clean_rails_memory_st 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 + expect(Rails.cache).to receive(:write).with(new_cache_key, anything, anything).and_call_original subject.execute(merge_request, new_diff) end