Commit a0edaa92 authored by Stan Hu's avatar Stan Hu

Cache Routable#full_path in RequestStore to reduce duplicate route loads

We see in #27387 that a call to `polymorphic_path` will cause duplicate
SELECT route calls for each merge request in a milestone. This happens
because calling `project.namespace.becomes(Namespace)` will instantiate
a new instance of a Namespace for each merge request, which causes a N+1
query on the routes table. This change caches the state of the route by
the specific class and ID, which dramatically eliminates duplicate work.
parent 1005389f
...@@ -163,7 +163,20 @@ module Routable ...@@ -163,7 +163,20 @@ module Routable
end end
end end
# Every time `project.namespace.becomes(Namespace)` is called for polymorphic_path,
# a new instance is instantiated, and we end up duplicating the same query to retrieve
# the route. Caching this per request ensures that even if we have multiple instances,
# we will not have to duplicate work, avoiding N+1 queries in some cases.
def full_path def full_path
return uncached_full_path unless RequestStore.active?
key = "routable/full_path/#{self.class.name}/#{self.id}"
RequestStore[key] ||= uncached_full_path
end
private
def uncached_full_path
if route && route.path.present? if route && route.path.present?
@full_path ||= route.path @full_path ||= route.path
else else
...@@ -173,8 +186,6 @@ module Routable ...@@ -173,8 +186,6 @@ module Routable
end end
end end
private
def full_name_changed? def full_name_changed?
name_changed? || parent_changed? name_changed? || parent_changed?
end end
......
---
title: Cache Routable#full_path in RequestStore to reduce duplicate route loads
merge_request:
author:
...@@ -194,6 +194,24 @@ describe Group, 'Routable' do ...@@ -194,6 +194,24 @@ describe Group, 'Routable' do
it { expect(group.full_path).to eq(group.path) } it { expect(group.full_path).to eq(group.path) }
it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") } it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") }
context 'with RequestStore active' do
before do
RequestStore.begin!
end
after do
RequestStore.end!
RequestStore.clear!
end
it 'does not load the route table more than once' do
expect(group).to receive(:uncached_full_path).once.and_call_original
3.times { group.full_path }
expect(group.full_path).to eq(group.path)
end
end
end end
describe '#full_name' do describe '#full_name' do
......
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