Skip to content

AO3-4438 Preload bookmarkable blurb associations on bookmark listings - #6031

Open
pmonfort wants to merge 2 commits into
otwcode:masterfrom
pmonfort:AO3-4438
Open

pmonfort wants to merge 2 commits into
otwcode:masterfrom
pmonfort:AO3-4438

Conversation

@pmonfort

Copy link
Copy Markdown
Contributor

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-4438

Purpose

Bookmark.for_blurb includes the bookmarkable, but since it's polymorphic it can't preload each type's own blurb associations, so uncached blurbs query pseuds, collections, and stat counters once per bookmark (~15 queries per bookmark on a user's bookmarks page).

This overrides Bookmark.load_from_elasticsearch to add a preload pass that groups bookmarkables by class and applies each class's for_blurb includes. That brings the user listing from ~15 down to ~10 queries per bookmark (a default page of 20 bookmarks goes from ~320 to ~230 queries), the same uncached blurb cost the works listing has. The base method in lib/searchable.rb says "Override for special behavior", and BookmarkableDecorator already does the same.

The uncached user-listing spec is the one that fails without the fix. The tag and cached specs pass on master already, they're there to cover both listings from the issue and catch regressions.

Testing Instructions

Automated tests are the main verification here (spec/requests/bookmarks_n_plus_one_spec.rb). To see it by hand in a dev environment:

  1. Create several bookmarks for one user and let them index.
  2. Load /users/<user>/bookmarks and watch the SQL log: before this change there's one StatCounter/Pseud/Collection query per bookmarked work; after it, one batched IN (...) query per association.
  3. The listings themselves should look exactly the same as before.

Credit

Pablo Monfort (he/him)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant