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

UX: Lag when selecting pictures #477

Closed
wiwie opened this issue Aug 21, 2020 · 29 comments
Closed

UX: Lag when selecting pictures #477

wiwie opened this issue Aug 21, 2020 · 29 comments
Assignees
Labels
enhancement Optimization, improvement or maintenance task frontend Requires experience with HTML/JS/CSS help wanted Well suited for external contributors! released Available in the stable release

Comments

@wiwie
Copy link

wiwie commented Aug 21, 2020

When browsing pictures under /photos and scrolling down a bit, such that a larger number of pictures is visible (maybe around 300), any kind of operation that changes the current selection of pictures is very laggy, i.e. clicking on a picture to select it or holding down shift and then clicking to select multiple pictures is followed by a delay of about 2s before the new selection is represented in the UI.

This is tested on a computer with decent hardware and Firefox 79.0.

@lastzero
Copy link
Member

It will probably be faster in Chrome and also depends on the hardware.

In general, we expected this to become an issue as the VueJS templates contain a lot of logic now due to added features. Might be faster if you use the Mosaic View as it contains less information.

To improve this, we need to take a closer look at the templates and optimize them (help welcome). It would be much faster to do this via pure CSS instead of Vuetify / VueJS, just wasn't a major issue so far.

@lastzero lastzero added enhancement Optimization, improvement or maintenance task help wanted Well suited for external contributors! labels Aug 21, 2020
@wiwie wiwie changed the title Lag when selection pictures Lag when selecting pictures Aug 22, 2020
@geirgp
Copy link

geirgp commented Oct 11, 2020

I experience the same lag. decent Macbook Pro, latest Chrome.

@lastzero
Copy link
Member

How many pictures shown / selected? As mentioned, it can and should be optimized.... Just didn't have time for this yet and apparently no one else :/

@geirgp
Copy link

geirgp commented Oct 15, 2020

I endend up selecting 110 photos, and scrolled through 1500-ish in that selection process.

@lastzero
Copy link
Member

Which one was it? List (table), mosaic (just previews) or cards (with details)?

@make-github-pseudonymous-again

@lastzero I looked at the implementation of Clipboard just as a sanity check. Does not look like the problem comes from there. More likely to come from the UI code as already noted. I left some comments here for some potential improvement of the code style of Clipboard.

@lastzero
Copy link
Member

Thanks for taking a look at this! I agree, even arrays containing 1000 items should not cause this kind of lag.

We use lots of conditional rendering (v-if, v-else-if, v-else) in our VueJS templates though. So when the selection state changes, VueJS might re-render all results. Instead, it should only refresh the selected picture. The difficult part is not to introduce (sometimes subtle) bugs with optimizations.

@make-github-pseudonymous-again

@lastzero Could you point me to the relevant files?

@lastzero
Copy link
Member

lastzero commented Oct 26, 2020

See:

I expect the component/photo/cards.vue view to be the slowest as it contains the most logic and thus causes the most overhead when re-rendering. Selecting pictures was much faster in the past. Since then we've added a ton of features like video & live photo support.

@make-github-pseudonymous-again

I did a quick performance recording on the default gallery view (mosaic I guess) after scrolling down a bit. I observe a lot of GC trashing. Vue probably has no way to understand what is going on inside Clipboard ($clipboard is not a prop). Once selection is updated, the entire list has to be recomputed (because render depends on $clipboard.has(...) calls). Replacing this.selection by a Set as suggested here won't solve the issue unless Vue is able to do some clever set diffing...

Something that may have a better chance of working could be having some sort of internal selected state for each card, decoupling the global Clipboard from that state, so that the list does not depend on a selection prop. Of course now it is ugly because the selection state is spread all over. If you do not want to decouple, I think one should precompute $clipboard.has(photo) for each card and have a component for individual cards so that Vue can more quickly tell which cards need to be rerendered. It would still imply recomputing something for each item, but maybe much faster so that it would no become a problem unless you load a really large collection.

Additionally, having a scrolling "pagination" would help: with a state looking like numberOfphotosBeforeDisplayedPhotos, displayedPhotos, numberOfphotosAfterDisplayedPhotos, a component to display empty space for before and after that only depends on the number of items to be faked, and a component for displaying displayedPhotos, I am sure this exists already somewhere?

Any other idea?

@lastzero
Copy link
Member

Interesting, might come up with a replacement for $clipboard.has() when I have time to think about it. Of course you're more than welcome to give it a try first!

In general, the code seems easier to understand and maintain when the logic is encapsulated and independent of the view - but not at any price. Without VueJS, I'd probably use simple CSS classes for setting and getting the state.

PhotoPrism intentionally doesn't know the exact number of results:

  • First, because the number is continuously changing while the indexer is running and/or other people are using the application at the same time.
  • Second, because the database returns results faster when it only needs to fetch the first rows instead of computing the complete result set just to come up with a number that might change the next second.
  • Last but not least, files are typically merged to photo stacks when they are shown in the UI. So the number of database rows is not the the same as the number of photos.

@make-github-pseudonymous-again

Interesting, might come up with a replacement for $clipboard.has() when I have time to think about it. Of course you're more than welcome to give it a try first!

First I can try to rewrite $clipboard using a Set if you think this will not break things. I will need to understand how I can run the test suite for the whole thing.

In general, the code seems easier to understand and maintain when the logic is encapsulated and independent of the view - but not at any price. Without VueJS, I'd probably use simple CSS classes for setting and getting the state.

Maybe the way selection is displayed is flawed anyway: with a large number of selected items the only exact information that can be extracted from the UI is selection.length in the bottom right corner. A more robust way to implement selection would be to have a preview of the selection somewhere which would be less prone to the kind of performance issues that are encountered here (since we only need to iterate over this.selection). Then there is no need to display a selected toggle on each gallery card... until the size of the selection becomes too large to inspect what is included... Probably both solutions should coexist but somehow the complexity of render should only depend on what can be shown on the screen, it should not depend on things outside of the viewport (I believe the default browser tab memory limit is proportional to the number of viewport pixels). See also below.

PhotoPrism intentionally doesn't know the exact number of results:

numberOfphotosBeforeDisplayedPhotos and numberOfphotosBeforeDisplayedPhotos do not have to be database row counts, really anything that reflects how much stuff has been loaded in the UI. Since each item has known dimensions, a placeholder can be rendered instead of a card that would depend on the selection state. Of course this placeholder could be identical to a normal, but ignoring the selection state. I am just describing some sort of "infinite scrolling" as defined in #500 (comment). A first implementation would just keep all metadata in cache but evict thumbnails of things that are far away from the currently displayed stuff, if thumbnails take up most of the memory.

@make-github-pseudonymous-again

First I can try to rewrite $clipboard using a Set if you think this will not break things. I will need to understand how I can run the test suite for the whole thing.

This seems like a bad idea because Vue does not support Set and Map props: vuejs/vue#2410 (comment)?

@make-github-pseudonymous-again

I did a quick performance recording on the default gallery view (mosaic I guess) after scrolling down a bit. I observe a lot of GC trashing. Vue probably has no way to understand what is going on inside Clipboard ($clipboard is not a prop). Once selection is updated, the entire list has to be recomputed (because render depends on $clipboard.has(...) calls). Replacing this.selection by a Set as suggested here won't solve the issue unless Vue is able to do some clever set diffing...

I am not sure now. In Vue, is it only props reactive updates that trigger renders? In that case I am not sure what is forcing the rerender. Certainly the selection.length check before each $clipboard.has(...) should trigger a render when going from 0 to 1. Not sure what happens once it is positive and stays positive. Was the selection.length check added for performance reasons?

@make-github-pseudonymous-again
Copy link

Was the selection.length check added for performance reasons?

I see, if I remove all mentions of selection.length then the view does not get updated :).

@make-github-pseudonymous-again

I have a proof of concept that fixes the issue for component/photo/mosaic.vue. Should I start a PR draft?

@make-github-pseudonymous-again

If you do not want to decouple, I think one should precompute $clipboard.has(photo) for each card and have a component for individual cards so that Vue can more quickly tell which cards need to be rerendered. It would still imply recomputing something for each item, but maybe much faster so that it would no become a problem unless you load a really large collection.

This is what I implemented.

make-github-pseudonymous-again added a commit to aureooms-contrib/photoprism that referenced this issue Oct 27, 2020
Achieved by factoring `tile.vue` out of `mosaic.vue` allowing to isolate
its state from the parent state.

See photoprism#477.
@lastzero
Copy link
Member

Probably won't be able to review your suggestions before the weekend... looking forward to your PR / draft... as mentioned, changes should be kept to a minimum as a major refactoring will likely introduce new issues and at least requires much more testing.

@make-github-pseudonymous-again

Probably won't be able to review your suggestions before the weekend... looking forward to your PR / draft... as mentioned, changes should be kept to a minimum as a major refactoring will likely introduce new issues and at least requires much more testing.

The changes I have introduced so far only fix the problem for component/photo/mosaic.vue. It does so by factoring out the v-flex element to a new component component/photo/tile.vue. To fix the performance everywhere requires similar changes to:

I will limit the changes to what is needed to fix the issue raised in this thread. But I think that it would make sense in the future to refactor to share logic between component and share.

@lastzero
Copy link
Member

Using separate components for result container & items caused issues, but I don't remember what exactly. That's why there is only one component for each view type right now.

@lastzero
Copy link
Member

Maybe the items didn't react anymore when the backend sent updated metadata.

@lastzero
Copy link
Member

As a developer, I would very much refactor the frontend further and continue working on sharing (which includes finding the right abstraction before implementing it). But we need to make a cut somewhere. The overall quality is good enough by any standard and users are waiting for a release.

@make-github-pseudonymous-again

Maybe the items didn't react anymore when the backend sent updated metadata.

Is there a regression test for this?

@lastzero
Copy link
Member

@graciousgrey maintains our acceptance tests... would be difficult to implement and a small project by itself.

@make-github-pseudonymous-again

Maybe the items didn't react anymore when the backend sent updated metadata.

Let's say you have a component that renders something related to a photo by receiving a Photo instance as a prop with key photo. If I understand Vue reactivity correctly (v2), you need the photo prop to change to trigger a render (meaning photo must be a new instance of Photo when, for instance, the metadata changes), or you can avoid creating a new Photo instance and changing the photo prop by instead updating some of the properties of the existing instance which you watch with a deep watcher. Am I correct?

@lastzero lastzero changed the title Lag when selecting pictures UX: Lag when selecting pictures Dec 31, 2020
@lastzero lastzero added the frontend Requires experience with HTML/JS/CSS label Dec 31, 2020
lastzero added a commit that referenced this issue Jan 9, 2021
lastzero added a commit that referenced this issue Jan 9, 2021
@lastzero lastzero added this to Testing 🐳 in Roadmap 🚀✨ Jan 10, 2021
@lastzero lastzero added the please-test Ready for acceptance test label Jan 10, 2021
@lastzero lastzero self-assigned this Jan 10, 2021
@wiwie
Copy link
Author

wiwie commented Jan 19, 2021

I can confirm that it has significantly improved.

@wiwie wiwie closed this as completed Jan 19, 2021
Roadmap 🚀✨ automation moved this from Preview 🐳 to Released 🌈 Jan 19, 2021
@wiwie wiwie reopened this Jan 19, 2021
@wiwie
Copy link
Author

wiwie commented Jan 19, 2021

reopened, so you can decide when to close the issue.

@lastzero
Copy link
Member

Already released! Thanks for testing ❤️

@efffghhbh
Copy link

efffghhbh commented Jan 19, 2021 via email

@lastzero lastzero removed the please-test Ready for acceptance test label Jan 19, 2021
@lastzero lastzero added the released Available in the stable release label Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Optimization, improvement or maintenance task frontend Requires experience with HTML/JS/CSS help wanted Well suited for external contributors! released Available in the stable release
Projects
Roadmap 🚀✨
Released 🌈
Development

Successfully merging a pull request may close this issue.

5 participants