Optimize LazyImage

[kean]

LazyImage currently uses FetchImage, which was not originally designed for it. Some performance improvements can be gained by switching to an ad-hoc ViewModel designed specifically for LazyImage. One of these improvements should include the synchronous memory cache lookup on the first body calculations to eliminate one body draw loop.

[cheif]

I think we're seeing an issue related to this.

We've got a fairly complex view that shows a half-screen modal-ish view animating in from the bottom, this view has a LazyImage in it. When running https://github.com/kean/Nuke/releases/tag/12.1.2 and earlier everything works correctly, but when upgrading to https://github.com/kean/Nuke/releases/tag/12.1.3 (or later), we get a glitchy animation on the Image. Basically the image just appears where it'll be at the end of the animation, and doesn't animate up with the rest of the elements.

By doing some printf-debugging I've seen that the difference is that in 12.1.3 the placeholder block is called once, since FetchImage.imageContainer is nil, while on 12.1.2 the placeholder is never called. I've drilled this down to this change: https://github.com/kean/Nuke/commit/fd1399c33b735272d2dc13cad7e07d4a850816b4

I've tried forking the library and manually calling the synchronous cache lookup part of load directly in the body of LazyImage, and this seems to solve our problem.

Unfortunately I'm not able to reproduce this in a simple example, I guess our complex-ish view also imacts this, but I guess the main issue is what you're describing here.

I'd be happy to help with diagnosing this further, or fixing this issue, but I'd probably need some (small) pointers on what would be an reasonable approach to specific LazyImage viewmodel. 😄

[kean]

I removed the sync cache lookup in 12.1.3 to fix https://github.com/kean/Nuke/issues/709. You can see what changed by looking at this diff. I over-corrected a bit, removing all optimizations because some were flaky. The goal of the original optimization was exactly to eliminate all body calls except for one in case the image is available in the memory cache.

Ideally, LazyImage needs its own ViewModel as FetchImage has not been optimized/tailored for it. But it's only some little details, really. It doesn't affect the user experience in any significant way.

As step one, I would bring back the memory cache optimization. I'm not sure how to do it safely and without adding redundant memory cache checks. Maybe I'm just not seeing it yet.

[urbaneewe]

Hey @kean I'm going to take this over if it's alright. Just wanted to check if you had any other advice regarding this?

If not I can start working on bringing back the memory cache optimisation and see how it goes from there, I may need some additional pointers after.

[kean]

Hey, @urbaneewe. If you'd like to kick this off, you are welcome to.

The two main goals are:

• Make sure there is only one body call when the image view is displayed for the first time, and it's available in the memory cache. Unfortunately, it's non-trivial in SwiftUI. I'm also unsure how important this actually is.
• Remove the dependency on FetchImage, which I'm planning to phase out, and have a dedicated ViewModel for LazyImage. This ViewModel should have the absolute minimum number of stored properties and emit the minimum number of updates to said properties (objectWillChange).

There are no other technical requirements. FetchImage can be used as a starting point. If you have any other questions, I'll be happy to help.

[urbaneewe]

Hey, @kean.

I've been at this for the past couple of days and I think I need some additional pointers.

Basically, whichever way I go about creating this new ViewModel it ends up looking the same as the already implemented FetchImage.

Do you have any suggestions as to which properties I should omit in the new ViewModel?

[kean]

Do you have any suggestions as to which properties I should omit in the new ViewModel?

I was planning to potentially consolidate some of them, but I haven't made any concrete plans.

One thing also worth considering is the new Observable system.

[steveriggins]

I think I am seeing these performance issues in Ice Cubes App for Mastodon. I've narrowed a VisionOS performance issue (many halts on the main thread) down to LazyImage. The app is using 12.4.0.

[kean]

I'm going to close it as it's an internal task and is tracked in Trello together with (many) other tasks.

am seeing these performance issues

It's highly unlikely to be related to this issue because the optimizations I'm talking about are really last-mile minor tuning.