-
-
Notifications
You must be signed in to change notification settings - Fork 419
feat: add infinite scrolling, improve page performance #1119
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
feat: add infinite scrolling, improve page performance #1119
Conversation
|
Thank you so much for this PR! Overall the changes look great, and the infinite scrolling feature appears to work very well. I also appreciate how it doesn't completely replace the pagination system and instead is another option for users to choose, while making the pagination system more usable and performant in the process. I've got one major point of feedback so far and then some minor observations from there. Most importantly, it appears that on my machine the CPU affinity and multithreading changes are majorly impacting performance. Before these changes the cached thumbnails would populate almost instantly, and the new renders would populate very quickly and you could see each thread working at them. After these changes, the cached thumbnails pop in one-by-one when scrolling the page and new renders take what I could roughly estimate to be around 10x longer. This is all while still having the UI react slugishly while renders or even cache grabs are taking place. When reverting the CPU affinity changes in this PR, I was able to restore the previous performance. Below is a screen recording of the performance as-is: And the performance when the CPU affinity and other multithreaded changes are reverted: Other than addressing that, at the moment I just have one request and one question: Could you change the infinite mode state from being reliant on a page size of zero to be reliant on a new internal setting that overrides the page size check? It doesn't matter so much for some of the internal mechanisms, but I feel it would be a better user experience to be able to hit a boolean checkbox in the settings for infinite scrolling that disables the page size box and also doesn't forget the last page size that they would normally use. You don't even have to do the full UI/translations work for that if you don't want in this PR, you could just add the internal setting for an unlimited page size and I could take care of the UI work in a followup PR if you'd prefer. And finally I was curious if having something like the next two or three rows be rendered and/or prepared off-screen would improve the usability? Currently it seems that requests for rendering are only made as soon as the grid widgets come into view, which means there's always going to be some "pop-in", especially if there's no cached thumbs yet, and this also contributes to a bit of a "sluggish scrolling" feeling when scrolling down, while scrolling back up to already rendered areas is buttery smooth. I want to reiterate how appreciated and incredibly useful this feature is! This has been something I've wanted badly in the program since day one, and you've done an incredible job on this so far! 👍 |
|
I've reverted the cpu affinity changes. I think most of my stutters are from having 32 cpu threads. TagStudio spawns a render thread for each one which really starves the main thread from the GIL. A setting for the number of render threads would probably be a better solution for this. I added an infinite_scoll option to GlobalSettings and to the UI. Not sure if I did the UI and translations correctly though. It should have already been loading 3 extra rows. But it would pick either the top or bottom rows depending on which were closer. I've changed it to just load both. |
The rule of thumb remember from my OS lectures was to have number of cores + 1 threads for best performance iirc so maybe give that a try |
|
That would be best for rendering thumbnails as fast as possible. But since each thread has to share Pythons GIL it ends up causing the main thread to wait much longer for it's turn to execute. Also the faster you generate thumbnails the more the main thread needs to process in a single frame potentially causing stutters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good! Thank you so much for your work on this, it's a great feature + improvement to have!
Summary
Use a custom
QLayoutfor the main grid view that reuses item thumbs as they go off screen.Allow setting page_size to 0 for infinite scrolling. Performs well even with ~650_000 entries
Selection logic has been rewritten and is now tracked by the grid layout. There are likely some bugs I missed with existing code that references
QtDriver.selected. Anything that modifiesQtDriver.selectedis invalid since it's just a copy of the actual data.Tasks Completed