Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with very large playlists #102

Closed
nonetrix opened this issue May 17, 2021 · 21 comments
Closed

Issues with very large playlists #102

nonetrix opened this issue May 17, 2021 · 21 comments

Comments

@nonetrix
Copy link

My Liked Tracks playlist has about 4000 tracks apparently and when I try to click on it the app will freeze for a few minutes, and when it's done the client is extremely laggy for a while, I'm assuming it's spamming the API to get all of the 4000 tracks then adding them to the UI, it should lazily load them ideally.

@nonetrix
Copy link
Author

This also makes it take up a lot of ram more than the normal spottily client
ram

@kraxarn
Copy link
Owner

kraxarn commented May 19, 2021

I also noted slow performance and high memory usage with a large playlist of mine, although, it only had around 700 tracks. Tried optimizing it a bit, but it didn't really help that much, so I put it on hold for now, but it's something that needs to be looked into. Afaik, there's no easy way to do lazy loading with Qt unfortunately.

@nonetrix
Copy link
Author

nonetrix commented May 19, 2021

Hmm interesting maybe you could add a load more button at the bottom that might be more easy I don't know much about QT since I have only just started learning it

@kraxarn
Copy link
Owner

kraxarn commented May 19, 2021

Maybe, but there's still something wrong since it should never use that much memory, so it needs to be fixed/improved either way ☺️

@kraxarn
Copy link
Owner

kraxarn commented Jun 5, 2021

I found a playlist with over 8000 tracks and was able to do some testing. Loading it makes the application use 100% CPU, and around 6 GB of RAM for a couple of minutes, making the application more or less unusable in the meantime. Kind of weird there's only a 2 GB difference between 4000 tracks and 8000 tracks, but it's definitely the same issue. This is what I've found so far:

  • It's not a memory leak, as loading the same playlist twice doesn't double the memory again. I have also tried to analyze the memory usage using various memory analyzing tools, and even though I found some other potential memory issues, none were related to this.

  • It's only caused by refreshing the playlist. Loading it from cache doesn't make it use anywhere near as much memory, only about 100 MB, which seems fine for 8000 tracks. This means it's nothing related to the UI itself, so paging wouldn't help much. As a kind of temporary workaround, I made a change to not refresh the playlist if it hasn't changed (919c5b8), although, it's obviously not a proper solution and won't work for library items.

  • Something is definitely going wrong around here:

    void api::get_items(const std::string &url, const std::string &key,

@jacksongoode
Copy link

Might it make sense to have this playlist refresh function on its own thread? Or keep it separate from the UI so there is no freeze in interaction? spotify-tui works around this by requesting one page of results at a time.

@kraxarn
Copy link
Owner

kraxarn commented Jun 10, 2021

Thanks for your suggestion ☺️

I wouldn't consider the application freezes to be the main issue here. Even if it was in its own thread, the fact that the refresh would still use so many resources and take multiple minutes to complete, is still not acceptable. Qt also doesn't allow networking from other threads, which would mean it would need an entire QObject context for each thread. However, when Qt is waiting for the server to respond, the application doesn't freeze, so this issue isn't directly related to the networking.

I also don't want to work around the issue by using paging, it just doesn't feel like a good solution to me and makes, for example, sorting not possible.

@magnus-rattlehead
Copy link

I'm not getting the exact same behavior. For me the initial freezing occurs on launch and once connected to spotifyd. After the initial loading of a playlist created by me in my library, app performance is restored. I'm not too well versed in how data is retrieved and processed from Spotify but maybe the sheer amount data should be streamed in chunks in some kind of queue?

@kraxarn
Copy link
Owner

kraxarn commented Jun 6, 2022

After loading the playlist once, the tracks are cached, and loading from cache works fine. The data is already kind of loaded in chunks, as playlists are loaded in pages/chunks, the application just waits for all pages to be loaded before displaying them in the UI. Maybe only loading one page at a time could work, but refreshing playlists would require some extra logic.

@5HT2
Copy link

5HT2 commented Jul 16, 2022

I was going to comment on this, my liked tracks playlist has 12k tracks in it. Accidentally clicking it causes the behavior described earlier, and clicking back to something else still means the application is really slow and unresponsive until I restart it entirely (presumably because it continues trying to load the playlist).

Maybe it would be helpful to pin this issue?

@kraxarn kraxarn pinned this issue Jul 29, 2022
@Matipolit
Copy link

Matipolit commented Dec 16, 2022

I have been experiencing this issue with my liked tracks playlist (3.7k tracks). It utilises 100% of one core every time I open it for about 30 seconds. As for RAM, it uses about 2.8 GB and stays that way even as I browse other playlists.

@ptrcnull
Copy link
Contributor

ptrcnull commented Feb 18, 2023

there seems to be a regression on master - while v3.9 can load a 2k+ playlist with some struggles, current master (5bccf90) hangs completely and never loads the full list

edit: after a quick bisect, the culprit is 7c74d40, commenting out the following block makes the performance go back to how it was in v3.9

https://github.com/kraxarn/spotify-qt/blob/31f62e9/src/list/tracks.cpp#L525-L543

@5HT2
Copy link

5HT2 commented Feb 18, 2023

Hm. That should probably be async + cached locally / synced from server when other devices like a track if spotify API sends events for that. (in terms of, solving performance for that component).

@kraxarn
Copy link
Owner

kraxarn commented Jul 7, 2023

Network request are already asynchronous, and results are cached locally. The cache even loads fine, it's when it's trying to fetch the results from the API that causes issues.

I think the main issue here is that all results are first stored in a temporary object before being loaded into the UI to reduce complexity. While this can probably be optimized, I thought the better solution was to skip the temporary object completely, and load it directly in chunks to the UI. It adds quite a bit more complexity, but I think it's worth it due to how much nicer the user experience should be.

cc1b1f6 adds a new method of loading tracks in chunks instead. It's very experimental currently, and still mostly just a proof of concept, but it already shows huge improvements. Playlists with more than 8000 tracks can now be loaded fine, and only using around 30 MB of extra memory, instead of the gigabytes it was using before.

I'll keep this issue open until it's stable enough to fully replace the current method.

@kraxarn
Copy link
Owner

kraxarn commented Nov 26, 2023

It's now stable enough to replace the current method. It's not perfect (yet) though, it only loads a page with 50 tracks at once and some weird stuff can happen if it errors in the middle of loading tracks, but I don't see those as bigger issues than it barely being able to load large playlists.

@RokeJulianLockhart
Copy link

#102 (comment)

@kraxarn, per #102 (comment), do you intend to reinvestigate how to use a temporary object in a more efficient manner? It sounds to me like this implementation could become quite a complex option (as you previously mentioned) due to how without that abstraction the logic shall eventually become much more purpose-specific.

@JustMyGithub
Copy link

JustMyGithub commented Jan 5, 2024

I wrote a pretty long text but for some reason it is lost now. So let me summarize:

After loading the liked tracks (~ 6.3 MB json), spotify-qt performs pretty well at ~ 140MB RAM and almost no CPU usage. scrolling and sorting pretty much perfect. So there is a high potential for awesomeness - however loading the large playlist took about 8-9 minutes.

Suggestions:

  • use a separate thread for the UI. That way the UI is not blocked during loading a chunk of a playlist ( C++ 11 has native support for threads, for backward compatibilty there are alternatives like pthreads)
  • refresh playlists only on user request (at least if it is larger than x songs)
  • Indicate that x of y songs of the playlist have been loaded from the server already
  • try to parallelize requests to the spotify API (not sure whether that is possible for a single playlist)
  • maybe you can find a way to use much less RAM during load of the playlist rather easily. The json of the playlist is 6MB, so it is pretty suprising that this takes > 1GB of RAM to read its content via the Spotify API and transform it to json. (This is low priority, as the regular client uses that many RAM most of the time, but I expect it can be improved rather easily so I'd like to mention it)

PS: It is not intended to sound demanding or like me insisting on it, only suggestions to improve this nice tool even more.

Edit: my original report, found it stored in my browser

When trying 3.11 first I noticed that my liked tracks were outdated (last one added 3 month ago)
As 3.11 was 100% one core and ~1GB of RAM I assume it tried to update the list in the background. During this the UI was very laggy (Klicks sometimes took 10 and more seconds for the UI to react).

I think the UI is blocked waiting for the next playlist fragment to load. Looking at the 3 matches for "thread" in the repo it seems that no threading is used, only a mutex for the config.

When the playlist has finished loading, spotify QT performs awesome as expected. 140 MB RAM and very lightweight on CPU. Even sorting happens in notime, so this has a high potential for awesomeness :D
Trying it a second time for time measuring, it took about 8.5 minutes until the playlist was shown in the UI and after about 1 minute more the UI crashed.

For testing I also did Ctrl+A on the liked tracks playlist, At that point the UI became laggy again. After clicking "OK" in "new playlist" the UI was frozen for minutes, at some point I just killed the app.

@nonetrix
Copy link
Author

nonetrix commented Jan 6, 2024

Just downloaded to try Spotify-qt again, memory usage has indeed improved somewhat. But it's still larger than it should be ideally at 2.5GBs. Worth mentioning that my playlist is now 7,000 songs long, also it never actually displays seemingly but the memory usage does seem to settle. If it can be made to actually display the memory issue is personally not a problem for me, I have 64GBs rather than 16GBs of RAM now but most don't. I don't know if it not displaying is related or not to this issue, or displaying it will allocate even more memory. I am using NixOS with version Spotify-qt v3.9 and QT 5.15.11 on Wayland Hyprland

@kraxarn
Copy link
Owner

kraxarn commented Mar 9, 2024

@RokeJulianLockhart No, and all features currently still using the old method will be migrated to using the new method in the coming versions. The old method simply doesn't provide enough advantages to be worth trying to optimize. The simple truth is just that asynchronous c++ is difficult to use efficiently.

@JustMyGithub Loading still takes a while mostly because it can only ever load 50 tracks at once, and they are only ever loaded once at a time to avoid rate limiting and whatnot. I have never really tried to parallelize it, but just having to sync if they come out of order sounds like enough of a pain to not do it for the time being at least.

Refreshing in a separate thread is very much the next step just to not freeze the entire UI while refreshing. Haven't fully decided on implementation for threading and data transfer, but I'll start playing around with it probably after the release of v4.0.

Spotify actually sends a "snapshot" as a version to avoid having to unnecessarily refresh, but it only really works for your own playlists and is in general just very unreliable, so it's not very useful in practice. There's already an option to refresh playlists manually, so for example adding an option to not refresh automatically could be added at some point without too much work.

Memory usage is so inconsistent between systems, it's quite difficult to try and improve. For example, loading a playlist with over 8000 tracks only uses around 70 MB of RAM for me, which I find very acceptable. Maybe there are just other factors affecting memory usage I'm not aware of, but for now, I'll probably not look into it much further without any clear idea of what's causing it. I'm also using Qt 6, but that shouldn't matter much.

@nonetrix This fix was implemented in v3.11, please try that version instead and see if it works better for you.

@JustMyGithub
Copy link

JustMyGithub commented Mar 11, 2024

I don't understand your "out of sync" concerns. the REST request is "give me song 51 to 100", isn't it? So even if the result of the request 101-150 arrives before the 51-100, it should not matter at all (as the total number of elements is known)? - Either the data structure is thread-safe anyway or amutex will take care for any such issues.

Did you actually have rate-limiting issues, or is it just a concern that causes you to chose the slow way? Searching for it it seems that SPotify does not explicitly tell the number of requests allowed, but worst thing that seems to happen is having to wait for 30 seconds until the next request.

What is your test system? maybe QT on Windows has a bad memory footprint and you are using Linux?

70 MB is perfectly fine, but 2GB+ is a lot :D

@kraxarn
Copy link
Owner

kraxarn commented Jul 4, 2024

Currently, it still has to fill the items in the list in order, so the UI would have to know what to do in the case where they arrive with gaps, like inserting them in the correct place instead of always placing them at the end, or something like that. Could of course be done, and might try implementing it at some point.

I don't think rate limiting is that much of an issue when there's only a single user calling with a specific key, I just might have a "public key" anyone can use if they don't want to set the keys up themselves, but either way, having the option to load them in parallell is still a good idea, although, the refresh has to be moved to a separate thread first at least.

I don't have a Windows system currently to easily test on, but the memory usage seems fine for me on both Linux and macOS. Might be something else going on as well, I'm not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants