From b1f15dfa42fb7b8f74a439806f004eaa5ed598a8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 14 Apr 2018 14:34:39 -0700 Subject: [PATCH 1/3] Memoize Git::Repository#has_visible_content? This is called repeatedly when viewing a merge request, and this should improve performance significantly by avoiding shelling out to git every time. This should help https://gitlab.com/gitlab-com/infrastructure/issues/4027. --- .../unreleased/sh-memoize-repository-empty.yml | 5 +++++ lib/gitlab/git/repository.rb | 7 +++++++ spec/lib/gitlab/git/repository_spec.rb | 12 +++++++++++- 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-memoize-repository-empty.yml diff --git a/changelogs/unreleased/sh-memoize-repository-empty.yml b/changelogs/unreleased/sh-memoize-repository-empty.yml new file mode 100644 index 00000000000..64db3ca0371 --- /dev/null +++ b/changelogs/unreleased/sh-memoize-repository-empty.yml @@ -0,0 +1,5 @@ +--- +title: Memoize Git::Repository#has_visible_content? +merge_request: +author: +type: performance diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 11a421cb430..61e92a78ab9 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -9,6 +9,7 @@ module Gitlab include Gitlab::Git::RepositoryMirroring include Gitlab::Git::Popen include Gitlab::EncodingHelper + include Gitlab::Utils::StrongMemoize ALLOWED_OBJECT_DIRECTORIES_VARIABLES = %w[ GIT_OBJECT_DIRECTORY @@ -232,6 +233,12 @@ module Gitlab end def has_local_branches? + strong_memoize(:has_local_branches) do + uncached_has_local_branches? + end + end + + def uncached_has_local_branches? gitaly_migrate(:has_local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| if is_enabled gitaly_repository_client.has_local_branches? diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 1e00e8d2739..f6bf7a162f6 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -463,7 +463,7 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'returns false when there are no branches' do # Sanity check - expect(repository.has_local_branches?).to eq(true) + expect(repository.uncached_has_local_branches?).to eq(true) FileUtils.rm_rf(File.join(repository.path, 'packed-refs')) heads_dir = File.join(repository.path, 'refs/heads') @@ -473,6 +473,16 @@ describe Gitlab::Git::Repository, seed_helper: true do expect(repository.has_local_branches?).to eq(false) end end + + context 'memoizes the value' do + it 'returns true' do + expect(repository).to receive(:uncached_has_local_branches?).once.and_call_original + + 2.times do + expect(repository.has_local_branches?).to eq(true) + end + end + end end context 'with gitaly' do From 74e5ec198cbafe5d83690fae970ff73e5ef4cfcb Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 14 Apr 2018 15:36:36 -0700 Subject: [PATCH 2/3] Fix failing ./spec/lib/backup/repository_spec.rb by clearing the memoized value --- app/models/repository.rb | 1 + lib/gitlab/git/repository.rb | 4 ++++ spec/lib/gitlab/git/repository_spec.rb | 3 ++- spec/models/repository_spec.rb | 6 ++++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index fd1afafe4df..5bdaa7f0720 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -331,6 +331,7 @@ class Repository return unless empty? expire_method_caches(%i(has_visible_content?)) + raw_repository.expire_has_local_branches_cache end def lookup_cache diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 61e92a78ab9..265707b1d09 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -232,6 +232,10 @@ module Gitlab end end + def expire_has_local_branches_cache + clear_memoization(:has_local_branches) + end + def has_local_branches? strong_memoize(:has_local_branches) do uncached_has_local_branches? diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index f6bf7a162f6..5acf40ea5ce 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -463,13 +463,14 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'returns false when there are no branches' do # Sanity check - expect(repository.uncached_has_local_branches?).to eq(true) + expect(repository.has_local_branches?).to eq(true) FileUtils.rm_rf(File.join(repository.path, 'packed-refs')) heads_dir = File.join(repository.path, 'refs/heads') FileUtils.rm_rf(heads_dir) FileUtils.mkdir_p(heads_dir) + repository.expire_has_local_branches_cache expect(repository.has_local_branches?).to eq(false) end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 60ab52565cb..e45fe7db1e7 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1437,6 +1437,12 @@ describe Repository do repository.expire_emptiness_caches end + + it 'expires the memoized repository cache' do + allow(repository.raw_repository).to receive(:expire_has_local_branches_cache).and_call_original + + repository.expire_emptiness_caches + end end describe 'skip_merges option' do From cf955af157cb8c2edb05e90d221689d21a1daee3 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 14 Apr 2018 15:38:53 -0700 Subject: [PATCH 3/3] Move uncached_has_local_branches? to a private method --- lib/gitlab/git/repository.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 265707b1d09..294475be9c6 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -242,16 +242,6 @@ module Gitlab end end - def uncached_has_local_branches? - gitaly_migrate(:has_local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| - if is_enabled - gitaly_repository_client.has_local_branches? - else - has_local_branches_rugged? - end - end - end - # Git repository can contains some hidden refs like: # /refs/notes/* # /refs/git-as-svn/* @@ -1570,6 +1560,16 @@ module Gitlab private + def uncached_has_local_branches? + gitaly_migrate(:has_local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| + if is_enabled + gitaly_repository_client.has_local_branches? + else + has_local_branches_rugged? + end + end + end + def local_write_ref(ref_path, ref, old_ref: nil, shell: true) if shell shell_write_ref(ref_path, ref, old_ref)