-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Performance: Prevent loading already loaded images on first photo-open #3435
base: develop
Are you sure you want to change the base?
Conversation
I've decided to request the list of pictures from the backend as dynamically computing the list in the format for the viewer proofed to be slower in many cases, especially when many pictures are loaded and on slower devices. |
When the pictures are loaded from the backend, watchers are placed on all results, which is way slower than converting the existing pictures to thumbs. My girlfriend sometimes scrolls through years of images, and nearly every time when she clicks one, the app crashes, presumably because of this memory increase (have not 100% tested it though). The only case where the static show function is better is when (without scrolling to far inbetween) a second picture is opened. In that case, nothing would be loaded or converted. If converting all images to thumbs (which is faster than converting a new result, but slower than not converting anything) is to slow, a reasonable solution could be to create a Thumb-instance whenever a Picture-Instance is created, and use these whenever the viewer is openend |
Or maybe we could teach the viewer to work with |
Thinking about it... We can currently generate a Now the viewer probably expects certain properties to exist in the "thumbs" it works with. I'll later try to find out what properties of a Thumb are required for PhotoSwipe and the Viewer. Adding them to the |
@lastzero As it turns out, the only things the viewer actually needs are the properties Instead of loading Thumbs-Info from the backend or converting What do you think? |
From what I remember, it was the While PhotoSwipe could be refactored to work with our own data structure (so the conversion isn't needed), implementing a new viewer is planned anyway: As for the watchers, I was looking for a solution, but probably didn't find one in the time available. Since real-world testing still showed a significant improvement for users who had previously reported problems, we decided to release the viewer in this form and wait for the new viewer before introducing further improvements. Edit: Another catch, if you touch this code, is to sync the results with the server AND the search result displayed in the UI. Note that I originally came up with this solution (at least I think so) when I was optimizing the performance of the viewer in Places, since that view currently has all the pictures loaded, but not with all the details that the viewer needs. So this solved both the performance issues and the missing data. |
I'll look into the potential problems you mentioned regarding syncing in general and performance in the places-view (not right now, but it's now on my radar) |
It could also be that the viewer is used in other places where the photo metadata is incomplete or in an incompatible format. By loading the pictures for the viewer from the server, we ultimately ensure that they are complete and in the right format without having to convert anything in the browser or adding wrappers, depending on the context. So before we change anything in the production code, it's important to understand what impact this might have on complexity and bug regressions in addition to potential performance benefits. |
@lastzero A couple thoughts and observations regarding your (valid) concerns
The only thing i actually changed is how the
the
Valid concern. One major change in this PR is indeed, that the Photo-Model now also calculates the tumbnail-urls. I should measure how much of an impact that has when opening (for example) a large album.
Can you provide me with a potential usecase where this happens? Are there any cases where the viewer (PhotoSwipe) can cause a change of data (like the title)? If not, my logic regarding this situation is as follows:
Does that make sense? If all my assumptions are correct, the key takeaway is: |
I tested the performance impact of having the Photo-Model calculate the thumbnail-urls in its constructor. The tested browsers were:
Here are the results:
I think this is acceptable considering that it enables us to not load possibly thousand of full thumbnail-models on photo-open. What do you think @lastzero? |
@lastzero do you need more information on this? |
This change prevents seemingly unnecessary, potentially huge requests to load photos whenever a photo is opened for the first time in a new batch.
Or in other words:
This is unnecessary because another option to show the viewer with already loaded results exists and is even used in some scenarios.
There are currently two
show
methods on the Photo viewer. one static, one not.The static one checks if a viewer already exists in the context and if so, checks if it has enough results to show the requested image.
If not, it basically loads all photos from the backend again, that the current view is already displaying.
The other show method accepts a list of
thumbsThumbs or Photos to display,which can be locally generated from already loaded photos. That. Providing the Photos directly is much faster, and i don't know why the static function exists.Acceptance Criteria: