From 739d6a5ad3ac3756d89d6d07fec5fb876aa333d6 Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Tue, 27 Aug 2019 15:29:53 +0200 Subject: Perform two-step Routable lookup by path In order to lookup a Project or Namespace by path, we prefer an exact match (case-sensitive) but in absence of that, we'd also take a case-insensitive match. The case-insensitive matching with preference for the exact match is a bit more involved in SQL as the exact lookup. Yet, the majority of cases will be an exact match. The thinking here is that we can optimize the lookup by performing an exact match first and only if there is no result, we perform the case-insensitive lookup. Data for GitLab.com: * We have about 15M records in routes table * About 2,500 routes exist where there's more than one record with the same `lower(path)` It is possible for a user to craft requests that would always trigger the 2-step search (e.g. we have a route for `/foo/bar`, the request is always for `/FOO/bar`). In this case, the change at hand is not beneficial as it would run an additional query. However, based on the data, it is highly likely that the vast majority of requests can be satisfied with an exact match only. The context for this change is https://gitlab.com/gitlab-org/gitlab-ce/issues/64590#note_208156463. --- app/models/concerns/routable.rb | 11 +++++++++-- spec/models/concerns/routable_spec.rb | 20 +++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 116e8967651..115c3ce2f91 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -33,8 +33,15 @@ module Routable # # Returns a single object, or nil. def find_by_full_path(path, follow_redirects: false) - order_sql = Arel.sql("(CASE WHEN routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)") - found = where_full_path_in([path]).reorder(order_sql).take + if Feature.enabled?(:routable_two_step_lookup) + # Case sensitive match first (it's cheaper and the usual case) + # If we didn't have an exact match, we perform a case insensitive search + found = joins(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take + else + order_sql = Arel.sql("(CASE WHEN routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)") + found = where_full_path_in([path]).reorder(order_sql).take + end + return found if found if follow_redirects diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index 31163a5bb5c..cff86afe768 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -58,7 +58,7 @@ describe Group, 'Routable' do end end - describe '.find_by_full_path' do + shared_examples_for '.find_by_full_path' do let!(:nested_group) { create(:group, parent: group) } context 'without any redirect routes' do @@ -110,6 +110,24 @@ describe Group, 'Routable' do end end + describe '.find_by_full_path' do + context 'with routable_two_step_lookup feature' do + before do + stub_feature_flags(routable_two_step_lookup: true) + end + + it_behaves_like '.find_by_full_path' + end + + context 'without routable_two_step_lookup feature' do + before do + stub_feature_flags(routable_two_step_lookup: false) + end + + it_behaves_like '.find_by_full_path' + end + end + describe '.where_full_path_in' do context 'without any paths' do it 'returns an empty relation' do -- cgit v1.2.1 From 1003cd92cbaf9df12bf01b85c16c8dabdb1a8b72 Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Thu, 29 Aug 2019 13:51:00 +0200 Subject: Add method call count instrumentation --- app/models/concerns/routable.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 115c3ce2f91..01b853d413c 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -59,12 +59,23 @@ module Routable def where_full_path_in(paths) return none if paths.empty? + increment_full_path_in_counter + wheres = paths.map do |path| "(LOWER(routes.path) = LOWER(#{connection.quote(path)}))" end joins(:route).where(wheres.join(' OR ')) end + + # Temporary instrumentation of method calls for .where_full_path_in + def increment_full_path_in_counter + @counter ||= Gitlab::Metrics.counter(:routable_caseinsensitive_lookup_calls, 'Number of calls to Routable.where_full_path_in') + + @counter.increment + rescue + # ignore the error + end end def full_name -- cgit v1.2.1 From edde1b708a6a605185a6dde73550b9f45e673371 Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Fri, 30 Aug 2019 11:50:26 +0200 Subject: Add another counter to calculate method call ratio We should see the ratio drop down when enabling the Feature. Recommendation by @andrewn --- app/models/concerns/routable.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 01b853d413c..3a486632800 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -33,6 +33,8 @@ module Routable # # Returns a single object, or nil. def find_by_full_path(path, follow_redirects: false) + increment_counter(:routable_find_by_full_path, 'Number of calls to Routable.find_by_full_path') + if Feature.enabled?(:routable_two_step_lookup) # Case sensitive match first (it's cheaper and the usual case) # If we didn't have an exact match, we perform a case insensitive search @@ -59,7 +61,7 @@ module Routable def where_full_path_in(paths) return none if paths.empty? - increment_full_path_in_counter + increment_counter(:routable_where_full_path_in, 'Number of calls to Routable.where_full_path_in') wheres = paths.map do |path| "(LOWER(routes.path) = LOWER(#{connection.quote(path)}))" @@ -68,11 +70,11 @@ module Routable joins(:route).where(wheres.join(' OR ')) end - # Temporary instrumentation of method calls for .where_full_path_in - def increment_full_path_in_counter - @counter ||= Gitlab::Metrics.counter(:routable_caseinsensitive_lookup_calls, 'Number of calls to Routable.where_full_path_in') + # Temporary instrumentation of method calls + def increment_counter(counter, description) + @counters[counter] ||= Gitlab::Metrics.counter(counter, description) - @counter.increment + @counters[counter].increment rescue # ignore the error end -- cgit v1.2.1