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(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

kahlstrm
Copy link

@kahlstrm kahlstrm commented Jun 27, 2024

What?

  • Reduces redundant image optimizations if the upstream image is unchanged, there is no need to run the optimization again as you can just use the previously optimized image.
  • Changes getHash-function to return base64url instead of custom variant of standard base64. This can be removed if deemed not necessary.
  • Moves all image ETag calculation logic (and introduce getImageEtag-function to do it) to happen inside image-optimizer for better consistency.

Why?

Currently when an image is requested and it becomes stale, the server will trigger a full image optimization for that image, using a significant spike in CPU-usage for routes that have multiple images (e.g. an image carousel).

How?

By calculating and storing the upstream etag in the cache entry, we can utilize the previously cached entry to check if the upstream image has remained the same, making it possible to reuse the previously cached entry again, as the image optimization would produce the same results anyways.

@ijjk
Copy link
Member

ijjk commented Jun 27, 2024

Allow CI Workflow Run

  • approve CI run for commit: a1f627d

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

// See https://en.wikipedia.org/wiki/Base64#Filenames
return hash.digest('base64').replace(/\//g, '-')
// See https://en.wikipedia.org/wiki/Base64#URL_applications
return hash.digest('base64url')
Copy link
Author

Choose a reason for hiding this comment

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

Not necessary for the PR, but while the CACHE_VERSION is anyways changing, it doesn't hurt? 🤷

isStatic: boolean,
xCache: XCacheHeader,
imagesConfig: ImageConfigComplete,
maxAge: number,
isDev: boolean
) {
const contentType = getContentType(extension)
const etag = getHash([buffer])
Copy link
Author

Choose a reason for hiding this comment

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

If we are storing the etag already, shouldn't we use that instead of recalculating the etag? This can be reverted to use getImageTag.

@kahlstrm kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch from bda2f1a to a1f627d Compare June 29, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants