diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index cf3ce3c9e54..ca65e81f27a 100644 --- a/app/models/ci/pipeline.rb +++ b/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? diff --git a/app/models/project.rb b/app/models/project.rb index 4689b588906..7185b4d44fc 100644 --- a/app/models/project.rb +++ b/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? diff --git a/app/models/repository.rb b/app/models/repository.rb index 7497b69f674..54388d1c8b4 100644 --- a/app/models/repository.rb +++ b/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? + return ref if ref.is_a?(::Commit) - commit = - if ref.is_a?(Gitlab::Git::Commit) - ref - else - Gitlab::Git::Commit.find(raw_repository, ref) - end + find_commit(ref) + end - commit = ::Commit.new(commit, @project) if commit - commit - rescue Rugged::OdbError, Rugged::TreeError - nil + # 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) @@ -1064,6 +1064,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 @@ -1102,12 +1114,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) diff --git a/changelogs/unreleased/zj-commit-cache.yml b/changelogs/unreleased/zj-commit-cache.yml new file mode 100644 index 00000000000..e3afe0ea7ef --- /dev/null +++ b/changelogs/unreleased/zj-commit-cache.yml @@ -0,0 +1,5 @@ +--- +title: Cache commits fetched from the repository +merge_request: +author: +type: performance diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index 1957c254c28..23ae37ff71e 100644 --- a/lib/gitlab/git/commit.rb +++ b/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 diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index f1ad5dd84ff..1604a2da485 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 73e038b61ed..2b8aa7cf9c3 100644 --- a/spec/models/merge_request_spec.rb +++ b/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 diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 874368ada67..455d5e8a656 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2260,4 +2260,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