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

Improve performance of the watch history handling #4017

Merged

Conversation

absidue
Copy link
Member

@absidue absidue commented Sep 10, 2023

Improve performance of the watch history handling

Pull Request Type

  • Bugfix
  • Performance Improvement

Related issue

closes #3796

Description

Currently all watch history interactions go through a chronologically sorted list, which is great for the history page, so that it displays items in the right order, but less than ideal if you need to check if a video is watched. For most users this will likely never be a problem, but for users like the ones in the issue, that have gigantic watch histories (32k+), this means that video lists and especially the subscriptions page, slow down.

This pull request introduces an additional history object, this time it is an object (vuex doesn't support Maps) with the video ids as the keys. This should allow for almost instant lookups, which should speed up video lists and any other place that accesses the watch history to check that an entry exists and read properties from them.

Testing

Check that history handling still works as expected:

  • Watch history and watch progress get saved and restored on the next playback (well the watch progress might not get restored, as it is now fast enough that it might try to do it before the video is loaded, yes it's a race condition that I am fully aware of, using a link with a timestamp doesn't work either if you have no history)
  • Adding and removing and item from the history should live update it's state in a second window
  • Watch history page still works
  • Exporting history still works
  • The hide watched items on the subscription page setting still works

@Nemo157 @apatheticslacker @StrangestNoob
As none of the devs have a large enough history to encounter any performance problems, it would be really helpful if you could test this pull request and check if it solves the problem for you. (Ideally all 3 of you, but mainly @Nemo157 as you opened the bug report).

Here is a test build you 3 can download, so that you don't have to set up a development environment to test it: https://github.com/absidue/FreeTube/actions/runs/6136849377

Desktop

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

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 10, 2023 11:59
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 10, 2023
@Nemo157
Copy link

Nemo157 commented Sep 10, 2023

I can confirm this version gets switching between profiles on the subscriptions page back to being instant, from the ~2s it is currently.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@kommunarr
Copy link
Collaborator

kommunarr commented Sep 10, 2023

All seems great, except I see a new error when importing history that was exported from this PR (Picture 1) compared to history that was exported from the development branch (Picture 2). I don't think it has any deleterious effect, though; you probably just need to update the error display logic.

Picture 1
Screenshot_20230910_183053

Picture 2
Screenshot_20230910_183753

@PikachuEXE
Copy link
Collaborator

@jasonhenriquez
Could it be the issue of #3006 not updating the import logic? (which would be a separate issue)
lastViewedPlaylistId not in required keys (well it's not required, there should be optional keys)

@absidue
Copy link
Member Author

absidue commented Sep 11, 2023

I think the issue is probably unrelated as this doesn't touch any of the import code.

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.

Readability

src/renderer/components/ft-list-video/ft-list-video.js Outdated Show resolved Hide resolved
src/renderer/components/ft-list-video/ft-list-video.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
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.

Tested others except

Watch history and watch progress get saved and restored on the next playback (well the watch progress might not get restored, as it is now fast enough that it might try to do it before the video is loaded, yes it's a race condition that I am fully aware of, using a link with a timestamp doesn't work either if you have no history)

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
PikachuEXE
PikachuEXE previously approved these changes Sep 13, 2023
kommunarr
kommunarr previously approved these changes Sep 13, 2023
@absidue absidue requested a review from PikachuEXE September 13, 2023 17:00
@FreeTubeBot FreeTubeBot merged commit d333990 into FreeTubeApp:development Sep 14, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 14, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Sep 14, 2023
* development:
  Improve performance of the watch history handling (FreeTubeApp#4017)

# Conflicts:
#	src/renderer/components/ft-list-video/ft-list-video.js
#	src/renderer/store/modules/history.js
@absidue absidue deleted the improve-history-performance branch September 14, 2023 06:42
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.

[Bug]: Slow rendering with large history
7 participants