Cache commits on the repository model

Now, when requesting a commit from the Repository model, the results are
not cached. This means we're fetching the same commit by oid multiple times
during the same request. To prevent us from doing this, we now cache
results. Caching is done only based on object id (aka SHA).

Given we cache on the Repository model, results are scoped to the
associated project, eventhough the change of two repositories having the
same oids for different commits is small.

app/models/ci/pipeline.rb

@@ -249,9 +249,7 @@ module Ci
     end
     def commit
-      @commit ||= project.commit(sha)
-    rescue
-      nil
+      @commit ||= project.commit_by(oid: sha)
     end
     def branch?

app/models/project.rb

@@ -540,6 +540,10 @@ class Project < ActiveRecord::Base
     repository.commit(ref)
   end
+  def commit_by(oid:)
+    repository.commit_by(oid: oid)
+  end
+
   # ref can't be HEAD, can only be branch/tag name or SHA
   def latest_successful_builds_for(ref = default_branch)
     latest_pipeline = pipelines.latest_successful_for(ref)
@@ -553,7 +557,7 @@ class Project < ActiveRecord::Base
   def merge_base_commit(first_commit_id, second_commit_id)
     sha = repository.merge_base(first_commit_id, second_commit_id)
-    repository.commit(sha) if sha
+    commit_by(oid: sha) if sha
   end
   def saved?

app/models/repository.rb

@@ -76,6 +76,7 @@ class Repository
     @full_path = full_path
     @disk_path = disk_path || full_path
     @project = project
+    @commit_cache = {}
   end
   def ==(other)
@@ -103,18 +104,17 @@ class Repository
   def commit(ref = 'HEAD')
     return nil unless exists?
-    commit =
-      if ref.is_a?(Gitlab::Git::Commit)
-        ref
-      else
-        Gitlab::Git::Commit.find(raw_repository, ref)
-      end
-    commit = ::Commit.new(commit, @project) if commit
-    commit
-  rescue Rugged::OdbError, Rugged::TreeError
-    nil
+    return ref if ref.is_a?(::Commit)
+    find_commit(ref)
+  end
+  # Finding a commit by the passed SHA
+  # Also takes care of caching, based on the SHA
+  def commit_by(oid:)
+    return @commit_cache[oid] if @commit_cache.key?(oid)
+
+    @commit_cache[oid] = find_commit(oid)
   end
   def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil)
@@ -231,7 +231,7 @@ class Repository
   # branches or tags, but we want to keep some of these commits around, for
   # example if they have comments or CI builds.
   def keep_around(sha)
-    return unless sha && commit(sha)
+    return unless sha && commit_by(oid: sha)
     return if kept_around?(sha)
@@ -1069,6 +1069,18 @@ class Repository
   private
+  # TODO Generice finder, later split this on finders by Ref or Oid
+  # gitlab-org/gitlab-ce#39239
+  def find_commit(oid_or_ref)
+    commit = if oid_or_ref.is_a?(Gitlab::Git::Commit)
+               oid_or_ref
+             else
+               Gitlab::Git::Commit.find(raw_repository, oid_or_ref)
+             end
+
+    ::Commit.new(commit, @project) if commit
+  end
+
   def blob_data_at(sha, path)
     blob = blob_at(sha, path)
     return unless blob
@@ -1107,12 +1119,12 @@ class Repository
   def last_commit_for_path_by_gitaly(sha, path)
     c = raw_repository.gitaly_commit_client.last_commit_for_path(sha, path)
-    commit(c)
+    commit_by(oid: c)
   end
   def last_commit_for_path_by_rugged(sha, path)
     sha = last_commit_id_for_path_by_shelling_out(sha, path)
-    commit(sha)
+    commit_by(oid: sha)
   end
   def last_commit_id_for_path_by_shelling_out(sha, path)

changelogs/unreleased/zj-commit-cache.yml

+---
+title: Cache commits fetched from the repository
+merge_request:
+author:
+type: performance

lib/gitlab/git/commit.rb

@@ -72,7 +72,8 @@ module Gitlab
           decorate(repo, commit) if commit
         rescue Rugged::ReferenceError, Rugged::InvalidError, Rugged::ObjectError,
-               Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository
+               Gitlab::Git::CommandError, Gitlab::Git::Repository::NoRepository,
+               Rugged::OdbError, Rugged::TreeError
           nil
         end

spec/controllers/projects/pipelines_controller_spec.rb

@@ -46,7 +46,7 @@ describe Projects::PipelinesController do
     context 'when performing gitaly calls', :request_store do
       it 'limits the Gitaly requests' do
-        expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10)
+        expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(8)
       end
     end
   end

spec/models/merge_request_spec.rb

@@ -86,7 +86,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
-        project.repository.raw_repository.delete_branch(subject.target_branch)
+        project.repository.rm_branch(subject.author, subject.target_branch)
       end
       it 'returns nil' do
@@ -1388,7 +1388,7 @@ describe MergeRequest do
     context 'when the target branch does not exist' do
       before do
-        subject.project.repository.raw_repository.delete_branch(subject.target_branch)
+        subject.project.repository.rm_branch(subject.author, subject.target_branch)
       end
       it 'returns nil' do

spec/models/repository_spec.rb

@@ -2238,4 +2238,24 @@ describe Repository do
       end
     end
   end
+
+  describe 'commit cache' do
+    set(:project) { create(:project, :repository) }
+
+    it 'caches based on SHA' do
+      # Gets the commit oid, and warms the cache
+      oid = project.commit.id
+
+      expect(Gitlab::Git::Commit).not_to receive(:find).once
+
+      project.commit_by(oid: oid)
+    end
+
+    it 'caches nil values' do
+      expect(Gitlab::Git::Commit).to receive(:find).once
+
+      project.commit_by(oid: '1' * 40)
+      project.commit_by(oid: '1' * 40)
+    end
+  end
 end