[Paywalls V2] Prefetch low res images #4658
Conversation
| #if PAYWALL_COMPONENTS | ||
|
|
||
| private var allImagesInPaywallsV2: Set<URL> { | ||
| // Attempting to warm up all offerings for Paywalls V2. |
There was a problem hiding this comment.
| // Attempting to warm up all offerings for Paywalls V2. | |
| // Attempting to warm up all images for Paywalls V2. |
There was a problem hiding this comment.
Oh, this is actually not a typo but I could clarify...
Paywalls V1 only warmed up image for the default/current offering 😬 And I totally don't love that. But it had to be done because we didn't have:
- Low res images
- Explicit draft/published state
But with paywalls V2, we can load "all low res images for all offerings" (so I'll change the comment to that to be more clear)
There was a problem hiding this comment.
Ah, great! Thanks for clarifying!
| } | ||
|
|
||
| // swiftlint:disable:next cyclomatic_complexity | ||
| private func collectAllImageURLs(in stack: PaywallComponent.StackComponent) -> [URL] { |
There was a problem hiding this comment.
Traversing the tree of components is not the most performant, especially since we're already doing that to build the paywall. But I guess we need to do it in a separate traversal, because if we do it at the same time as building the paywall it's too late? (It wouldn't be prefetch anymore ha.)
There was a problem hiding this comment.
Yeaahhhh, I figured this could be improved in the future at some point if we wanted 🤷♂️ The thing this does differently than paywalls is that it doesn't load the entire view models and very localization stuff. Its pretty specific in how/what its traversing.
But tbh... precaching a bunch of images is LESS performant than traversing this free but its leading to better performance of the paywall 😛 🥴
There was a problem hiding this comment.
That's true, good perspective!
| for (_, localeValues) in self.componentsLocalizations { | ||
| for (_, value) in localeValues { | ||
| switch value { | ||
| case .string: | ||
| break | ||
| case .image(let image): | ||
| imageUrls += image.imageUrls | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This made me think: should all images be in componentsLocalizations, including the "base" images?
There was a problem hiding this comment.
I had a reason against putting the base image in localization at one point and it made sense so I'm going to say... no. But now I need to remember that good argument that I had 😛
But TBH, I did think the same thing as you while writing this code how that would be easier but... we'd have to do the same thing for icons anyway 🤷♂️
joshdholtz
force-pushed
the
paywalls-v2/prefetch-images
branch
from
55c8b91 to
4e723db
Compare
|
Nice attempt to prefetch images! If you want (and have perf metrics in place to measure), you can push even further than this. For example, your backend knows which images it's about to include in the response body. So it can collect them all and put them in a seperate list at the top level of the JSON. This way, the client won't need to traverse the model tree to collect them. Backend can even have a custom HTTP header just for this list. The client can then just grab the header and kick off the image requests. No need to wait for the body to be parsed or even fully transmitted (since headers are sent over first). And this optimization works on both iOS and Android (and possibly web though I can't recall). |
@nguyenhuy Hey, thanks for this! 😊 I did think of something similar a while back but definitely did not act on it 😅 But since you brought it up, it might actually be something we look into! I'm not sure if we have the capacity/priority to also make the backend change for this right now but... I think that's something we can add in soon ™️ and then just replace this logic. What do you think? cc: @JayShortway @tonidero This would be nice because less duplicated logic and we can control the prefetch logic a bit more on the backend. I think it would have to go to in the body of the offerings response right now (not the header) because of the way our caching works 🤷♂️ |
|
@joshdholtz @nguyenhuy I kinda love it! If we use a top-level dictionary, the image components in the tree can reference the keys from that dictionary. That way we can save a bit on bandwidth, as the JSON can already get quite big. |
|
I like it! It should indeed simplify the logic in the SDKs. I'm not sure if it would save much bandwidth unless we have a bunch of repeated images (which might be the case) but still, it should simplify the parsing. I guess it might increase the complexity in the backend a bit, depending on how the images are stored? Not sure about this part... But well, I'm all for it! |
Exactly. Backend can control which images and how many of them clients should prefetch (only main ones for example) and which resolution(s).
Yeap, that works too if network speed/consumption is a concern. |
|
A different but related question, I guess this will also need to support prefetching localized images if any? If we do it in the SDK, I guess that's more complexity that it needs to handle, that can be dynamically handled by the backend |
Backend has support for locale filtering (we just aren't using it yet int he SDK) so it once the SDK support that, it would just really do looking at any localized images 🤷♂️ But yeah, doing it on backend does still decrease that complexity in the SDK by quite a bit 😇 Will make some time to do that in the upcoming week 💪 |
joshdholtz
force-pushed
the
paywalls-v2/prefetch-images
branch
from
4e723db to
ee492f3
Compare
1 build increased size
Paywalls 1.0 (1)
|
| Item | Install Size Change |
|---|---|
| DYLD.String Table | ⬆️ 20.9 kB |
| Code Signature | ⬆️ 2.9 kB |
| Other | ⬆️ 80.3 kB |
View Treemap
🛸 Powered by Emerge Tools
Comment trigger: Size diff threshold of 100.00kB exceeded
Motivation
Prefetch low res images of all paywalls v2 paywalls
Description
Collects all the images in v2 paywalls for warming up cache