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

feat: prefetch images (avatars, media, and external thumbnails) in feeds and threads #2196

Closed
wants to merge 17 commits into from

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented Dec 12, 2023

Currently, images are not loaded until the individual post comes into view. This results in a pretty subpar experience, especially with a degraded connection:

1zVPEJt.mp4

Instead, it is best to prefetch images before they come into the view, and we get this result:

1TJlTQW.1.mp4

Originally, I intended to just prefetch all of the images in the query. However, this seems like a subpar solution. If we load a feed of posts that each have two images, and avatar, etc. we might end up fetching more than we need (i.e. user just goes directly to the first post in the feed).

Instead, the solution is to fetch the images as we scroll. This requires the use of onVieableItemsChanged which is a bit funky. Here's briefly what I did:

  1. Bump expo-image to 1.8.1. There are a fair number of performance benefits in upgrading. The same functionality should be available in the version used right now, however (from what I can see, you all might know of specific issues in your use case) there shouldn't be any downside to upgrading. I can revert that though if necessary. Looks like you're upgrading to 1.10 🎉
  2. Set cachePolicy to memory-disk for avatars and AutoSizedImage. Prefetch caches both to the memory and the disk, so the optimal setting here would be to load first from the memory.
  3. Add a usePrefetchListImages hook. Easy to reuse in both threads and feeds. I believe that the main cases where this is needed are in those two places. I suppose there would be benefit in making it work with user lists too, though avatars shouldn't need to be if they end up being smaller.

We cannot change the value of onViewableItemsChanged after rendering, so we cannot use useCallback with a feedItems (or whatever) dep array. Instead, we have to create a ref that stores the items. I'll highlight where that happens.

I'm pretty unfamiliar with the codebase (or atproto in general) so I'm not sure if everything is typed exactly how it should be or not. Throw whatever feedback at me and I'll clean it up.

@@ -171,6 +171,8 @@ export function UserAvatar({
contentFit="cover"
source={{uri: avatar}}
blurRadius={moderation?.blur ? BLUR_AMOUNT : 0}
cachePolicy="memory-disk"
recyclingKey={avatar}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a recycling key here too although you guys may have ended up doing that already?

@@ -181,8 +188,10 @@ let Feed = ({
} else {
arr.push(LOADING_ITEM)
}

setItems(arr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the items here. (can't use a useCallback)

@@ -192,12 +201,13 @@ let Feed = ({
setIsPTRing(true)
try {
await refetch()
onRefreshPrefetch()
Copy link
Contributor Author

@haileyok haileyok Dec 13, 2023

Choose a reason for hiding this comment

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

this resets the last index to zero so we can start fetching from the top again. not a fan of that name, will change.

@haileyok
Copy link
Contributor Author

haileyok commented Dec 13, 2023

As a side note, (and you may have already noticed this on your own) caching to memory appears to have significant memory usage in the simulator. That's a product of the dev client though. Profiling a production build results in only marginally higher memory usage (SDWebImage and Glide both take care of this) from my experience elsewhere. Since I'm curious, I'll look and see how this affects this app at all later.

@haileyok haileyok marked this pull request as ready for review December 13, 2023 06:04
@haileyok
Copy link
Contributor Author

Tested production builds on both iOS and Android (mid-range) without issues. Also went ahead and disabled this functionality on web since it isn't necessary.

@pfrazee
Copy link
Collaborator

pfrazee commented Dec 13, 2023

This is pretty great work. I need to find time for more extensive QA and review but I wanted to drop a 🙌

@haileyok
Copy link
Contributor Author

Additional fix so we don't needless attempt to prefetch the already visible items when we first load, as well as make sure we don't miss any posts (always forget that slice is noninclusive of the end...)

@pfrazee pfrazee self-assigned this Dec 19, 2023
@pfrazee pfrazee requested a review from gaearon December 20, 2023 21:27
@pfrazee
Copy link
Collaborator

pfrazee commented Dec 20, 2023

I'm going to let @gaearon review this because he's DRI on FlatList behaviors right now, and obviously more expert at evaluating the performance implications of react systems.

@pfrazee pfrazee assigned gaearon and unassigned pfrazee Jan 4, 2024
@pfrazee pfrazee added the improvement Not quite a feature but improves existing behavior label Jan 8, 2024
@haileyok haileyok closed this Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Not quite a feature but improves existing behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants