AO3-7457 Preload unrevealed collections for work blurbs#5907
AO3-7457 Preload unrevealed collections for work blurbs#5907nateberkopec wants to merge 4 commits into
Conversation
nateberkopec
changed the title
73fd150 to
9d22ce5
Compare
| <% if item.approved_collections.unrevealed.present? %> | ||
| <h5 class="heading"><%= h(ts("Part of ")) + show_collections_data(item.approved_collections.unrevealed) %></h5> | ||
| <% if approved_unrevealed_collections.any? %> | ||
| <h5 class="heading"><%= h(t(".part_of", default: "Part of ")) + show_collections_data(item.approved_unrevealed_collections) %></h5> |
There was a problem hiding this comment.
Change is unrelated but was enforced on me by Rubocop
4755e28 to
0f54639
Compare
0f54639 to
19131f7
Compare
sarken
changed the title
|
Noticed some missing stuff re polymorphism. |
Bilka2
left a comment
Bilka2
left a comment
There was a problem hiding this comment.
Thank you a lot for this, especially the additions of for_blurb all over the place!
I mostly have a bunch of questions to make sure I understand the reasoning behind specific implementations that you went for (and to document them for future!us via comments here on GitHub)
| def exec_queries(&block) | ||
| super(&block).tap { |records| CollectionItem.preload_item_blurbs(records) } | ||
| end | ||
| end | ||
|
|
||
| scope :include_for_works, -> { includes(:item).extending(BlurbPreloads) } |
There was a problem hiding this comment.
This means that exec_queries is only called for this single scope here, right? In that case, I am a bit suspicious about the combination include_for_works and preloading blurbs for bookmarks as well - looking at the usage of this scope I suppose the scope is badly named, since it can be both bookmarks and works. So could we fix the name?
I'm also mildly concerned that we may reintroduce the issue that #5114 fixed, but looks like the regression test is still passing 🤞
|
|
||
| ActiveRecord::Associations::Preloader.new( | ||
| records: work_items, | ||
| associations: [:pseuds, :approved_unrevealed_collections] |
There was a problem hiding this comment.
Why only some of the associations, not all from Work's with_includes_for_blurb, similar to how you do it for bookmarks here?
| collection_item.item if collection_item.item_type == "Bookmark" | ||
| end | ||
|
|
||
| ActiveRecord::Associations::Preloader.new( |
There was a problem hiding this comment.
I suppose my general question is why we need this code that seems to grab into Rails internals instead of being able to use something like includes(work: :pseuds, bookmark: :bookmarkable) - does Rails not support that?
Edit: I remembered that we were told not to set up the work and bookmark associations here the way we are, but my question still stands if we scoped them by type instead
| work_items = records.filter_map do |collection_item| | ||
| collection_item.item if collection_item.item_type == "Work" | ||
| end | ||
| bookmark_items = records.filter_map do |collection_item| | ||
| collection_item.item if collection_item.item_type == "Bookmark" | ||
| end |
There was a problem hiding this comment.
Is there a clean way to avoid looping twice?
| scope :giftworks_for_recipient_name, lambda { |name| select("DISTINCT works.*").joins(:gifts).where("recipient_name = ?", name).where("gifts.rejected = FALSE") } | ||
| scope :giftworks_for_recipient_name, lambda { |name| | ||
| distinct.joins(:gifts).where(gifts: { recipient_name: name, rejected: false }) | ||
| } |
There was a problem hiding this comment.
Does changing the query to use Rails methods change something about performance, or is this only (very welcome) cleanup?
|
|
||
| scope :with_includes_for_blurb, lambda { | ||
| includes(:pseuds, :approved_collections, :stat_counter) | ||
| includes(:pseuds, :approved_collections, :approved_unrevealed_collections, :stat_counter) |
There was a problem hiding this comment.
I suppose there is no way to cleverly use the fact that approved_unrevealed_collections is always a subset of approved_collections to avoid preloading the collections and collection items twice?
| include LoginMacros | ||
|
|
||
| describe "#index" do | ||
| it "renders bookmark items whose bookmarkable is an unrevealed work" do |
There was a problem hiding this comment.
since this (and the test in requests/tags_n_plus_one_spec.rb) aren't testing N+1 behaviour, should they maybe be in the normal controller specs instead of the N+1 specs? Or could they be converted to serve as a regression test for the N+1 fixed here (e.g. by having multiple items and relying on the strict loading to raise) since so far none of the tests cover that specifically?
Edit: I suppose the strict_loading would also raise for a single item, so these tests do check it... could that be made clearer, maybe via the test name or a comment?
| if params[:view_adult] | ||
| cookies[:view_adult] = "true" | ||
| elsif @work.adult? && !see_adult? | ||
| @work = Work.with_includes_for_blurb.find(@work.id) |
There was a problem hiding this comment.
Since it's just one work doesn't this effectively do nothing? Or even potentially even preload too much if the blurb is cached? (This question applies to all the additions of the scope to a Work.find)
| end | ||
| @readings = @readings.order("last_viewed DESC").includes(work: :pseuds) | ||
| @readings = @readings.order("last_viewed DESC").includes( | ||
| work: [:pseuds, :approved_collections, :approved_unrevealed_collections, :stat_counter] |
There was a problem hiding this comment.
Could this instead use a reusable BLURB_INCLUDES similar to bookmarks? (No idea why I didn't think of that when I touched this code...)
| def index | ||
| @user = User.orphan_account | ||
| @works = @user.works | ||
| @works = @user.works.for_blurb |
There was a problem hiding this comment.
This page barely even loads on staging with 5k orphaned works, but sure, we can preload the blurbs for approximately 980 000 unpaginated works on production, this crime was already here before you came along and optimised it :D
2 participants
Issue
https://otwarchive.atlassian.net/browse/AO3-7457
Purpose
This removes an N+1 when rendering mystery blurbs on many, many different pages.
Prior to this change, rendering a mystery blurb called:
This is a guaranteed N+1, because scope calls on associations cannot be preloaded.
The fix here is to change that into a relation,
approved_unrevealed_collections, which can be preloaded.As a regression test/enforcement mechanism, we use strict_loading: true to ensure that exceptions are raised in dev/test if this new association is ever not preloaded. Once that exception was raised, all I had to do was backfill through all the controllers to add
for_blurbpreloads where missing and then the exceptions went away.Because this adds
for_blurbpreloads in a number of places, it probably also removes additional N+1s from the non-mystery blurb as well.This is part of (but does not close) AO3-7457 because it is cleaning up data access for this set of partials before adding fragment caching around it.