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

API: Add explicit cache headers #822

Closed
benmccann opened this issue Jan 6, 2021 · 14 comments
Closed

API: Add explicit cache headers #822

benmccann opened this issue Jan 6, 2021 · 14 comments
Assignees
Labels
enhancement Optimization, improvement or maintenance task priority Supported by early sponsors or popular demand released Available in the stable release

Comments

@benmccann
Copy link
Contributor

https://demo.photoprism.org loads slower than I would expect even though I've been there before.

It takes a few seconds to download https://demo.photoprism.org/static/build/app.js?1c8b098e and that file has cache-control: no-cache and pragma: no-cache headers. I'm assuming that the ?1c8b098e is a unique hash of the file to bust the cache when the file contents change (I'm not quite sure why it's using a query string as opposed to putting the hash in the filename which is the webpack default). If this is true, and all files under /static/ are served with a unique hash, then it should be fairly straight forward to set a long-lived cache expiry header, which would greatly speed up future visits.

Some of the API responses could probably also be served with caching enabled. E.g. I noticed that page requests https://demo.photoprism.org/api/v1/t/c1ebffbb6a607cd8464b67751f643390444d6a2e/4ff58586/tile_500. I'm not that familiar with the API, but as long as the API includes a hash of the file contents (and size of the image to be served if that would impact the results) then it should also be able to be cached which would greatly speed things up as well.

@lastzero
Copy link
Member

lastzero commented Jan 6, 2021

From what I know, images should be cached by browsers already - if you say that is not the case, I'll take a look.

@lastzero lastzero self-assigned this Jan 6, 2021
@lastzero lastzero added the enhancement Optimization, improvement or maintenance task label Jan 6, 2021
@graciousgrey graciousgrey added the priority Supported by early sponsors or popular demand label Jan 7, 2021
lastzero added a commit that referenced this issue Jan 7, 2021
Signed-off-by: Michael Mayer <michael@liquidbytes.net>
@lastzero
Copy link
Member

lastzero commented Jan 7, 2021

May I ask where you see those cache-control: no-cache and pragma: no-cache headers? I don't see them:

Screenshot 2021-01-07 at 22 44 49

Based on the general speed and what dev tools show, caching works as expected (all browsers I know cache GET requests by default):

Screenshot 2021-01-07 at 22 47 55

However, I've been adding cache control headers to our API for thumbs. I may also add them to other assets, but don't quite understand where your problems are coming from as we don't have any... in addition, preview tokens can be found in the headers when they may be needed, e.g. when querying the search API. This should provide a smoother dev & user experience.

However, we now need to think about the max age of thumbs as we set explicit cache headers:

c.Header("Cache-Control", fmt.Sprintf("private, max-age=%s, no-transform", maxAge))

Google is using 24 hours aka 86400 seconds - seems a bit short to me, but maybe there is a reason? 👀

@benmccann
Copy link
Contributor Author

Gahh. It turns out I had "Disable cache" checked in DevTools from debugging another site quite awhile ago. I didn't realize that I had it on. So I think there may not have been an issue here at all :-/

May I ask where you see those cache-control: no-cache and pragma: no-cache headers? I don't see them

Doh. Sorry. Those were under request headers - not response headers, so ignore that

The caching looks good to me on the demo site. The only suggestion I'd have on this is whether we could also set immutable in the Cache-Control header. And I do agree that 86400 seconds seems short

c.Header("Cache-Control", fmt.Sprintf("private, max-age=%s, no-transform", maxAge))

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

Added immutable, but only for file thumbs - album / folder covers may indeed change very often, see #807

That's the actual issue I wanted to solve before I got into this 😆

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

Approved?

headers

@lastzero lastzero added the please-test Ready for acceptance test label Jan 8, 2021
@lastzero lastzero changed the title Asset caching API: Add explicit cache headers Jan 8, 2021
@benmccann
Copy link
Contributor Author

benmccann commented Jan 8, 2021

Thanks so much for all your help!! This all looks good for the main pages!

I actually have one idea for the album cover. It looks like we fetch the album covers with a URL like:
https://demo.photoprism.org/api/v1/albums/aqmmcx925g83qib9/t/public/tile_500

This same image is also available from:
https://demo.photoprism.org/api/v1/t/de1aa5c5a3432a7ec3487cd0c5fdda5b022f38b5/public/tile_500

If the https://demo.photoprism.org/api/v1/albums?count=24&offset=0&q=&category=&type=month request returned the thumbnail ID then we could use this second thumbnail API with the more aggressive cache and probably immediately see updates to the album cover without having to wait 1hr because the album API would return a new thumbnail ID. This also would make loading the album page much faster the first time because the user has probably already loaded the image from the thumbnail API on one of the main pages and it would already be cached whereas the album thumbnail API is a different URL so we'd have less cache reuse

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

Actually wanted to implement the service worker next 😂

Caching issues would remain as the cover query is too heavy to run it for every GET albums request, that's why it's implemented this way right now.

@benmccann
Copy link
Contributor Author

You could still cache the thumbnail ID on the server. That would remove a layer of caching so that you know any caching is happening on the server - then if someone wanted to clear the cache they'd only have to do it on the server instead of the browser as well as the server.

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

Our plan was to render special album covers, so that we can not use a single pre-rendered image thumbnail. It's just not done yet. Last but not least, we should slowly start working on batch editing / facial recognition, and don't invest too much time in "nice to have" optimizations at this point.

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

There should already be a Cover UID column in the database table, by the way. It's just not populated right now, will be eventually. I suggest doing this later as there are more than enough other issues waiting.

@benmccann
Copy link
Contributor Author

I'm curious what the special album cover would be?

I did notice CoverUID in the JSON response was empty. I guess that explains it 😄

Anyway, I'll go ahead and close this as I think it's done now. Thanks again!!!

@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

@lastzero lastzero added released Available in the stable release and removed please-test Ready for acceptance test labels Jan 8, 2021
@domharrington
Copy link

Is this still working as intended? I can no longer see "cache-control: immutable" on any of the /tile endpoints:

$ curl -I https://demo-cdn.photoprism.org/api/v1/t/e296654221cb9badc0de201874cc4c816fdd8b24/public/tile_224 --silent | grep cache-control
cache-control: no-cache

The code is still there:

// AddCacheHeader adds thumbnail cache control headers to the response.
func AddThumbCacheHeader(c *gin.Context) {
c.Header("Cache-Control", fmt.Sprintf("private, max-age=%s, no-transform, immutable", ThumbCacheTTL.String()))
}

And still appears to be getting called from some code paths:

AddThumbCacheHeader(c)

On my instance (which is sat behind Cloudflare) I've added some page rules to force cache the thumb endpoints and performance is greatly improved.

image

Am I missing something?

@lastzero
Copy link
Member

@domharrington Thank you very much! I wish we had seen this earlier. The headers were missing in one place, which may have been the most common execution path. Note that the headers that KeyCDN sends/uses for our demo are out of our control. Started a new Development Preview build for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Optimization, improvement or maintenance task priority Supported by early sponsors or popular demand released Available in the stable release
Projects
Status: Release 🌈
Development

No branches or pull requests

4 participants