perf(books): reuse the author book list and cache BuildWorkGroups per author (#163, #172) - #218
Open
jordanfelle wants to merge 5 commits into
Open
jordanfelle wants to merge 5 commits into
jordanfelle wants to merge 5 commits into
Conversation
…oring sync (Chaptarr#163) GetSyncUpdatesForMutations (called via SaveEntity on every book save) calls ApplyMutationSyncForWorkGroup once per author-owned work group when that author has SyncMonitoredAcrossFormats enabled. That path could call EnsureOneMonitoredOnFormat -> CanEnableMonitoringForMediaType -> HasCompatibleRootFolderForMediaType, which called _rootFolderService.All() fresh every time - an uncached full table read of a small, rarely-changing table, repeated once per media type per work group per changed book. Fetch it once per author (only for authors that actually have sync enabled) and thread it through instead. Existing call sites that aren't on this hot path (ApplyInsertSyncDefaults, BuildReconcileSyncUpdates, the direct CanEnableMonitoringForMediaType caller) are unchanged - the new parameter is optional and falls back to the original per-call fetch when not supplied. Measured on an isolated copy of a production database, on top of Chaptarr#166 and Chaptarr#167: the RootFolders query, which was running ~2,000/min during a RefreshAuthor pass, dropped out of the top statements entirely.
…tching+re-cloning it per book saved (Chaptarr#163, Chaptarr#172) BookService.SaveEntity is called once per book during a refresh, and GetSyncUpdatesForMutations unconditionally re-fetches and re-clones an author's ENTIRE book list from the database every time, when SyncMonitoredAcrossFormats is enabled - once per book saved. For an author with N books, that's up to N re-fetches of an N-sized list: O(N^2). Charles Dickens (10,107 books) and Mark Twain (5,695 books) both have this flag enabled, as do 1,821 authors in a real library tested against. Watching a live refresh with Chaptarr#166/Chaptarr#167/Chaptarr#168 already applied: Mark Twain alone kept chaptarr pinned at 100% CPU for 25+ minutes with Postgres nearly idle - confirming the remaining cost had shifted from database time (fixed by the earlier three PRs) to pure in-process repetition of this O(N^2) pattern. RefreshBookService.RefreshBookInfo(List<Book> books, ...) already has this author's full local catalogue in memory before it starts saving books one at a time. This threads that list through as an optional hint so GetSyncUpdatesForMutations can reuse it instead of re-querying, falling back to the original always-correct DB fetch whenever the hint is missing or doesn't actually cover the author being processed. Deliberately NOT added to IBookService: doing so would force every hand-written IBookService test double in the suite (25+ files) to implement a new interface member they have no use for. Instead this is a second, non-interface overload on the concrete BookService class; the one caller that has the hint available (RefreshBookService) checks for the concrete type and uses it there, and falls back to the plain interface call otherwise - every other caller and every existing test double is completely unaffected. Testing: full Chaptarr.Core.Test suite (3,008 tests) passes, including the dedicated BookServiceFormatMonitoringSyncFixture and RefreshAuthorServiceMonitoringFixture covering this exact cross-format monitoring logic.
…per book saved (Chaptarr#163, Chaptarr#172) Found via dotnet-trace against the live instance while Charles Dickens (10,107 books) was refreshing: BuildWorkGroups accounted for ~99% of sampled CPU time in a 30-second window, dwarfing everything the earlier commits in this PR already fixed. BuildWorkGroups is itself O(N^2) internally (a pairwise WorkIdMatcher.CrossFormatSafeMatches sweep over the remaining book list for every book), and GetSyncUpdatesForMutations calls it once per book saved - so for an N-book author it's an O(N^2) call happening up to N times. Reusing the author's book list (the previous commit in this PR) fixed the database side of that repetition; this fixes the CPU side, since the computation itself was still repeated in full every time. The grouping only depends on provider identity tokens, which don't change mid-refresh-pass for a given author, so it's safe to compute once and reuse - cached on the same authorBooksHint reference already threaded through for the database fetch, with the same null-hint fallback (always recompute) for every non-refresh caller. Testing: full Chaptarr.Core.Test suite (3,008 tests) passes.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 26, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
Author
|
Measured on a live library (346k books) while refreshing large authors with this PR deployed: the monitoring-sync author-book load that this PR targets no longer appears among the top statements. The remaining per-book full-catalogue read was EnsureUniqueTitleSlugs, fixed separately in #221. Marking ready for review. |
jordanfelle
marked this pull request as ready for review
September 26, 2026 20:12
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 26, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
…d regroup when books change The hint (the author's local books) and the BuildWorkGroups cache were instance fields on singleton services and were justified by 'RefreshAuthor is a disk-access command, so it never runs concurrently'. RefreshAuthorCommand neither requires disk access nor is type exclusive for a single author, so refreshes of different authors run concurrently and shared the hint, and the two cache fields (source, groups) were written separately, so an interleaving could leave one author's groups keyed to another author's hint. The cache was also keyed only on the hint list reference: a refresh moves and merges books in place, so a cached grouping could keep a book in a work it had left, and it held whichever Book instance was first seen instead of the instance being saved. Deleted books stayed in the hint. Hold the hint and the grouping in one AuthorBooksHint object stored in an AsyncLocal for the duration of RefreshBookInfo, remap cached groups onto the current instances by id, regroup when any identity that grouping depends on changes, and drop deleted books from the hint. Adds tests.
/p:NuGetAudit=false in Dockerfile.build was a local build convenience that slipped into the first commit of this PR; it is unrelated to the change.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 26, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
# Conflicts: # src/NzbDrone.Core/Books/Services/RefreshBookService.cs
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.
DRAFT. Depends on #168 (
perf-163-rootfolders-refetch-per-workgroup); stacked on that branch. Continues #163/#172.Problem
GetSyncUpdatesForMutationscallsBuildWorkGroupsonce per book saved, and each call re-fetches the author's book list and recomputes the groups. A dotnet-trace against a live instance while Charles Dickens (10,107 books) was refreshing showedBuildWorkGroupsat about 99% of sampled CPU in a 30-second window. It is itself O(N^2) (pairwiseWorkIdMatcher.CrossFormatSafeMatches), so for an N-book author it is an O(N^2) call made up to N times.Fix
BuildWorkGroupsper author instead of recomputing per book saved (fixes the CPU side).Verification
Review update (concurrency and staleness)
An adversarial review found that the first version kept the hint and the work-group cache in fields shared across authors. Per-author
RefreshAuthorCommands are not disk-access exclusive and run concurrently (3 by default, more when the command thread count is raised, #222), so one refresh could pair another author's grouping with its own hint and apply monitoring changes to the wrong books. Fixed:AuthorBooksHintobject held in anAsyncLocalfor the length ofRefreshBookInfo(per refresh, not shared).Bookinstances (sync decisions use current monitored flags), and rebuilt whenever any identity the grouping depends on changes (provider ids move and merge during a refresh).Note: the hint avoids the database re-fetch of the author's books, not the per-save clone of them; that clone remains.