Cinstance N+1 issues clean-up #3889
Conversation
app/concerns/new_application_form.rb
Outdated
| @@ -27,7 +27,7 @@ def buyers | |||
| end | |||
|
|
|||
| def products | |||
| paginated_products.map { |p| ServicePresenter.new(p).new_application_data.as_json } | |||
| paginated_products.includes(:default_application_plan).map { |p| ServicePresenter.new(p).new_application_data.as_json } | |||
There was a problem hiding this comment.
How do you identify the N+1 scenarios? From Bullet logs?
There was a problem hiding this comment.
Bullet errors are enabled in testing but many are whitelisted to avoid breaking test suite. See the environment file.
| @@ -110,6 +115,12 @@ def buyer_account | |||
| @buyer_account ||= buyer_accounts.find(params[:id]) | |||
| end | |||
|
|
|||
| def to_present(accounts) | |||
| # ActiveRecord::Associations::Preloader.new(records: Array(accounts), associations: [:annotations, {bought_plans: %i[original]}]).call # Rails 7.x | |||
| ActiveRecord::Associations::Preloader.new.preload(Array(accounts), [:annotations, {bought_plans: %i[original]}]) | |||
There was a problem hiding this comment.
Not sure how this Preloader class works. Is the data kept in the memory forever? or only for this controller instance life?
There was a problem hiding this comment.
the associated objects are preloaded into the array of objects. Similar to how #includes on associations works. So you can say it is in memory until normal garbage collection takes place.
| @@ -110,6 +115,12 @@ def buyer_account | |||
| @buyer_account ||= buyer_accounts.find(params[:id]) | |||
| end | |||
|
|
|||
| def to_present(accounts) | |||
There was a problem hiding this comment.
Maybe the method should be called preload!? Calling it to_present makes me think of a conversion that doesn't modify the original given object.
There was a problem hiding this comment.
this has changed in the merged PR.
There was a problem hiding this comment.
same one as in the other comments #3845
| @@ -7,7 +7,7 @@ class Admin::Api::ApplicationsController < Admin::Api::BaseController | |||
| # GET /admin/api/applications.xml | |||
| def index | |||
| apps = applications.scope_search(search) | |||
| .serialization_preloading.paginate(:page => current_page, :per_page => per_page) | |||
| .serialization_preloading(request.format).paginate(:page => current_page, :per_page => per_page) | |||
There was a problem hiding this comment.
different preloading rules for different requested format (xml vs json). So a parameter had to be added. But this is already upstream, need to rebase.
| @@ -18,7 +18,7 @@ def policy_chain | |||
| end | |||
|
|
|||
| def with_subpaths? | |||
| backend_api_configs.with_subpath.any? | |||
| backend_api_configs.any?(&:with_subpath?) | |||
There was a problem hiding this comment.
Is not the original better? It scopes the results. Your modified version always returns all configs and then calls :with_subpath? for each one.
There was a problem hiding this comment.
This was already merged with #3845
If you look from the perspective of taking a service and calling this method on it, what you say makes sense. I fixed a number of N+1 though with the aforementioned PR and I don't remember which test it was related to. Also I looked at a couple of requests and optimized them to a maximum least number of queries.
I think in this case, this change is based on the premises that we have the backend_api_configs already preloaded for the service(s) we deal with. So calling with_subpath? on each is more effective than performing a new database query. Applying a scope results in a new query.
In the other PR I have added active_record_query_trace gem and I assume it showed to me that a query came from this line where I didn't expect a query at this point.
I think even if backend_api_configs is not preloaded, it wouldn't be a huge deal because I don't expect too many backends in services. And it will still be one query, although with more data returned than the original code. It will also load the backend_api_configs into the respective service in case they are further needed.
If you have spotted a particular call that is less efficient this way, we may think about it. But I think it might likely be an edge case when a single service is involved. But with original code I don't see how we can avoid N+1 when many services are loaded at once and we want to preload everything needed for presentation. As the original code will still try to perform a new query for each service.
Hope explanation makes sense. But better comment further on the original (merged) PR because I will be rebasing this one to avoid all the extra commits already in master.
There was a problem hiding this comment.
Nice, so data is preloaded and you save a query. Fine.
app/models/service.rb
Outdated
| sifter :of_account do |account| | ||
| account_id == account.id | ||
| end | ||
|
|
||
| scope :of_account, ->(account) { where.has { sift(:of_account, account) } } |
There was a problem hiding this comment.
Why is sifter better than a regular scope?
There was a problem hiding this comment.
sifters can be applied to other models, scopes only to associations of same model
| Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :plan | ||
| Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :user_account | ||
| Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :user_account | ||
| Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :service |
There was a problem hiding this comment.
I didn't know about this list. Nice to remove items from it
There was a problem hiding this comment.
There was a problem hiding this comment.
As for now, this is the only change in this PR, why did you remove the others? are they already merged or something?
There was a problem hiding this comment.
Yes, merged already. This needs more work, I'm not sure why I kept open, maybe not to forget.
|
This PR is stale because it has not received activity for more than 30 days. Remove stale label or comment or this will be closed in 15 days. |
No description provided.