Repository navigation
Replies: 2 comments
|
Thank you for the detailed analysis. In the paginated recording list, share counts, duplicates, per-user states, edit permissions, owners, tags and folders are now loaded once per page. A page takes the same number of SQL statements for 25 rows as for 100, and the response is unchanged. The inbox and shared-with-me lists are loaded in the same way. This is included in v0.10.16-alpha. |
0 replies
|
Thank you! I hope this helps improve the project’s efficiency and speed. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Description
GET /api/recordingsselects one page of recordings and then runs several queries per row while it serializes them, so a page of 25 recordings costs between 80 and 205 SQL statements depending on what the rows contain, and a page of 100 owned recordings costs 305.On a throwaway SQLite database built by the test suite, with internal sharing enabled and one page per scenario, the endpoint issued 80 statements for 25 owned rows with no tags, folder or file hash, 157 for 25 owned rows that each have a folder, a file hash and a tag, 180 for 25 shared rows with a distinct owner per row and
can_edit=True, and 205 for the same 25 shared rows withcan_edit=False.The per-row work depends only on how many rows are serialized, not on how selective the query is, so the round-trip count grows linearly with
per_pageeven though the query itself is already paginated.This matters most for shared libraries and tagged collections, where the per-row lookups are the mandatory ones and page latency is dominated by the round trips rather than by the page SELECT.
Affected Version
v0.10.15-alpha
Requested Feature
Batch every per-row lookup for the current page into a fixed set of queries, so a page costs roughly 8 to 12 statements regardless of how many rows it holds and how many shares, states, tags, folders and duplicates those rows have.
The JSON payload must not change, including the fields that are easy to get wrong:
share_info,duplicate_infoon shared rows,has_group_tags,is_inbox,is_highlighted,is_archived,can_edit,shared_with_countandpublic_share_count.Proposed Implementation
COUNTs into_list_dict(src/models/recording.py:370-377) with two grouped queries over the page ids,SELECT recording_id, count(*) ... WHERE recording_id IN (...) GROUP BY recording_id, and pass the resulting maps intoto_list_dictas optional arguments. Keep the current per-row path as the fallback when no map is supplied, so detail, share and export callers are unchanged.Recording.user_id == self.user_id(src/models/recording.py:280-307), whileget_duplicate_info_mapfilters by the viewer (src/models/recording.py:245-278), so passing the viewer map for shared rows would change the payload. Build one map for the page's distinct(user_id, file_hash)pairs in a single query and key it by recording id, or runget_duplicate_info_maponce per distinct owner when a page has few owners. The current shared-row output is reproduced by the owner-scoped query: with an owner holding two copies and a viewer holding two other copies under the same hash, the shared row reports the owner's two ids, and the viewer-scoped map reports the viewer's two ids instead.SharedRecordingStateonce for the page and the viewer (recording_id IN (...) AND user_id = viewer) and feed both inbox/highlighted and archived state from that map. Todayget_user_recording_status(src/app.py:248-276) andget_user_archived(src/services/recording_state.py:19-23) each query the same table for the same row.can_editfrom the batchedshare_mapthat the route already builds (src/api/recordings.py:1761-1770) instead of callinghas_recording_access(..., require_edit=True)per row (src/api/recordings.py:1789), which repeats the sameInternalSharelookup and, whencan_editis false, adds a group-adminJOIN(src/app.py:187-245). Batch the group-admin check for the page ids that need it into one query that returns the set of recording ids the viewer administers.User.id.in_(...)query instead ofdb.session.get(User, recording.user_id)per shared row (src/api/recordings.py:1796). The identity map already collapses repeated owners, so this mainly removes the first lookup per distinct owner and makes the cost explicit.get_visible_tagsonce per row and reuse the result. It currently runs inside_serialize_tags_and_folder(src/models/recording.py:321) and again in the route forhas_group_tags(src/api/recordings.py:1824), and each group tag costs aGroupMembershipquery (src/models/recording.py:149-153). Eager-load the page's tag associations withselectinloadon the page query, and use a lightweight tag and folder serializer for lists that does not callTag.to_dictandFolder.to_dict, because those load every recording association and count every recording in the folder (src/models/organization.py:225-253,src/models/organization.py:137-163). If the list needs the counts, batch them in the same grouped queries as the share counts.load_onlyprojection for the page query, sinceto_list_dictnever readstranscription,summaryornotes(src/models/recording.py:357-415), so the list stops transferring large TEXT columns it does not serialize.before_cursor_executelistener and asserts a fixed upper bound for a 25-row owned page and a 25-row shared page with tags and folders, plus the same bound atper_page=100; add a payload equality test against the current output for a fixture with owned rows, shared rows, tags, folders, duplicates and archived/highlighted states.Expected Behavior
per_pagegrows to 100.duplicate_infoon shared rows,share_info,has_group_tagsand the per-user inbox, highlighted and archived flags.duplicate_infofor a shared recording still describes the owner's other copies, not the viewer's.to_list_dictandto_dictkeep their current behavior through the existing fallback when no maps are passed.Root Cause Analysis
The route builds the page statement, executes it once (
src/api/recordings.py:1759), batch-loads the viewer'sInternalSharerows for the page (src/api/recordings.py:1761-1771), and then enriches every row in a Python loop (src/api/recordings.py:1774-1828). Inside that loop,to_list_dictruns two unconditionalCOUNTqueries, one forInternalShareand one forShare(src/models/recording.py:370-377), and theshare_mapargument only affects tag serialization (src/models/recording.py:379), so it never skips the counts; for shared rows the results are then overwritten with zero (src/api/recordings.py:1800-1801). The paginated call also omitsduplicate_info_map(src/api/recordings.py:1775), so every row with afile_hashfalls back to the owner-scoped duplicate query (src/models/recording.py:417-435, query atsrc/models/recording.py:280-307). On top of that, the loop callsget_user_recording_status(src/api/recordings.py:1782),get_user_archived(src/api/recordings.py:1786) andhas_recording_access(src/api/recordings.py:1789) per row, each of which queriesSharedRecordingStateorInternalSharefor shared rows, and callsdb.session.get(User, ...)per distinct owner (src/api/recordings.py:1796). Finally,get_visible_tagsruns a second time forhas_group_tags(src/api/recordings.py:1824) after it already ran during tag serialization (src/models/recording.py:321), andTag.to_dictloads every association of each tag to computerecording_count(src/models/organization.py:253). The current behavior deviates from the desired one because all of this work is per row and page-scoped data that could be fetched once for the page.Additional Context
Measured with a throwaway SQLite database initialized by
initialize_database, internal sharing patched on, and abefore_cursor_executelistener counting statements for oneGET /api/recordingsper scenario:The 25-owned baseline breaks down as 5 fixed statements (owned ids, shared ids, count, page SELECT, viewer share map) plus 3 per row: the two share counts and the lazy load of
tag_associations. The 100-owned page confirms the linear growth: 5 fixed plus 3 per row.Response times in this run were 29.6 ms, 48.8 ms, 53.7 ms, 59.1 ms and 90.4 ms respectively; the database is tiny, so the statement count is the transferable number, not the milliseconds.
The run used Python 3.13 with numpy 2.2.6 in place of the pinned 1.24.3, which has no wheel for that interpreter; the endpoint path does not use numpy, so the counts are unaffected.
The payload contract has 32 keys per row:
audio_available,audio_deleted_at,audio_duration,audio_ready,can_delete,can_edit,completed_at,created_at,deletion_exempt,duplicate_info,file_size,folder,folder_id,has_group_tags,has_notes,id,is_archived,is_highlighted,is_inbox,is_owner,is_shared,keep_audio_only,meeting_date,mime_type,original_filename,participants,public_share_count,shared_with_count,status,tags,title,updated_at, plusowner_usernameandshare_infoon shared rows.share_infoitself carriescan_edit,can_reshare,owner_username,share_idandshared_at. The pagination object carrieshas_next,has_prev,page,per_page,totalandtotal_pages.This item follows the pagination count fix and the transcript chunk index that already shipped (
src/api/recordings.py:1751,src/models/recording.py:534). It does not depend on a schema change, so it can be reviewed and merged on its own; a later change that rewrites the access filter as a SQL set operation would reduce the page SELECT and count cost, but the per-row layer described here stays until it is batched.All reactions