diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2017-12-11 16:38:16 +0100 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2017-12-12 16:28:26 +0100 |
commit | 3ab026b7d69ec97796fe4bb409933d7a716eef19 (patch) | |
tree | 6711c10cf577250647330b1d2725e13ce2177a66 | |
parent | 806a68a81f1baeed07c146b1b5d9eb77796c46ba (diff) | |
download | gitlab-ce-3ab026b7d69ec97796fe4bb409933d7a716eef19.tar.gz |
Use memoization for commits on diffs
The Gitaly CommitService is being hammered by n + 1 calls, mostly when
finding commits. This leads to this gRPC being turned of on production:
https://gitlab.com/gitlab-org/gitaly/issues/514#note_48991378
Hunting down where it came from, most of them were due to
MergeRequest#show. To prove this, I set a script to request the
MergeRequest#show page 50 times. The GDK was being scraped by
Prometheus, where we have metrics on controller#action and their Gitaly
calls performed. On both occations I've restarted the full GDK so all
caches had to be rebuild.
Current master, 806a68a81f1baee, needed 435 requests
After this commit, 154 requests
-rw-r--r-- | app/models/merge_request.rb | 21 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 6 | ||||
-rw-r--r-- | changelogs/unreleased/zj-memoization-mr-commits.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/conflict/file_collection.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/utils/strong_memoize.rb | 20 | ||||
-rw-r--r-- | spec/lib/gitlab/utils/strong_memoize_spec.rb | 13 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 4 |
7 files changed, 58 insertions, 13 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 422f138c4ea..26a3388602a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base include ManualInverseAssociation include EachBatch include ThrottledTouch + include Gitlab::Utils::StrongMemoize ignore_column :locked_at, :ref_fetched @@ -53,6 +54,7 @@ class MergeRequest < ActiveRecord::Base after_create :ensure_merge_request_diff, unless: :importing? after_update :reload_diff_if_branch_changed + after_update :clear_memoized_shas # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests @@ -387,13 +389,17 @@ class MergeRequest < ActiveRecord::Base end def source_branch_head - return unless source_project - - source_project.repository.commit(source_branch_ref) if source_branch_ref + strong_memoize(:source_branch_head) do + if source_project && source_branch_ref + source_project.repository.commit(source_branch_ref) + end + end end def target_branch_head - target_project.repository.commit(target_branch_ref) + strong_memoize(:target_branch_head) do + target_project.repository.commit(target_branch_ref) + end end def branch_merge_base_commit @@ -525,6 +531,13 @@ class MergeRequest < ActiveRecord::Base end end + def clear_memoized_shas + @target_branch_sha = @source_branch_sha = nil + + clear_memoization(:source_branch_head) + clear_memoization(:target_branch_head) + end + def reload_diff_if_branch_changed if (source_branch_changed? || target_branch_changed?) && (source_branch_head && target_branch_head) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index c37aa0a594b..e35de9b97ee 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -104,19 +104,19 @@ class MergeRequestDiff < ActiveRecord::Base def base_commit return unless base_commit_sha - project.commit(base_commit_sha) + project.commit_by(oid: base_commit_sha) end def start_commit return unless start_commit_sha - project.commit(start_commit_sha) + project.commit_by(oid: start_commit_sha) end def head_commit return unless head_commit_sha - project.commit(head_commit_sha) + project.commit_by(oid: head_commit_sha) end def commit_shas diff --git a/changelogs/unreleased/zj-memoization-mr-commits.yml b/changelogs/unreleased/zj-memoization-mr-commits.yml new file mode 100644 index 00000000000..59dfc6d6049 --- /dev/null +++ b/changelogs/unreleased/zj-memoization-mr-commits.yml @@ -0,0 +1,5 @@ +--- +title: Cache commits for MergeRequest diffs +merge_request: +author: +type: performance diff --git a/lib/gitlab/conflict/file_collection.rb b/lib/gitlab/conflict/file_collection.rb index fb28e80ff73..b9099ce256a 100644 --- a/lib/gitlab/conflict/file_collection.rb +++ b/lib/gitlab/conflict/file_collection.rb @@ -19,6 +19,8 @@ module Gitlab commit_message: commit_message || default_commit_message } resolver.resolve_conflicts(user, files, args) + ensure + @merge_request.clear_memoized_shas end def files diff --git a/lib/gitlab/utils/strong_memoize.rb b/lib/gitlab/utils/strong_memoize.rb index a2ac9285b56..fe091f4611b 100644 --- a/lib/gitlab/utils/strong_memoize.rb +++ b/lib/gitlab/utils/strong_memoize.rb @@ -11,6 +11,8 @@ module Gitlab # # We could write it like: # + # include Gitlab::Utils::StrongMemoize + # # def trigger_from_token # strong_memoize(:trigger) do # Ci::Trigger.find_by_token(params[:token].to_s) @@ -18,14 +20,22 @@ module Gitlab # end # def strong_memoize(name) - ivar_name = "@#{name}" - - if instance_variable_defined?(ivar_name) - instance_variable_get(ivar_name) + if instance_variable_defined?(ivar(name)) + instance_variable_get(ivar(name)) else - instance_variable_set(ivar_name, yield) + instance_variable_set(ivar(name), yield) end end + + def clear_memoization(name) + remove_instance_variable(ivar(name)) if instance_variable_defined?(ivar(name)) + end + + private + + def ivar(name) + "@#{name}" + end end end end diff --git a/spec/lib/gitlab/utils/strong_memoize_spec.rb b/spec/lib/gitlab/utils/strong_memoize_spec.rb index 4a104ab6d97..474532dd82b 100644 --- a/spec/lib/gitlab/utils/strong_memoize_spec.rb +++ b/spec/lib/gitlab/utils/strong_memoize_spec.rb @@ -49,4 +49,17 @@ describe Gitlab::Utils::StrongMemoize do end end end + + describe '#clear_memoization' do + let(:value) { 'mepmep' } + + it 'removes the instance variable' do + object.method_name + + object.clear_memoization(:method_name) + + expect(object.instance_variable_defined?(:@method_name)).to be(false) + end + end end + diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 30a5a3bbff7..6b98d013ded 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -124,6 +124,7 @@ describe MergeRequest do context 'when the target branch does not exist' do before do project.repository.rm_branch(subject.author, subject.target_branch) + subject.clear_memoized_shas end it 'returns nil' do @@ -733,7 +734,7 @@ describe MergeRequest do before do project.repository.raw_repository.delete_branch(subject.target_branch) - subject.reload + subject.clear_memoized_shas end it 'does not crash' do @@ -1468,6 +1469,7 @@ describe MergeRequest do context 'when the target branch does not exist' do before do subject.project.repository.rm_branch(subject.author, subject.target_branch) + subject.clear_memoized_shas end it 'returns nil' do |