From a583f137ddc9833796752b9cf8c07077d76078ba Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 1 Oct 2018 13:29:47 +0100 Subject: [PATCH] Removes N+1 gitaly rpc call to fetch the last commit for path Implements list_last_commits_for_tree to communicate with the ListLastCommitsForTree Gitaly RPC Bumps the Gitaly server version Bumps the Gitaly-Proto gem version --- GITALY_SERVER_VERSION | 2 +- Gemfile | 2 +- Gemfile.lock | 4 +- Gemfile.rails5.lock | 4 +- app/models/repository.rb | 8 +++ ...solve-n-1-in-refs-controller-logs-tree.yml | 6 +++ lib/gitlab/git/repository.rb | 6 +++ lib/gitlab/gitaly_client/commit_service.rb | 18 +++++++ lib/gitlab/tree_summary.rb | 28 +++++----- spec/models/repository_spec.rb | 51 +++++++++++++++++++ 10 files changed, 111 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index ee1e4d2aee5..3ba7bd5ba83 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.122.0 +0.123.0 diff --git a/Gemfile b/Gemfile index 35e83a530f0..dcb97d1e7ad 100644 --- a/Gemfile +++ b/Gemfile @@ -423,7 +423,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.117.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.118.1', require: 'gitaly' gem 'grpc', '~> 1.11.0' # Locked until https://github.com/google/protobuf/issues/4210 is closed diff --git a/Gemfile.lock b/Gemfile.lock index d8eaaac99b1..8e8abb05a67 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -276,7 +276,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.117.0) + gitaly-proto (0.118.1) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1031,7 +1031,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.117.0) + gitaly-proto (~> 0.118.1) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index ab35a4a399f..a7728af0ebe 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -279,7 +279,7 @@ GEM gettext_i18n_rails (>= 0.7.1) po_to_json (>= 1.0.0) rails (>= 3.2.0) - gitaly-proto (0.117.0) + gitaly-proto (0.118.1) google-protobuf (~> 3.1) grpc (~> 1.10) github-linguist (5.3.3) @@ -1040,7 +1040,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.3) - gitaly-proto (~> 0.117.0) + gitaly-proto (~> 0.118.1) github-linguist (~> 5.3.3) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-gollum-lib (~> 4.2) diff --git a/app/models/repository.rb b/app/models/repository.rb index 4fecdb3c1ad..a3a3ce179fc 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -668,6 +668,14 @@ class Repository end end + def list_last_commits_for_tree(sha, path, offset: 0, limit: 25) + commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit) + + commits.each do |path, commit| + commits[path] = ::Commit.new(commit, @project) + end + end + def last_commit_for_path(sha, path) commit = raw_repository.last_commit_for_path(sha, path) ::Commit.new(commit, @project) if commit diff --git a/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml b/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml new file mode 100644 index 00000000000..04662a7cfe2 --- /dev/null +++ b/changelogs/unreleased/37433-solve-n-1-in-refs-controller-logs-tree.yml @@ -0,0 +1,6 @@ +--- +title: Adds support for Gitaly ListLastCommitsForTree RPC in order to make bulk-fetch + of commits more performant +merge_request: 21921 +author: +type: performance diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 3d5a63bdbac..45d42c7078f 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -953,6 +953,12 @@ module Gitlab end end + def list_last_commits_for_tree(sha, path, offset: 0, limit: 25) + wrapped_gitaly_errors do + gitaly_commit_client.list_last_commits_for_tree(sha, path, offset: offset, limit: limit) + end + end + def last_commit_for_path(sha, path) wrapped_gitaly_errors do gitaly_commit_client.last_commit_for_path(sha, path) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 07e5e204b68..085b2a127a5 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -148,6 +148,24 @@ module Gitlab GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient.medium_timeout).count end + def list_last_commits_for_tree(revision, path, offset: 0, limit: 25) + request = Gitaly::ListLastCommitsForTreeRequest.new( + repository: @gitaly_repo, + revision: encode_binary(revision), + path: encode_binary(path.to_s), + offset: offset, + limit: limit + ) + + response = GitalyClient.call(@repository.storage, :commit_service, :list_last_commits_for_tree, request, timeout: GitalyClient.medium_timeout) + + response.each_with_object({}) do |gitaly_response, hsh| + gitaly_response.commits.each do |commit_for_tree| + hsh[commit_for_tree.path] = Gitlab::Git::Commit.new(@repository, commit_for_tree.commit) + end + end + end + def last_commit_for_path(revision, path) request = Gitaly::LastCommitForPathRequest.new( repository: @gitaly_repo, diff --git a/lib/gitlab/tree_summary.rb b/lib/gitlab/tree_summary.rb index b05d408b1c0..c2955cd374c 100644 --- a/lib/gitlab/tree_summary.rb +++ b/lib/gitlab/tree_summary.rb @@ -73,25 +73,29 @@ module Gitlab end def fill_last_commits!(entries) - # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433 - Gitlab::GitalyClient.allow_n_plus_1_calls do - entries.each do |entry| - raw_commit = repository.last_commit_for_path(commit.id, entry_path(entry)) + # Ensure the path is in "path/" format + ensured_path = + if path + File.join(*[path, ""]) + end - if raw_commit - commit = resolve_commit(raw_commit) + commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit) - entry[:commit] = commit - entry[:commit_path] = commit_path(commit) - end + entries.each do |entry| + path_key = entry_path(entry) + commit = cache_commit(commits_hsh[path_key]) + + if commit + entry[:commit] = commit + entry[:commit_path] = commit_path(commit) end end end - def resolve_commit(raw_commit) - return nil unless raw_commit.present? + def cache_commit(commit) + return nil unless commit.present? - resolved_commits[raw_commit.id] ||= ::Commit.new(raw_commit, project) + resolved_commits[commit.id] ||= commit end def commit_path(commit) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 7e1b7c35517..784d17e271e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -188,6 +188,57 @@ describe Repository do end end + describe '#list_last_commits_for_tree' do + let(:path_to_commit) do + { + "encoding" => "913c66a37b4a45b9769037c55c2d238bd0942d2e", + "files" => "570e7b2abdd848b95f2f578043fc23bd6f6fd24d", + ".gitignore" => "c1acaa58bbcbc3eafe538cb8274ba387047b69f8", + ".gitmodules" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", + "CHANGELOG" => "913c66a37b4a45b9769037c55c2d238bd0942d2e", + "CONTRIBUTING.md" => "6d394385cf567f80a8fd85055db1ab4c5295806f", + "Gemfile.zip" => "ae73cb07c9eeaf35924a10f713b364d32b2dd34f", + "LICENSE" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", + "MAINTENANCE.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e", + "PROCESS.md" => "913c66a37b4a45b9769037c55c2d238bd0942d2e", + "README.md" => "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863", + "VERSION" => "913c66a37b4a45b9769037c55c2d238bd0942d2e", + "gitlab-shell" => "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", + "six" => "cfe32cf61b73a0d5e9f13e774abde7ff789b1660" + } + end + + subject { repository.list_last_commits_for_tree(sample_commit.id, '.').id } + + it 'returns the last commits for every entry in the current path' do + result = repository.list_last_commits_for_tree(sample_commit.id, '.') + + result.each do |key, value| + result[key] = value.id + end + + expect(result).to include(path_to_commit) + end + + it 'returns the last commits for every entry in the current path starting from the offset' do + result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size - 1) + + expect(result.size).to eq(1) + end + + it 'returns a limited number of last commits for every entry in the current path starting from the offset' do + result = repository.list_last_commits_for_tree(sample_commit.id, '.', limit: 1) + + expect(result.size).to eq(1) + end + + it 'returns an empty hash when offset is out of bounds' do + result = repository.list_last_commits_for_tree(sample_commit.id, '.', offset: path_to_commit.size) + + expect(result.size).to eq(0) + end + end + describe '#last_commit_for_path' do shared_examples 'getting last commit for path' do subject { repository.last_commit_for_path(sample_commit.id, '.gitignore').id }