MozillaFoundation / foundation.mozilla.org

Mozilla Foundation website
https://foundation.mozilla.org
Mozilla Public License 2.0
389 stars 153 forks source link

Consider reducing the thumbnail images we serve on the PNI imagepage #6395

Closed Pomax closed 3 years ago

Pomax commented 3 years ago

Summary

The PNI homepage uses 8 different sized thumbnails for product cards, and with 136 products that means 1088 database queries to get all our images, and that is about 1000 too much (normally, hyperbole, but in this case, pretty much literally 1000...) causing the page to time out because wagtail has to resolve each of those to real files before it send the page HTML to the user.

We're looking at turning this into something manageable, and the most immediate win would be to only use a single x1 and a single x2 size for PNI thumbnails, set to something that works for all sizes (probably: 360x360 for x1, 720x720 for x2).

@kristinashu would this be something we could do, or would there be objections for reasons we didn't think of?

Backlog From slack [pomax] @kristinashu @KalobTaulien soooo we have an interesting problem relating to image sizes [pomax] basically we have so many different source sizes for the homepage/category page view that wagtail is hitting its performance limit just trying to service all the [source] instructions =) [pomax] would it be possible to reduce the number of resolutions we declare in https://github.com/mozilla/foundation.mozilla.org/blob/master/network-api/networkapi/buyersguide/templates/buyersguide/catalog.html#L82-L135 ? [pomax] (the problem being that wagtail apparently sees an {% image ... %} instruction, queries the database, then sees a new {% image ... }% instruction, queries the database again, eight times for each product, 136 products on the homepage = that's a lot of database work before we can even render the page for the user =S) (edited) [ben] Is it possible to move this logic into the view so we can group the db queries and pass them into the template via context? [pomax] there's no view function like in plain Django, so not as easily as you would hope, but kalob did mention an interesting idea around a new template tag that can batch a whole bunch of different sizes as a single operation. [pomax] even so, reading through this <picture> set, I'm a little puzzles, since all these sizes are very similar [pomax] but also the smaller the viewport, the large an image we serve? unless I'm reading this wrong. It looks like on desktop, it's 260x260 / 520x520 but on mobile it's 360x360 / 720x720 [Kalob Taulien] Its because the bootstrap grid kicks in and makes each image wider at a certain point, and then smaller again when the breakpoints kick in. That threw me off at first too [pomax] just a single intermediate size to cover all sizes feels like it might solve a lot of our problems here, too. [Kalob Taulien] It would.. But we lose accessibility on more devices the more we cut down, but we would get our page back to a loading state at least. [pomax] 360x360/720x720 is still pretty small. [pomax] I was more thinking "if we already serve 'large' for mobile right now, no one loses if we just serve large to everyone" (edited) [Kalob Taulien] Depends on our audience. Most people view sites from their phones, if they have lower bandwidth, slower speed, etc. a smaller image is better for them, but worse for us. [pomax] no but I mean... we are sending them the biggest image right now [pomax] anything 415px to 768px is getting the 360/720 combo [Kalob Taulien] Browsers will only ask for the right one for the device though. We might send them the DOM elements, but it wont make a request unless the device needs it as far as im aware [Kalob Taulien] (That said I use a Pixel 2.. and 2014 is calling me.. so I dont really know) [ben] @KalobTaulien this sounds like a problem that the rest of the wagtail community faces as image srcsets and pictures are pretty common, I imagine? Any tips from the wider wagtail community around how to not make renditions take forever? [pomax] I suspect not a lot of folks show 136 images on a single page though [Kalob Taulien] In fairness, most places either serve images poorly (a single large image) or they serve a handful of images per page rather than hundreds of images all at once with 9 rendition sizes. Because this problem has become so painful though, Im definitely getting close to throwing in the towel and just saying “lets serve 2 image sizes for each product” [pomax] you'd use lazy loading of some sort [Kalob Taulien] definitely ^ [pomax] is there a some prebuilt wagtail image lazyload solution? [ben] @pomax lazy loading on the client side? [pomax] because I'd imagine it would have to ask wagtail for a sized image, which it can't do with the template tag [pomax] @benhohner yeah [Kalob Taulien] We have an image url generator feature in core. Not that same thing we’re looking for though [ben] Ah, I mean, that would speed up client side loading, but would it improve the rendition render time? [Kalob Taulien] We need images to lazily load still. Ok, my sanity check time: What would be the worst to happen if we went back to a single image per product? [pomax] for instance, we do this for blogs: you get 12 (I think?) blog cards, if you want to see more, you need to click "see more". [pomax] but we can't do that for PNI [pomax] at least not without some really clever rewriting to make sure creepiness averages still do the right thing, especially when the user clicks "back" from a product [Kalob Taulien] Yeah. We’d need proper search, not just filtering. Which is a big task [pomax] sanity check wise: the worst that would happen is that the amount of data causes higher charges for our mobile users. [ben] @KalobTaulien for the all PNI home page or for an individual product page? For the home page it might be fine to just have 1x and 2x images, as the product images will always be pretty tiny? [pomax] however, the current pageload on the pni homepage is 2.2MB [Kalob Taulien] Surely its nowhere near streaming a short youtube video though. [pomax] which is "big but hardly outrageous" given modern bundle sizes on many websites [Kalob Taulien] Thats tiny for all the images we have! [pomax] they're just thumbnails [pomax] @benhohner renditions are based on what the template tries to show, so the homepage has 8 different sized thumbnails, but the product page only has a single image x1+x2 set [pomax] plus, a product page is literally a single product, so even 20 db calls would still be fine, compared to 8 x 136 on the PNI homepage [ben] Ah. Not sure why we have 8 different sized thumbnails for something where the images are approximately the same size on every screen width [ben] Let's get rid of em and just keep 1x and 2x? [pomax] let's get buyin from @kristinashu though [Kalob Taulien] Im in favour of that [Kalob Taulien] Both of those you both said* [pomax] I think it's worth doing, but I want to make sure design has input [Kalob Taulien] @benhohner It was because on certain mobile views the images become different sizes. This is 767px wide vs 768px wide — but is one large image is better than a properly sized image in terms of quality? ![image1](https://user-images.githubusercontent.com/177243/111548659-43c78680-8738-11eb-9852-51c03c640e5d.png) ![image2](https://user-images.githubusercontent.com/177243/111548664-44f8b380-8738-11eb-8fac-75479b90cfc3.png) [ben] Like, this is the max variance in size. We can definitely just serve the large one ![image3](https://user-images.githubusercontent.com/177243/111548672-47f3a400-8738-11eb-911a-759b2adeaefc.png) ![image4](https://user-images.githubusercontent.com/177243/111548676-4924d100-8738-11eb-903c-7fd22374eae3.png) [ben] Oh also my monitor is 2x [ben] so the 1x will be 1/4 that area [pomax] @KalobTaulien odd question: can image renditions be turned off? [pomax] e.g. make wagtail proxy the original size image with a resize-on-the-fly based on request url? [pomax] upside: no database calls, immediate template response. downside: wagtail actually services image requests, with overhead, but the overhead'd be "per image request" (cached by the browser as a normal image response) instead of "per page". (edited) [ben] @KalobTaulien haha, looks like we were doing the same thing at the same time. Anyway, I've gotta head off. But I totally approve getting rid of the 8 images sizes and just keeping a 1x and 2x per product where the 1x width is like ~343px. Also, yes, let's get Kristina's input before we pull the trigger [Kalob Taulien] @pomax As far as I know, no. We have a URL generator though which might maybe do the trick, Ill look into that in the morning! @benhohner Im in the same boat. Less is more in this case.
Pomax commented 3 years ago

Let's use only a single source set, with 360px for the 1x and 720px for the 2x thumb sizes. The page weight will be ~2MB, but that seems infinitely better than "server error 500" due to a rendering timeout, with rapid further improvements planned as per https://github.com/mozilla/foundation.mozilla.org/issues/6410

KalobTaulien commented 3 years ago

PR #6413 tackles this problem. Closing as this is a duplicate of https://github.com/mozilla/foundation.mozilla.org/issues/6378