Improve Sync To User performance (batch 1) #1896
Conversation
There are ~4k facilities in production, fetching the block_region itself takes 2 queries + 1 query where we fetch the facility for the page. This means a grand total of potentially 8000 region-related queries get fire just for inserting a block_region_id. This preloads the block_regions for all facilities that need to be shipped and that brings us back to 4 queries for a pagesize of 1000.
kitallis
force-pushed
the
sync-perf-1
branch
from
e424312 to
0b18ad0
Compare
| .includes(:facility_group) | ||
| .where.not(facility_group: nil) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Memoize this call here so that we don't end up making thousands of calls to check user for each facility | ||
| def block_level_sync? | ||
| @is_block_level_sync_enabled = current_user&.block_level_sync? if @is_block_level_sync_enabled.nil? |
There was a problem hiding this comment.
Can you change this to match the more idiomatic way of doing this type of nil-safe memoization?
def block_level_sync_enabled?
return @block_level_sync_enabled if defined? @block_level_sync_enabled
@block_level_sync_enabled = current_user&.block_level_sync?
endThere was a problem hiding this comment.
Ah, wasn't aware of this, this seems a lot better, thanks!
app/models/facility.rb
Outdated
| @@ -49,6 +49,13 @@ class Facility < ApplicationRecord | |||
| foreign_key: "assigned_facility_id" | |||
|
|
|||
| pg_search_scope :search_by_name, against: {name: "A", slug: "B"}, using: {tsearch: {prefix: true, any_word: true}} | |||
| scope :with_block_region_id, -> { | |||
| return all unless Flipper.enabled?(:regions_prep) | |||
There was a problem hiding this comment.
I think the feature flag check should go up into the sync code that is building up the query...having it done in the scope itself is hard to understand. It isn't clear why return all is valid if regions_prep is turned off...I think it would make more sense reading in context of the records_to_sync method.
There was a problem hiding this comment.
It looks like we can't simply add another includes(:region) because we have to get the block region for each facility, not just its corresponding facility region.
Just ideating for a second, is there room to introduce a belongs_to :block_region on the facility model?
In any case, getting the block region is the awkward part. I suspect setting that up as an association would be even more awkward.
Looks good to me!
|
@harimohanraj89 I think it'll be equally awkward to setup the association, yep. We can tweak this around if we need something like this more and more. For now this is the only instance where preloading is necessary. |
Story card: ch2086
Because
Whilst perf-testing block-syncs, we discovered various inefficiencies in our sync (GET) code-paths that get exacerbated when many users re-sync at the same time.
A full list of improvements and the rationale is here.
This addresses
This is a batch of fixes pulled from #1189 that should help in improving Facility sync performance.