Cache various Repository Git operationscaching-repository-git-operations

This caches the output of the following methods:

* Repository#empty?
* Repository#has_visible_content?
* Repository#root_ref

The cache for Repository#has_visible_content? is flushed whenever a
commit is pushed to a new branch or an existing branch is removed.
The cache for Repository#root_ref is only flushed whenever a user
changes the default branch of a project. The cache for Repository#empty?
is never explicitly flushed as there's no need for it.

6 files changed, 140 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 983a219703a..abcec42e0fb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.5.0 (unreleased)
+  - Cache various Repository methods to improve performance (Yorick Peterse)
   - Ensure rake tasks that don't need a DB connection can be run without one
   - Update New Relic gem to 3.14.1.311 (Stan Hu)
   - Add "visibility" flag to GET /projects api endpoint
diff --git a/app/models/project.rb b/app/models/project.rb
index 043f08b9a13..f11c6d7c6be 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -790,6 +790,8 @@ class Project < ActiveRecord::Base
   def change_head(branch)
     # Cached divergent commit counts are based on repository head
     repository.expire_branch_cache
+    repository.expire_root_ref_cache
+
     gitlab_shell.update_repository_head(self.path_with_namespace, branch)
     reload_default_branch
   end
diff --git a/app/models/repository.rb b/app/models/repository.rb
index e813c946bc1..27bdbac3e52 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -44,7 +44,9 @@ class Repository
   end
 
   def empty?
-    raw_repository.empty?
+    return @empty unless @empty.nil?
+
+    @empty = cache.fetch(:empty?) { raw_repository.empty? }
   end
 
   #
@@ -57,7 +59,11 @@ class Repository
   # This method return true if repository contains some content visible in project page.
   #
   def has_visible_content?
-    raw_repository.branch_count > 0
+    return @has_visible_content unless @has_visible_content.nil?
+
+    @has_visible_content = cache.fetch(:has_visible_content?) do
+      raw_repository.branch_count > 0
+    end
   end
 
   def commit(id = 'HEAD')
@@ -243,6 +249,16 @@ class Repository
     end
   end
 
+  def expire_root_ref_cache
+    cache.expire(:root_ref)
+    @root_ref = nil
+  end
+
+  def expire_has_visible_content_cache
+    cache.expire(:has_visible_content?)
+    @has_visible_content = nil
+  end
+
   def rebuild_cache
     cache_keys.each do |key|
       cache.expire(key)
@@ -480,7 +496,7 @@ class Repository
   end
 
   def root_ref
-    @root_ref ||= raw_repository.root_ref
+    @root_ref ||= cache.fetch(:root_ref) { raw_repository.root_ref }
   end
 
   def commit_dir(user, path, message, branch)
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index d7ea30bc315..0f31756797d 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -21,8 +21,12 @@ class GitPushService
     project.repository.expire_cache
 
     if push_remove_branch?(ref, newrev)
+      project.repository.expire_has_visible_content_cache
+
       @push_commits = []
     elsif push_to_new_branch?(ref, oldrev)
+      project.repository.expire_has_visible_content_cache
+
       # Re-find the pushed commits.
       if is_default_branch?(ref)
         # Initial push to the default branch. Take the full history of that branch as "newly pushed".
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index c484ae8fc8c..753012be578 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -232,11 +232,92 @@ describe Repository, models: true do
     end
 
     describe 'when there are branches' do
-      before do
-        allow(repository.raw_repository).to receive(:branch_count).and_return(3)
+      it 'returns true' do
+        expect(repository.raw_repository).to receive(:branch_count).and_return(3)
+
+        expect(subject).to eq(true)
+      end
+
+      it 'caches the output' do
+        expect(repository.raw_repository).to receive(:branch_count).
+          once.
+          and_return(3)
+
+        repository.has_visible_content?
+        repository.has_visible_content?
       end
+    end
+  end
+
+  describe '#empty?' do
+    let(:empty_repository) { create(:project_empty_repo).repository }
+
+    it 'returns true for an empty repository' do
+      expect(empty_repository.empty?).to eq(true)
+    end
+
+    it 'returns false for a non-empty repository' do
+      expect(repository.empty?).to eq(false)
+    end
+
+    it 'caches the output' do
+      expect(repository.raw_repository).to receive(:empty?).
+        once.
+        and_return(false)
+
+      repository.empty?
+      repository.empty?
+    end
+  end
+
+  describe '#root_ref' do
+    it 'returns a branch name' do
+      expect(repository.root_ref).to be_an_instance_of(String)
+    end
+
+    it 'caches the output' do
+      expect(repository.raw_repository).to receive(:root_ref).
+        once.
+        and_return('master')
+
+      repository.root_ref
+      repository.root_ref
+    end
+  end
+
+  describe '#expire_cache' do
+    it 'expires all caches' do
+      expect(repository).to receive(:expire_branch_cache)
+
+      repository.expire_cache
+    end
+  end
+
+  describe '#expire_root_ref_cache' do
+    it 'expires the root reference cache' do
+      repository.root_ref
+
+      expect(repository.raw_repository).to receive(:root_ref).
+        once.
+        and_return('foo')
+
+      repository.expire_root_ref_cache
+
+      expect(repository.root_ref).to eq('foo')
+    end
+  end
+
+  describe '#expire_has_visible_content_cache' do
+    it 'expires the visible content cache' do
+      repository.has_visible_content?
+
+      expect(repository.raw_repository).to receive(:branch_count).
+        once.
+        and_return(0)
+
+      repository.expire_has_visible_content_cache
 
-      it { is_expected.to eq(true) }
+      expect(repository.has_visible_content?).to eq(false)
     end
   end
 end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index c1080ef190a..349809743c7 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -21,6 +21,18 @@ describe GitPushService, services: true do
       end
 
       it { is_expected.to be_truthy }
+
+      it 'flushes general cached data' do
+        expect(project.repository).to receive(:expire_cache)
+
+        subject
+      end
+
+      it 'flushes the visible content cache' do
+        expect(project.repository).to receive(:expire_has_visible_content_cache)
+
+        subject
+      end
     end
 
     context 'existing branch' do
@@ -29,6 +41,12 @@ describe GitPushService, services: true do
       end
 
       it { is_expected.to be_truthy }
+
+      it 'flushes general cached data' do
+        expect(project.repository).to receive(:expire_cache)
+
+        subject
+      end
     end
 
     context 'rm branch' do
@@ -37,6 +55,18 @@ describe GitPushService, services: true do
       end
 
       it { is_expected.to be_truthy }
+
+      it 'flushes the visible content cache' do
+        expect(project.repository).to receive(:expire_has_visible_content_cache)
+
+        subject
+      end
+
+      it 'flushes general cached data' do
+        expect(project.repository).to receive(:expire_cache)
+
+        subject
+      end
     end
   end