Performance: Prevent loading already loaded images on first photo-open

[heikomat]

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:

• scroll for > 1 batchsize
• open a photo
• all photos up to that image are re-requested
• scroll for > 1 batchsize again
• open another photo
• all photos up to that image are re-requested

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 thumbs Thumbs 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:

• Features and enhancements must be fully implemented so that they can be released at any time without additional work
• Automated unit and/or acceptance tests are mandatory to ensure the changes work as expected and to reduce repetitive manual work
• Frontend components must be responsive to work and look properly on phones, tablets, and desktop computers; you must have tested them on all major browsers and different devices

[lastzero]

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.

[heikomat]

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.
When a user has scrolled ~2k images, then opening a picture will load the infos of 2k images and add watchers to all of them, resulting in a ~2MB download and a huge memory increase.

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

[heikomat]

Or maybe we could teach the viewer to work with Thumb instances and Photo instances, so we wouldn't have to generate thumbs at all

[heikomat]

Thinking about it... We can currently generate a Thumb instance completely from a Photo instance.
That means all Info to use a thumb is in there.

Now the viewer probably expects certain properties to exist in the "thumbs" it works with.
If these properties happen to also exist on a Photo, nothing would prevent it from working with Photos instead, without converting them. (or in other words: the Photo class should "implement" whatever type is required to be used in PhotoSwipe)

I'll later try to find out what properties of a Thumb are required for PhotoSwipe and the Viewer. Adding them to the Photo class would make it so nothing needs to get loaded or converted at all

[heikomat]

@lastzero As it turns out, the only things the viewer actually needs are the properties Thumbs and DownloadUrl.
You might want to take a look at the latest commit. I added these two properties to the Photo model.

Instead of loading Thumbs-Info from the backend or converting Photos to Thumbs on click i now just directly pass the already loaded Photos to the viewer. No loading or converting required.

What do you think?

[lastzero]

From what I remember, it was the generateThumbs() function that caused a significant slowdown when testing with large result sets.

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:

• UX: Hybrid Photo and Video Viewer #1307

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.

[heikomat]

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)

[lastzero]

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.

[heikomat]

@lastzero A couple thoughts and observations regarding your (valid) concerns

It could also be that the viewer is used in other places where the photo metadata is incomplete or in an incompatible format.

The only thing i actually changed is how the photos-page and the album/photos-page use the viewer. After that i deleted the static show method because it was not referenced anywhere in the code anymore.

• Other places that use the viewer are untouched
• absolutely no functionality of the viewer was changed, only unused code removed (0 adds)
• It therefore follows (afaik) that no other code that uses the viewer other than photos.vue and album/photos.vue can be affected.

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

the places.vue is unchanged. It still calls the geo/view-route and uses Thumb.wrap to display thumbs in the viewer. Haven't tested it, but i don't see how the places-view could possibly be affected by this change, considering that it doesn't depend on photos.vue or albums/photos.vue

From what I remember, it was the generateThumbs() function that caused a significant slowdown when testing with large result sets.

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.
If we've done things right, the creation of a couple of urls shouldn't be noticable performance wise. Especially considering that a lot of the methods of the photo-model are memoized.

Another catch, if you touch this code, is to sync the results with the server AND the search result displayed in the UI

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:

• The only thing changed is in some cases where the viewer (photoswipe) get's its data from (Photo instead of Thumb)
• The Viewer (presumably) can't cause a change to the data, so the only relevant sync-scenario is how the viewer gets updated if the data is changed from somewhere else
• The Photo-Models should probably already be kept up-to-date, just like the Thumb-models, so for the viewer that should not make a difference

Does that make sense?

If all my assumptions are correct, the key takeaway is:
photos.vue and album/photos.vue and the performance of creating many Photo-Model-instances should be tested, but everything else should be unchanged

[heikomat]

I tested the performance impact of having the Photo-Model calculate the thumbnail-urls in its constructor.

The tested browsers were:

1. chrome on a 2020 i7 macbook pro
2. chrome on a google Pixel 7 smartphone

Here are the results:

device	slowdown	average time to calculate thumbnail-urls for 1000 photos
macbook	6x	~65ms
macbook	none	6-13ms
smartphone	6x	119ms
smartphone	none	18ms

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?

[heikomat]

@lastzero do you need more information on this?
Anything i can help with here?