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
This commit is contained in:
Robert Speicher 2016-03-21 14:42:25 +00:00
commit ffc3acd498
13 changed files with 140 additions and 23 deletions

View file

@ -108,9 +108,9 @@ class Issue < ActiveRecord::Base
end
def related_branches
return [] if self.project.empty_repo?
self.project.repository.branch_names.select { |branch| branch.end_with?("-#{iid}") }
project.repository.branch_names.select do |branch|
branch.end_with?("-#{iid}")
end
end
# Reset issue events cache

View file

@ -876,6 +876,7 @@ class Project < ActiveRecord::Base
# Forked import is handled asynchronously
unless forked?
if gitlab_shell.add_repository(path_with_namespace)
repository.after_create
true
else
errors.add(:base, 'Failed to create repository via gitlab-shell')

View file

@ -123,23 +123,27 @@ class ProjectWiki
end
def repository
Repository.new(path_with_namespace, @project)
@repository ||= Repository.new(path_with_namespace, @project)
end
def default_branch
wiki.class.default_ref
end
private
def create_repo!
if init_repo(path_with_namespace)
Gollum::Wiki.new(path_to_repo)
wiki = Gollum::Wiki.new(path_to_repo)
else
raise CouldNotCreateWikiError
end
repository.after_create
wiki
end
private
def init_repo(path_with_namespace)
gitlab_shell.add_repository(path_with_namespace)
end

View file

@ -42,12 +42,15 @@ class Repository
end
def exists?
return false unless raw_repository
return @exists unless @exists.nil?
raw_repository.rugged
true
rescue Gitlab::Git::Repository::NoRepository
false
@exists = cache.fetch(:exists?) do
begin
raw_repository && raw_repository.rugged ? true : false
rescue Gitlab::Git::Repository::NoRepository
false
end
end
end
def empty?
@ -320,12 +323,23 @@ class Repository
@avatar = nil
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.
def before_delete
expire_cache if exists?
expire_root_ref_cache
expire_emptiness_caches
expire_exists_cache
end
# Runs code just before the HEAD of a repository is changed.
@ -351,6 +365,7 @@ class Repository
# Runs code after a repository has been forked/imported.
def after_import
expire_emptiness_caches
expire_exists_cache
end
# Runs code after a new commit has been pushed.

View file

@ -20,14 +20,15 @@ class RepositoryForkWorker
return
end
project.repository.after_import
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.import_fail
return
end
project.repository.after_import
project.import_finish
end
end

View file

@ -160,6 +160,7 @@ Feature: Project Issues
Scenario: Issues on empty project
Given empty project "Empty Project"
And I have an ssh key
When I visit empty project page
And I see empty project details with ssh clone info
When I visit empty project's issues page

View file

@ -5,6 +5,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
include SharedNote
include SharedPaths
include SharedMarkdown
include SharedUser
step 'I should see "Release 0.4" in issues' do
expect(page).to have_content "Release 0.4"
@ -240,7 +241,7 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
end
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
When 'I visit empty project page' do

View file

@ -2,7 +2,7 @@ require('spec_helper')
describe Projects::IssuesController do
describe "GET #index" do
let(:project) { create(:project) }
let(:project) { create(:project_empty_repo) }
let(:user) { create(:user) }
let(:issue) { create(:issue, project: project) }
@ -41,7 +41,7 @@ describe Projects::IssuesController do
end
describe 'Confidential Issues' do
let(:project) { create(:empty_project, :public) }
let(:project) { create(:project_empty_repo, :public) }
let(:assignee) { create(:assignee) }
let(:author) { create(:user) }
let(:non_member) { create(:user) }

View file

@ -183,9 +183,9 @@ describe Issue, models: true do
describe '#related_branches' do
it "selects the right branches" do
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

View file

@ -720,4 +720,45 @@ describe Project, models: true do
expect(described_class.search_by_title('KITTENS')).to eq([project])
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

View file

@ -244,6 +244,18 @@ describe ProjectWiki, models: true do
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
def create_temp_repo(path)

View file

@ -537,6 +537,12 @@ describe Repository, models: true do
repository.before_delete
end
it 'flushes the exists cache' do
expect(repository).to receive(:expire_exists_cache)
repository.before_delete
end
end
describe 'when a repository exists' do
@ -593,6 +599,12 @@ describe Repository, models: true do
repository.after_import
end
it 'flushes the exists cache' do
expect(repository).to receive(:expire_exists_cache)
repository.after_import
end
end
describe '#after_push_commit' do
@ -619,6 +631,14 @@ describe Repository, models: true do
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
it 'shows the main language of the project' do
expect(repository.main_language).to eq("Ruby")
@ -781,6 +801,16 @@ describe Repository, models: true do
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
let(:cache) { repository.send(:cache) }

View file

@ -3,12 +3,17 @@ require 'spec_helper'
describe RepositoryForkWorker do
let(:project) { create(:project) }
let(:fork_project) { create(:project, forked_from_project: project) }
let(:shell) { Gitlab::Shell.new }
subject { RepositoryForkWorker.new }
before do
allow(subject).to receive(:gitlab_shell).and_return(shell)
end
describe "#perform" 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,
fork_project.namespace.path
).and_return(true)
@ -19,20 +24,26 @@ describe RepositoryForkWorker do
fork_project.namespace.path)
end
it 'flushes the empty caches' do
expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).
it 'flushes various caches' do
expect(shell).to receive(:fork_repository).
with(project.path_with_namespace, fork_project.namespace.path).
and_return(true)
expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).
and_call_original
expect_any_instance_of(Repository).to receive(:expire_exists_cache).
and_call_original
subject.perform(project.id, project.path_with_namespace,
fork_project.namespace.path)
end
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(
project.id,
project.path_with_namespace,