Repository navigation
Batch people lookups when building item DTOs - #17571
Conversation
GetBaseItemDtos already batch fetches user data, child counts, played counts and artists before its per item loop, but AttachPeople still ran one GetPeople query per item. Rendering a page of items (for example a large playlist) fired one extra query per row. Add GetPeopleByItems to IPeopleRepository, which reads every requested item in a single query over the people mapping table and returns full PersonInfo (role, type and sort order) grouped by item id. GetBaseItemDtos prefetches this once when the People field is requested and passes it into AttachPeople, which reads from the batch instead of querying per item. The single item GetBaseItemDto path keeps its existing per item behaviour when no batch is supplied. Adds a DtoService test asserting people resolve from the batch and the per item GetPeople is never called.
|
|
Please fill out the PR template |
|
LGTM from code wise, but yes, please fill our template. |
Jellyfin API (v 12.0.0)ResultNo differences. Specifications are equivalent |
|
Changes This adds I checked the output on a live 12.0.0 server and the
The control path that does not request people is unaffected. Adds a Code assistance Used a LLM for the benchmarking harness and profiling guidance while tracking down the N+1. Issues Traced this while fixing a related playlist slowness in Finamp (finamp-app/finamp#1707). |
…hHDR10Plus ShouldRemoveDynamicHdrMetadata stripped the HDR10+ SEI from every DOVIWithHDR10Plus stream copy as soon as the client declared DOVI in its rangetype, without considering that the client may have declared DOVIWithHDR10Plus support itself. The rewritten bitstream (-bsf:v hevc_mp4toannexb,hevc_metadata=remove_hdr10plus=1) breaks HLS fMP4 playback on AVPlayer (tvOS), and no device profile combination avoids it while keeping Dolby Vision remuxing, because the dvh1 tagging in DynamicHlsController requires DOVI in the rangetype. Honor an explicit DOVIWithHDR10Plus declaration the same way the DOVIWithELHDR10Plus coexistence case already honors its own: only plan the HDR10+ removal for DOVI clients that did not declare the coexistence range. Clients that do not declare it keep the previous behavior. No upstream fix exists as of master (post-jellyfin#17571); the removal logic is unchanged since it was introduced in jellyfin#13277. Related upstream issue: jellyfin#16687. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
While fixing a playlist performance regression in Finamp (a Jellyfin client), I traced the slowness to the server: requesting
Fields=People on a list endpoint issues one people query per item. Fixing the client led me to look at Jellyfin itself.
DtoService.GetBaseItemDtosalready batch fetches user data, child counts, played counts and artists before its per item loop, butAttachPeoplestill ran one_libraryManager.GetPeople(item)query per item. Rendering a page of items with thePeoplefield enabled (a playlist, a library grid, any/Itemscall requesting people) therefore fires one extra database round trip per row.This is an N+1: a page of 100 items issues 100 separate people queries on top of the main item query.
Change
Add
GetPeopleByItems(itemIds)toIPeopleRepository, which reads every requested item in a single query over the people mapping table and returns fullPersonInfo(name, role, type and sort order) grouped by item id.GetBaseItemDtosprefetches this once when the People field is requested and passes it intoAttachPeople, which now reads from the batch instead of querying per item.The single item
GetBaseItemDtopath is unchanged: when no batch is suppliedAttachPeoplefalls back to the existing per item query, so callers that render one item keep their current behaviour.Correctness
The batched query mirrors the ordering and mapping of the existing single item people query (ordered by cast list order, mapped to
PersonInfowith the same role, type and sort order). Verified on a live 12.0.0 server: the/Itemsresponses withFields=Peopleare byte identical before and after this change, so the output is unchanged and only the query count drops.Measured impact
Live server, median of six timed requests per endpoint after a warmup:
The control path that does not request people is unaffected, confirming the win comes from removing the per item people round trips.
Tests
Adds a
DtoServicetest asserting people resolve from the batch and that the per itemGetPeopleis never called, modeled on the existing artist batch regression test. The fullJellyfin.Server.Implementations.Testssuite passes locally.