summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorZeger-Jan van de Weg <git@zjvandeweg.nl>2017-12-11 16:38:16 +0100
committerZeger-Jan van de Weg <git@zjvandeweg.nl>2017-12-12 16:28:26 +0100
commit3ab026b7d69ec97796fe4bb409933d7a716eef19 (patch)
tree6711c10cf577250647330b1d2725e13ce2177a66
parent806a68a81f1baeed07c146b1b5d9eb77796c46ba (diff)
downloadgitlab-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.rb21
-rw-r--r--app/models/merge_request_diff.rb6
-rw-r--r--changelogs/unreleased/zj-memoization-mr-commits.yml5
-rw-r--r--lib/gitlab/conflict/file_collection.rb2
-rw-r--r--lib/gitlab/utils/strong_memoize.rb20
-rw-r--r--spec/lib/gitlab/utils/strong_memoize_spec.rb13
-rw-r--r--spec/models/merge_request_spec.rb4
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