perf(books): bulk-load editions before format-sync work grouping (N+1 per save) - #265
Open
jordanfelle wants to merge 2 commits into
Open
jordanfelle wants to merge 2 commits into
jordanfelle wants to merge 2 commits into
Conversation
…nstead of one query per book Book.Editions is lazy-loaded. BuildWorkGroups (BookIdentity.GetProviderIdentityTokens -> BookEditionIdentity.GetOrderedEditions) and CloneStoredBook (GetAsin) both read it, and the format-sync pass hands them every one of an author's books on each save (UpdateMany, insert defaults, bulk reconcile). That issued one Editions query per book, per save - a stack sample of a bulk refresh spent most samples in that lazy load or in the DB round trips around it, on a single thread. Add PreloadEditions (one GetEditionsByAuthor per author) and PreloadEditionsByBook (one IN query for a given set) and call them right after the books are fetched and before they are cloned or grouped. Only books whose editions have not loaded are filled, with the same rows the lazy loader returns (Editions by BookId), so already-loaded books keep their in-memory editions. Tests: an UpdateMany over 25 books performs no per-book lazy loads and one bulk author query; each book gets only its own editions and a loaded book is left untouched. Both fail without the change. The strict edition-service proxy in BookProviderIdCanonicalizationFixture learns the bulk GetEditionsByBook overload.
…re-running the author-wide editions query Adversarial review of Chaptarr#265: the second PreloadEditions call in GetSyncUpdatesForMutations re-loaded every edition for the author whenever the caller's changed books were unloaded (the normal case). Use the by-book IN query for just those books, and pin exactly one author-wide query in the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Book.Editionsis lazy-loaded. In the format-sync pass,BuildWorkGroups(BookIdentity.GetProviderIdentityTokens->BookEditionIdentity.GetOrderedEditions) andCloneStoredBook(GetAsin) both read it, and the pass is handed every one of an author's books on each save (UpdateMany, insert defaults, bulk reconcile,PersistWithLifecycle,SetMonitored*). Each book costs its ownEditionsquery, on every save.Found by sampling a live bulk refresh with
dotnet-stack(4 samples, one busy thread): two landed inGetOrderedEditions->LazyLoad()underBuildWorkGroups, the other two in Npgsql waiting on anUPDATE/DELETEfrom the same loop. Postgres CPU and memory were not saturated; the thread was serialised on round trips.Change
PreloadEditions(authorId, books): oneGetEditionsByAuthorper author, called right after the author's books are fetched and before they are cloned or grouped.PreloadEditionsByBook(books): oneINquery for a specific set (the stored copies cloned inUpdateMany,PersistWithLifecycle,SetMonitored,SetMonitoredForMediaType).EditionsbyBookId), so already-loaded books keep their in-memory editions and behaviour is otherwise unchanged.Tests
update_many_should_load_author_editions_in_one_query_instead_of_one_per_book: 25 books, no per-book lazy loads, at most two bulk author queries.update_many_should_give_each_book_only_its_own_editions_and_not_touch_loaded_books.Chaptarr.Core.Test: 3037 passed. The strict edition-service proxy inBookProviderIdCanonicalizationFixturegains the bulkGetEditionsByBookoverload.Not measured
I have no before/after wall-clock for a refresh yet; the claim is the query-count change, not a speedup figure.