From 0f7ed33759a2d995043beaf6ed99dd6bdac62dc2 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Thu, 7 Mar 2019 17:19:56 -0800
Subject: [PATCH] Cache Repository#root_ref within a request

When an empty project is loaded in the UI, there are 15 separate Gitaly
FindDefaultBranch calls to determine the root_ref. Previously, it was
not cached even within the request. This change caches it within the
request so only a single FindDefaultBranch RPC is needed.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/58684
---
 app/models/repository.rb                      |  5 +-
 .../sh-cache-root-ref-asymetrically.yml       |  5 ++
 spec/models/repository_spec.rb                | 86 ++++++++++---------
 3 files changed, 53 insertions(+), 43 deletions(-)
 create mode 100644 changelogs/unreleased/sh-cache-root-ref-asymetrically.yml

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 851175a61b7..285fce1e6dd 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -534,10 +534,9 @@ class Repository
   end
 
   def root_ref
-    # When the repo does not exist, or there is no root ref, we raise this error so no data is cached.
-    raw_repository&.root_ref or raise Gitlab::Git::Repository::NoRepository # rubocop:disable Style/AndOr
+    raw_repository&.root_ref
   end
-  cache_method :root_ref
+  cache_method_asymmetrically :root_ref
 
   # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/314
   def exists?
diff --git a/changelogs/unreleased/sh-cache-root-ref-asymetrically.yml b/changelogs/unreleased/sh-cache-root-ref-asymetrically.yml
new file mode 100644
index 00000000000..106d070cc05
--- /dev/null
+++ b/changelogs/unreleased/sh-cache-root-ref-asymetrically.yml
@@ -0,0 +1,5 @@
+---
+title: Cache Repository#root_ref within a request
+merge_request: 25903
+author:
+type: performance
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 70630467d24..6599b4e765a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1095,6 +1095,49 @@ describe Repository do
     end
   end
 
+  shared_examples 'asymmetric cached method' do |method|
+    context 'asymmetric caching', :use_clean_rails_memory_store_caching, :request_store do
+      let(:cache) { repository.send(:cache) }
+      let(:request_store_cache) { repository.send(:request_store_cache) }
+
+      context 'when it returns true' do
+        before do
+          expect(repository.raw_repository).to receive(method).once.and_return(true)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.send(method)
+          end.to change { request_store_cache.read(method) }.from(nil).to(true)
+        end
+
+        it 'caches the output in RepositoryCache' do
+          expect do
+            repository.send(method)
+          end.to change { cache.read(method) }.from(nil).to(true)
+        end
+      end
+
+      context 'when it returns false' do
+        before do
+          expect(repository.raw_repository).to receive(method).once.and_return(false)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.send(method)
+          end.to change { request_store_cache.read(method) }.from(nil).to(false)
+        end
+
+        it 'does NOT cache the output in RepositoryCache' do
+          expect do
+            repository.send(method)
+          end.not_to change { cache.read(method) }.from(nil)
+        end
+      end
+    end
+  end
+
   describe '#exists?' do
     it 'returns true when a repository exists' do
       expect(repository.exists?).to be(true)
@@ -1112,46 +1155,7 @@ describe Repository do
       end
     end
 
-    context 'asymmetric caching', :use_clean_rails_memory_store_caching, :request_store do
-      let(:cache) { repository.send(:cache) }
-      let(:request_store_cache) { repository.send(:request_store_cache) }
-
-      context 'when it returns true' do
-        before do
-          expect(repository.raw_repository).to receive(:exists?).once.and_return(true)
-        end
-
-        it 'caches the output in RequestStore' do
-          expect do
-            repository.exists?
-          end.to change { request_store_cache.read(:exists?) }.from(nil).to(true)
-        end
-
-        it 'caches the output in RepositoryCache' do
-          expect do
-            repository.exists?
-          end.to change { cache.read(:exists?) }.from(nil).to(true)
-        end
-      end
-
-      context 'when it returns false' do
-        before do
-          expect(repository.raw_repository).to receive(:exists?).once.and_return(false)
-        end
-
-        it 'caches the output in RequestStore' do
-          expect do
-            repository.exists?
-          end.to change { request_store_cache.read(:exists?) }.from(nil).to(false)
-        end
-
-        it 'does NOT cache the output in RepositoryCache' do
-          expect do
-            repository.exists?
-          end.not_to change { cache.read(:exists?) }.from(nil)
-        end
-      end
-    end
+    it_behaves_like 'asymmetric cached method', :exists?
   end
 
   describe '#has_visible_content?' do
@@ -1271,6 +1275,8 @@ describe Repository do
       repository.root_ref
       repository.root_ref
     end
+
+    it_behaves_like 'asymmetric cached method', :root_ref
   end
 
   describe '#expire_root_ref_cache' do