Commit b8812824 authored by Robert Speicher's avatar Robert Speicher Committed by Rémy Coutable

Merge branch 'issues-show-performance' into 'master'

Improve performance of viewing individual issues

This MR does two things:

1. `Issue#related_branches` no longer performs Git operations that aren't needed
2. The output of `Repository#exists?` is now cached and flushed properly

Combined these two changes should further cut down the amount of Git operations performed when viewing individual issues (and possibly other pages).

See merge request !3296
parent ab3681d1
...@@ -108,9 +108,9 @@ class Issue < ActiveRecord::Base ...@@ -108,9 +108,9 @@ class Issue < ActiveRecord::Base
end end
def related_branches def related_branches
return [] if self.project.empty_repo? project.repository.branch_names.select do |branch|
branch.end_with?("-#{iid}")
self.project.repository.branch_names.select { |branch| branch.end_with?("-#{iid}") } end
end end
# Reset issue events cache # Reset issue events cache
......
...@@ -876,6 +876,7 @@ class Project < ActiveRecord::Base ...@@ -876,6 +876,7 @@ class Project < ActiveRecord::Base
# Forked import is handled asynchronously # Forked import is handled asynchronously
unless forked? unless forked?
if gitlab_shell.add_repository(path_with_namespace) if gitlab_shell.add_repository(path_with_namespace)
repository.after_create
true true
else else
errors.add(:base, 'Failed to create repository via gitlab-shell') errors.add(:base, 'Failed to create repository via gitlab-shell')
......
...@@ -123,23 +123,27 @@ class ProjectWiki ...@@ -123,23 +123,27 @@ class ProjectWiki
end end
def repository def repository
Repository.new(path_with_namespace, @project) @repository ||= Repository.new(path_with_namespace, @project)
end end
def default_branch def default_branch
wiki.class.default_ref wiki.class.default_ref
end end
private
def create_repo! def create_repo!
if init_repo(path_with_namespace) if init_repo(path_with_namespace)
Gollum::Wiki.new(path_to_repo) wiki = Gollum::Wiki.new(path_to_repo)
else else
raise CouldNotCreateWikiError raise CouldNotCreateWikiError
end end
repository.after_create
wiki
end end
private
def init_repo(path_with_namespace) def init_repo(path_with_namespace)
gitlab_shell.add_repository(path_with_namespace) gitlab_shell.add_repository(path_with_namespace)
end end
......
...@@ -42,13 +42,16 @@ class Repository ...@@ -42,13 +42,16 @@ class Repository
end end
def exists? def exists?
return false unless raw_repository return @exists unless @exists.nil?
raw_repository.rugged @exists = cache.fetch(:exists?) do
true begin
raw_repository && raw_repository.rugged ? true : false
rescue Gitlab::Git::Repository::NoRepository rescue Gitlab::Git::Repository::NoRepository
false false
end end
end
end
def empty? def empty?
return @empty unless @empty.nil? return @empty unless @empty.nil?
...@@ -320,12 +323,23 @@ class Repository ...@@ -320,12 +323,23 @@ class Repository
@avatar = nil @avatar = nil
end end
def expire_exists_cache
cache.expire(:exists?)
@exists = nil
end
# Runs code after a repository has been created.
def after_create
expire_exists_cache
end
# Runs code just before a repository is deleted. # Runs code just before a repository is deleted.
def before_delete def before_delete
expire_cache if exists? expire_cache if exists?
expire_root_ref_cache expire_root_ref_cache
expire_emptiness_caches expire_emptiness_caches
expire_exists_cache
end end
# Runs code just before the HEAD of a repository is changed. # Runs code just before the HEAD of a repository is changed.
...@@ -351,6 +365,7 @@ class Repository ...@@ -351,6 +365,7 @@ class Repository
# Runs code after a repository has been forked/imported. # Runs code after a repository has been forked/imported.
def after_import def after_import
expire_emptiness_caches expire_emptiness_caches
expire_exists_cache
end end
# Runs code after a new commit has been pushed. # Runs code after a new commit has been pushed.
......
...@@ -20,14 +20,15 @@ class RepositoryForkWorker ...@@ -20,14 +20,15 @@ class RepositoryForkWorker
return return
end end
project.repository.after_import
unless project.valid_repo? unless project.valid_repo?
logger.error("Project #{id} had an invalid repository after fork") logger.error("Project #{project_id} had an invalid repository after fork")
project.update(import_error: "The forked repository is invalid.") project.update(import_error: "The forked repository is invalid.")
project.import_fail project.import_fail
return return
end end
project.repository.after_import
project.import_finish project.import_finish
end end
end end
...@@ -160,6 +160,7 @@ Feature: Project Issues ...@@ -160,6 +160,7 @@ Feature: Project Issues
Scenario: Issues on empty project Scenario: Issues on empty project
Given empty project "Empty Project" Given empty project "Empty Project"
And I have an ssh key
When I visit empty project page When I visit empty project page
And I see empty project details with ssh clone info And I see empty project details with ssh clone info
When I visit empty project's issues page When I visit empty project's issues page
......
...@@ -5,6 +5,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps ...@@ -5,6 +5,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
include SharedNote include SharedNote
include SharedPaths include SharedPaths
include SharedMarkdown include SharedMarkdown
include SharedUser
step 'I should see "Release 0.4" in issues' do step 'I should see "Release 0.4" in issues' do
expect(page).to have_content "Release 0.4" expect(page).to have_content "Release 0.4"
...@@ -240,7 +241,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps ...@@ -240,7 +241,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
end end
step 'empty project "Empty Project"' do step 'empty project "Empty Project"' do
create :empty_project, name: 'Empty Project', namespace: @user.namespace create :project_empty_repo, name: 'Empty Project', namespace: @user.namespace
end end
When 'I visit empty project page' do When 'I visit empty project page' do
......
...@@ -2,7 +2,7 @@ require('spec_helper') ...@@ -2,7 +2,7 @@ require('spec_helper')
describe Projects::IssuesController do describe Projects::IssuesController do
describe "GET #index" do describe "GET #index" do
let(:project) { create(:project) } let(:project) { create(:project_empty_repo) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
...@@ -41,7 +41,7 @@ describe Projects::IssuesController do ...@@ -41,7 +41,7 @@ describe Projects::IssuesController do
end end
describe 'Confidential Issues' do describe 'Confidential Issues' do
let(:project) { create(:empty_project, :public) } let(:project) { create(:project_empty_repo, :public) }
let(:assignee) { create(:assignee) } let(:assignee) { create(:assignee) }
let(:author) { create(:user) } let(:author) { create(:user) }
let(:non_member) { create(:user) } let(:non_member) { create(:user) }
......
...@@ -185,7 +185,7 @@ describe Issue, models: true do ...@@ -185,7 +185,7 @@ describe Issue, models: true do
allow(subject.project.repository).to receive(:branch_names). allow(subject.project.repository).to receive(:branch_names).
and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name]) and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name])
expect(subject.related_branches).to eq [subject.to_branch_name] expect(subject.related_branches).to eq([subject.to_branch_name])
end end
end end
......
...@@ -720,4 +720,45 @@ describe Project, models: true do ...@@ -720,4 +720,45 @@ describe Project, models: true do
expect(described_class.search_by_title('KITTENS')).to eq([project]) expect(described_class.search_by_title('KITTENS')).to eq([project])
end end
end end
describe '#create_repository' do
let(:project) { create(:project) }
let(:shell) { Gitlab::Shell.new }
before do
allow(project).to receive(:gitlab_shell).and_return(shell)
end
context 'using a regular repository' do
it 'creates the repository' do
expect(shell).to receive(:add_repository).
with(project.path_with_namespace).
and_return(true)
expect(project.repository).to receive(:after_create)
expect(project.create_repository).to eq(true)
end
it 'adds an error if the repository could not be created' do
expect(shell).to receive(:add_repository).
with(project.path_with_namespace).
and_return(false)
expect(project.repository).not_to receive(:after_create)
expect(project.create_repository).to eq(false)
expect(project.errors).not_to be_empty
end
end
context 'using a forked repository' do
it 'does nothing' do
expect(project).to receive(:forked?).and_return(true)
expect(shell).not_to receive(:add_repository)
project.create_repository
end
end
end
end end
...@@ -244,6 +244,18 @@ describe ProjectWiki, models: true do ...@@ -244,6 +244,18 @@ describe ProjectWiki, models: true do
end end
end end
describe '#create_repo!' do
it 'creates a repository' do
expect(subject).to receive(:init_repo).
with(subject.path_with_namespace).
and_return(true)
expect(subject.repository).to receive(:after_create)
expect(subject.create_repo!).to be_an_instance_of(Gollum::Wiki)
end
end
private private
def create_temp_repo(path) def create_temp_repo(path)
......
...@@ -537,6 +537,12 @@ describe Repository, models: true do ...@@ -537,6 +537,12 @@ describe Repository, models: true do
repository.before_delete repository.before_delete
end end
it 'flushes the exists cache' do
expect(repository).to receive(:expire_exists_cache)
repository.before_delete
end
end end
describe 'when a repository exists' do describe 'when a repository exists' do
...@@ -593,6 +599,12 @@ describe Repository, models: true do ...@@ -593,6 +599,12 @@ describe Repository, models: true do
repository.after_import repository.after_import
end end
it 'flushes the exists cache' do
expect(repository).to receive(:expire_exists_cache)
repository.after_import
end
end end
describe '#after_push_commit' do describe '#after_push_commit' do
...@@ -619,6 +631,14 @@ describe Repository, models: true do ...@@ -619,6 +631,14 @@ describe Repository, models: true do
end end
end end
describe '#after_create' do
it 'flushes the exists cache' do
expect(repository).to receive(:expire_exists_cache)
repository.after_create
end
end
describe "#main_language" do describe "#main_language" do
it 'shows the main language of the project' do it 'shows the main language of the project' do
expect(repository.main_language).to eq("Ruby") expect(repository.main_language).to eq("Ruby")
...@@ -781,6 +801,16 @@ describe Repository, models: true do ...@@ -781,6 +801,16 @@ describe Repository, models: true do
end end
end end
describe '#expire_exists_cache' do
let(:cache) { repository.send(:cache) }
it 'expires the cache' do
expect(cache).to receive(:expire).with(:exists?)
repository.expire_exists_cache
end
end
describe '#build_cache' do describe '#build_cache' do
let(:cache) { repository.send(:cache) } let(:cache) { repository.send(:cache) }
......
...@@ -3,12 +3,17 @@ require 'spec_helper' ...@@ -3,12 +3,17 @@ require 'spec_helper'
describe RepositoryForkWorker do describe RepositoryForkWorker do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:fork_project) { create(:project, forked_from_project: project) } let(:fork_project) { create(:project, forked_from_project: project) }
let(:shell) { Gitlab::Shell.new }
subject { RepositoryForkWorker.new } subject { RepositoryForkWorker.new }
before do
allow(subject).to receive(:gitlab_shell).and_return(shell)
end
describe "#perform" do describe "#perform" do
it "creates a new repository from a fork" do it "creates a new repository from a fork" do
expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).with( expect(shell).to receive(:fork_repository).with(
project.path_with_namespace, project.path_with_namespace,
fork_project.namespace.path fork_project.namespace.path
).and_return(true) ).and_return(true)
...@@ -19,20 +24,26 @@ describe RepositoryForkWorker do ...@@ -19,20 +24,26 @@ describe RepositoryForkWorker do
fork_project.namespace.path) fork_project.namespace.path)
end end
it 'flushes the empty caches' do it 'flushes various caches' do
expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository). expect(shell).to receive(:fork_repository).
with(project.path_with_namespace, fork_project.namespace.path). with(project.path_with_namespace, fork_project.namespace.path).
and_return(true) and_return(true)
expect_any_instance_of(Repository).to receive(:expire_emptiness_caches). expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).
and_call_original and_call_original
expect_any_instance_of(Repository).to receive(:expire_exists_cache).
and_call_original
subject.perform(project.id, project.path_with_namespace, subject.perform(project.id, project.path_with_namespace,
fork_project.namespace.path) fork_project.namespace.path)
end end
it "handles bad fork" do it "handles bad fork" do
expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(false) expect(shell).to receive(:fork_repository).and_return(false)
expect(subject.logger).to receive(:error)
subject.perform( subject.perform(
project.id, project.id,
project.path_with_namespace, project.path_with_namespace,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment