Speed up playlist & collection query #3015
Conversation
…ists (otherwise, return the entire playlist)
…lections (otherwise, return the entire collection)
|
The reason this works is that when we load the If one has 100 playlists with 100 books each, that means we're loading the metadata for 10,000 books just to display the list of playlists, and then when a playlist is opened, we load those 100 books in that playlist again. Instead, we only load 4 books for playlist, which should be sufficient to render the cover for each playlist, as the playlist consists of the covers of the first 4 books (if I understand correctly) As for Collections, it appears that it loads the covers of the first 2 books, so the request for the list of collections only returns 2 books per collection. Together, this vastly decreases the loading time when the total number of books in all collections/playlists is very large, in the hundreds to thousands of books, from multiple seconds to under 50ms |
| @@ -492,7 +492,7 @@ class LibraryController { | |||
| } | |||
|
|
|||
| // TODO: Create paginated queries | |||
| let collections = await Database.collectionModel.getOldCollectionsJsonExpanded(req.user, req.library.id, include) | |||
| let collections = await Database.collectionModel.getOldCollectionsJsonExpanded(req.user, req.library.id, include, 2) | |||
There was a problem hiding this comment.
Instead of making this 2 at the API level, it would probably be better to make this an optional request parameter, with a default of "get everything" so 3rd party clients and scripts can still get all items from the single API call.
Not sure what a good name would be, maybe itemCount?
There was a problem hiding this comment.
For this one, I meant something like adding
itemCount: req.query.itemCount && !isNaN(req.query.itemCount) ? Number(req.query.itemCount) : undefined,
to the payload a few lines before, and then passing payload.itemCount to getOldCollectionsJsonExpanded instead of the constant value of 2.
| @@ -513,7 +513,7 @@ class LibraryController { | |||
| */ | |||
| async getUserPlaylistsForLibrary(req, res) { | |||
| let playlistsForUser = await Database.playlistModel.getPlaylistsForUserAndLibrary(req.user.id, req.library.id) | |||
| playlistsForUser = await Promise.all(playlistsForUser.map(async (p) => p.getOldJsonExpanded())) | |||
| playlistsForUser = await Promise.all(playlistsForUser.map(async (p) => p.getLimitedOldJsonExpanded())) | |||
There was a problem hiding this comment.
Same thing as the collections endpoint, but looks like this function is written backwards for doing the paging at the database request level, so that may need to be swapped for the paging to actually work.
There was a problem hiding this comment.
To be clear, it was already requesting everything from the database and then limiting the response in memory, so maybe the getUserPlaylistsForLibrary function needs to be restructured.
There was a problem hiding this comment.
Thanks @nichwall you are right. I made a mistake initially it looks like and we are loading all the items twice.
I'm going to start fresh on this one instead of working off this PR to clean more of this up
There was a problem hiding this comment.
This explains the slow loading actually because we are iterating through every playlist and loading the playlistMediaItems when we should be getting them all in one go.
It would be helpful to also add an optional query param to limit the items returned in each playlist so I'll look into that also
There was a problem hiding this comment.
Just updated this 1576164
I wasn't able to limit the media items in that query because of a sequelize bug sequelize/sequelize#12971
The only other way I can think to implement that is to filter out media items with an order < 5 or something like that.
It would be better to migrate that endpoint to return the new data model then to spend a lot more time trying to get the old one to perform better.
|
The data model went through a migration from JSON objects in files to a relational db. There are a bunch of places where more data is loaded than necessary because the new relational data model is mapping all the data onto the old JSON objects so that the entire API didn't have to get rewritten for the migration. |
|
I investigated the way that the query works, there's not really a way to limit the number of results in the included objects that I could see? |
|
See #3015 (comment) |
Should fix #2852