From d171ff60168cd55b6d7b9ee920269f44a26e577e Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 25 Oct 2018 17:35:31 +0200 Subject: Rewrite SnippetsFinder to improve performance This completely rewrites the SnippetsFinder class from the ground up in order to improve its performance. The old code was beyond salvaging. It was complex, included various Rails 5 workarounds, comments that shouldn't be necessary, and most important of all: it produced a really poorly performing database query. As a result, I opted for rewriting the finder from scratch, instead of trying to patch the existing code. Instead of trying to reuse as many existing methods as possible, I opted for defining new methods specifically meant for the SnippetsFinder. This requires some extra code here and there, but allows us to have much more control over the resulting SQL queries. It is these changes that then allow us to produce a _much_ more efficient query. To illustrate how bad the old query was, we will use my own snippets as an example. Currently I have 52 snippets, most of which are global ones. To retrieve these, you would run the following Ruby code: user = User.find_by(username: 'yorickpeterse') SnippetsFinder.new(user, author: user).execute On GitLab.com the resulting query will take between 10 and 15 seconds to run, producing the query plan found at https://explain.depesz.com/s/Y5IX. Apart from the long execution time, the total number of buffers (the sum of all shared hits) is around 185 GB, though the real number is probably (hopefully) much lower as I doubt simply summing these numbers produces the true total number of buffers used. The new query's plan can be found at https://explain.depesz.com/s/wHdN, and this query takes between 10 and 100-ish milliseconds to run. The total number of buffers used is only about 30 MB. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/52639 --- spec/finders/snippets_finder_spec.rb | 45 +++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 16 deletions(-) (limited to 'spec/finders/snippets_finder_spec.rb') diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index 1ae0bd988f2..dfeeb3040c6 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -4,16 +4,13 @@ describe SnippetsFinder do include Gitlab::Allowable using RSpec::Parameterized::TableSyntax - context 'filter by visibility' do - let!(:snippet1) { create(:personal_snippet, :private) } - let!(:snippet2) { create(:personal_snippet, :internal) } - let!(:snippet3) { create(:personal_snippet, :public) } + describe '#initialize' do + it 'raises ArgumentError when a project and author are given' do + user = build(:user) + project = build(:project) - it "returns public snippets when visibility is PUBLIC" do - snippets = described_class.new(nil, visibility: Snippet::PUBLIC).execute - - expect(snippets).to include(snippet3) - expect(snippets).not_to include(snippet1, snippet2) + expect { described_class.new(user, author: user, project: project) } + .to raise_error(ArgumentError) end end @@ -66,21 +63,21 @@ describe SnippetsFinder do end it "returns internal snippets" do - snippets = described_class.new(user, author: user, visibility: Snippet::INTERNAL).execute + snippets = described_class.new(user, author: user, scope: :are_internal).execute expect(snippets).to include(snippet2) expect(snippets).not_to include(snippet1, snippet3) end it "returns private snippets" do - snippets = described_class.new(user, author: user, visibility: Snippet::PRIVATE).execute + snippets = described_class.new(user, author: user, scope: :are_private).execute expect(snippets).to include(snippet1) expect(snippets).not_to include(snippet2, snippet3) end it "returns public snippets" do - snippets = described_class.new(user, author: user, visibility: Snippet::PUBLIC).execute + snippets = described_class.new(user, author: user, scope: :are_public).execute expect(snippets).to include(snippet3) expect(snippets).not_to include(snippet1, snippet2) @@ -98,6 +95,13 @@ describe SnippetsFinder do expect(snippets).to include(snippet3) expect(snippets).not_to include(snippet2, snippet1) end + + it 'returns all snippets for an admin' do + admin = create(:user, :admin) + snippets = described_class.new(admin, author: user).execute + + expect(snippets).to include(snippet1, snippet2, snippet3) + end end context 'filter by project' do @@ -126,21 +130,21 @@ describe SnippetsFinder do end it "returns public snippets for non project members" do - snippets = described_class.new(user, project: project1, visibility: Snippet::PUBLIC).execute + snippets = described_class.new(user, project: project1, scope: :are_public).execute expect(snippets).to include(@snippet3) expect(snippets).not_to include(@snippet1, @snippet2) end it "returns internal snippets for non project members" do - snippets = described_class.new(user, project: project1, visibility: Snippet::INTERNAL).execute + snippets = described_class.new(user, project: project1, scope: :are_internal).execute expect(snippets).to include(@snippet2) expect(snippets).not_to include(@snippet1, @snippet3) end it "does not return private snippets for non project members" do - snippets = described_class.new(user, project: project1, visibility: Snippet::PRIVATE).execute + snippets = described_class.new(user, project: project1, scope: :are_private).execute expect(snippets).not_to include(@snippet1, @snippet2, @snippet3) end @@ -156,10 +160,17 @@ describe SnippetsFinder do it "returns private snippets for project members" do project1.add_developer(user) - snippets = described_class.new(user, project: project1, visibility: Snippet::PRIVATE).execute + snippets = described_class.new(user, project: project1, scope: :are_private).execute expect(snippets).to include(@snippet1) end + + it 'returns all snippets for an admin' do + admin = create(:user, :admin) + snippets = described_class.new(admin, project: project1).execute + + expect(snippets).to include(@snippet1, @snippet2, @snippet3) + end end describe '#execute' do @@ -184,4 +195,6 @@ describe SnippetsFinder do end end end + + it_behaves_like 'snippet visibility' end -- cgit v1.2.1