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

Playlist performance improvements #4597

Merged

Conversation

absidue
Copy link
Member

@absidue absidue commented Jan 25, 2024

Playlist performance improvements

Pull Request Type

  • Bugfix
  • Performance Improvement

Related issue

#4578
Mostly solves the issue, however we will need to do some more involved refactoring to fix the watch page entirely.

Description

This pull request addresses some of the performance problems concerning gigantic user playlists (over 17k items) that were brought up in the linked issue. It doesn't completely solve the issue, especially on the watch page, as we are still rendering at least one <div> per playlist item, so that we can figure out when to load the items (there is a reason YouTube only displays a maximum of 200 videos on their watch page), we also don't unload items when they disappear from view again. Fixing the watch page properly will require switching to a virtual scrolling approach, so that we really only have as many HTML elements in the DOM as we really need, which is out of scope of this pull request.

Here is the list of changes that this pull request makes:

  1. Add pagination to the playlist page for user playlists, extra pages can be loaded with the load more button.
  2. Fixes the load more loading icon being displayed off-screen (it's now used for both local API and user playlists, we don't support pagination for Invidious).
  3. Avoids caching users playlists when switching from the playlist page to the watch page, as user playlists are already in the store so it's just extra work. The caching is still in place for remote playlists, as it saves making some network requests on the watch page.
  4. Various minor changes to the playlists store, in the worst case (every possible fixable issue is present) these shave about 30 milliseconds off, in the best case (all checks succeed, no repairs to the data necessary) they add a negligible overhead (less than a millisecond), so they seem worth it.
  5. Makes the ft-list-video element slightly cheaper to render:
    • Only creates the description element when there is actually a description (user playlists don't have one, the code in the store explicitly removes them)
    • Removes an unnecessary template wrapper
    • Replaces the watched property with the historyEntryExists computed property, as they served the same purpose.
  6. Fix the video number not getting lazy loaded, it will now load at the same time as the rest of the video item.

The final result is that the playlist page loads quickly, as it only has to render 100 items initially, however if you press load more enough times, things do slow down, because we don't unload items once they are loaded. The watch page is still slow to load, it did go from 16.24 to 14.88 seconds with the test playlist on my computer, but that is still slow. Doing a performance benchmark shows that the majority of time is spent in Vue's internals, as it struggles with the sheer number of component instances and DOM elements (16200+ of each).

Testing

  1. Backup the playlists in your dev environment (Almost the same directory as usual, replace the last path segment (FreeTube) with Electron https://docs.freetubeapp.io/usage/data-location/)
  2. Download this database that contains a test playlist with 16200 items inside it.
  3. Rename it to playlists.db
  4. Place it in the dev data location
  5. Test the playlist page and watch page for that playlist

The playlist was created by copying https://www.youtube.com/playlist?list=PLxYGDfxfyiF85ZwSBchVX7datipeOJhtH multiple times inside itself.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.19.1

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 25, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 25, 2024 18:20
@PikachuEXE
Copy link
Collaborator

Not necessary need to be fixed in this PR
But I do notice the watch page playlist still load many playlist items
Perhaps that's issue making the rendering slow
image

@PikachuEXE
Copy link
Collaborator

When moving a video in a playlist up or down, the animation for the videos swapping plays twice is fixed in f1d700d (#4593)

Since we probably only merge this PR instead of that I think it's better to also include that fix as well

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like too many nesting levels :)

src/renderer/views/Playlist/Playlist.js Outdated Show resolved Hide resolved
@absidue
Copy link
Member Author

absidue commented Jan 26, 2024

Not necessary need to be fixed in this PR
But I do notice the watch page playlist still load many playlist items
Perhaps that's issue making the rendering slow

Yes, I'm sorry that I didn't explain that well enough in the pull request description. That is out of scope of this pull request though, as it will require a complete rethink of how we do lazy loading (see section about virtual scrolling). Our current lazy loading implementation throughout the app, requires you to have one div per item (video, channel, playlist) to exist at all times, so that you can detect when it becomes visible (gets scrolled into view) and trigger the loading of it's children.

A better but more complicated to implement approach would have a parent element be in charge of the entire list, making sure that there are only ever enough items in the DOM, as are actually visible items. That probably involves listening to scroll events and reacting based on that or something. I haven't looked into how other virtual scrolling implementations work, so I not quite sure how we would do it.

@PikachuEXE
Copy link
Collaborator

I am not talking about the no. of divs
But the elements that should be invisible are rendered as visible (my fault for not expanding one of them when taking the screenshot)
But I can take another look after this is merged

Co-authored-by: PikachuEXE <git@pikachuexe.net>
@absidue
Copy link
Member Author

absidue commented Jan 29, 2024

When moving a video in a playlist up or down, the animation for the videos swapping plays twice is fixed in f1d700d (#4593)

Since we probably only merge this PR instead of that I think it's better to also include that fix as well

I can't seem to be able to reproduce that problem on this pull request, happy to try fixing it if you can provide steps to reproduce it.

@absidue
Copy link
Member Author

absidue commented Jan 29, 2024

I am not talking about the no. of divs But the elements that should be invisible are rendered as visible (my fault for not expanding one of them when taking the screenshot) But I can take another look after this is merged

As far as I can tell this is working as expected, all divs with the placeholder class don't have any children. Once an item has been loaded it stays loaded, we don't unload them.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jan 30, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 30, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No conflict good enough

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very unrelated to this PR but, big number likes to hide behind thumbnail

VirtualBoxVM_APnwNpnU8V.mp4

@FreeTubeBot FreeTubeBot merged commit 351fdb9 into FreeTubeApp:development Feb 1, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 1, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Feb 1, 2024
…into custom-builds/current

* feature/playlist-search-videos-in-one-user-playlist-2:
  ! Fix load more button appears when searching & visible items under pagination limit
  * Update single playlist view for user playlists to add search video function
  Playlist performance improvements (FreeTubeApp#4597)
  ! Fix playlist type not passed when playing next/prev item in a user playlist (FreeTubeApp#4623)
  Properly localize playlist view and video counts (FreeTubeApp#4620)
  Translated using Weblate (Croatian)
@absidue absidue deleted the playlist-performance-improvements branch February 1, 2024 12:48
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Feb 3, 2024
* development: (92 commits)
  Make video info section more concise (FreeTubeApp#4338)
  Playlist performance improvements (FreeTubeApp#4597)
  ! Fix playlist type not passed when playing next/prev item in a user playlist (FreeTubeApp#4623)
  Properly localize playlist view and video counts (FreeTubeApp#4620)
  Translated using Weblate (Croatian)
  Translated using Weblate (German)
  Translated using Weblate (Croatian)
  Fix search bar handling of Invidious channel URLs (FreeTubeApp#4568)
  Local API: List related games in featured channels section (FreeTubeApp#4562)
  Workaround community post slider dependency incorrectly calculating its size (FreeTubeApp#4598)
  Add support for viewing movie trailers with local api (FreeTubeApp#4391)
  Bump the eslint group with 2 updates (FreeTubeApp#4616)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Bump electron from 28.1.4 to 28.2.0 (FreeTubeApp#4611)
  Translated using Weblate (French)
  Bump the eslint group with 4 updates (FreeTubeApp#4581)
  Bump lefthook from 1.6.0 to 1.6.1 (FreeTubeApp#4608)
  Bump marked from 11.1.1 to 11.2.0 (FreeTubeApp#4612)
  Bump webpack from 5.89.0 to 5.90.0 (FreeTubeApp#4610)
  ...
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

Successfully merging this pull request may close these issues.

5 participants