feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

kahlstrm · 2024-06-27T21:46:34Z

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 · 2024-06-27T21:47:00Z

Allow CI Workflow Run

approve CI run for commit: f406464

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

packages/next/src/server/image-optimizer.ts

kahlstrm · 2024-06-27T21:57:53Z

packages/next/src/server/image-optimizer.ts

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


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 · 2024-07-03T05:27:00Z

@styfle Hi, I hate to be that guy who randomly pings maintainers, but could you look at this to check if this is a wanted feature? For instance, our site becomes slow and unresponsive after a stale cache.

We have minimumCacheTTL configured for an hour, but many people might not know about it, and IMO, this would benefit people in general.

ijjk · 2024-07-03T20:28:27Z

Failing test suites

Commit: a1f627d

TURBOPACK=1 pnpm test-dev test/development/acceptance/ReactRefreshRequire.test.ts (turbopack)

ReactRefreshRequire > re-runs accepted modules
ReactRefreshRequire > propagates a hot update to closest accepted module
ReactRefreshRequire > propagates a module that stops accepting in next version

Expand output

● ReactRefreshRequire › re-runs accepted modules

expect(received).toEqual(expected) // deep equality

Expected: ["init FooV1", "init BarV1"]
Received: undefined

  33 |       `require('./foo'); export default function Noop() { return null; };`
  34 |     )
> 35 |     expect(await session.evaluate(() => (window as any).log)).toEqual([
     |                                                               ^
  36 |       'init FooV1',
  37 |       'init BarV1',
  38 |     ])

  at Object.toEqual (development/acceptance/ReactRefreshRequire.test.ts:35:63)

● ReactRefreshRequire › propagates a hot update to closest accepted module

expect(received).toEqual(expected) // deep equality

- Expected  - 0
+ Received  + 2

  Array [
+   "init FooV1",
+   "init FooV1",
    "init BarV4",
  ]

  161 |       `
  162 |     )
> 163 |     expect(await session.evaluate(() => (window as any).log)).toEqual([
      |                                                               ^
  164 |       'init BarV4',
  165 |     ])
  166 |

  at Object.toEqual (development/acceptance/ReactRefreshRequire.test.ts:163:63)

● ReactRefreshRequire › propagates a module that stops accepting in next version

expect(received).toEqual(expected) // deep equality

- Expected  - 0
+ Received  + 1

  Array [
+   "init FooV1",
    "init BarV2",
    "init FooV1",
  ]

  442 |     // Since the export list changed, we have to re-run both the parent
  443 |     // and the child.
> 444 |     expect(await session.evaluate(() => (window as any).log)).toEqual([
      |                                                               ^
  445 |       'init BarV2',
  446 |       'init FooV1',
  447 |     ])

  at Object.toEqual (development/acceptance/ReactRefreshRequire.test.ts:444:63)

Read more about building and testing Next.js in contributing.md.

pnpm test-dev test/e2e/app-dir/params-hooks-compat/index.test.ts

app-dir - params hooks compat > should only access search params with useSearchParams

Expand output

● app-dir - params hooks compat › should only access search params with useSearchParams

expect(received).toEqual(expected) // deep equality

- Expected  - 3
+ Received  + 1

- Object {
-   "q": "pages",
- }
+ Object {}

  17 |
  18 |     expect(appSearchparamsJSON).toEqual({ q: 'app' })
> 19 |     expect(pagesSearchparamsJSON).toEqual({ q: 'pages' })
     |                                   ^
  20 |   })
  21 |
  22 |   it('should only access path params with useParams', async () => {

  at Object.toEqual (e2e/app-dir/params-hooks-compat/index.test.ts:19:35)

Read more about building and testing Next.js in contributing.md.

styfle · 2024-07-03T21:09:28Z

packages/next/src/server/image-optimizer.ts

-        '"url" parameter is valid but image type is not allowed'
-      )
-    }
+  if (


Why was this svg logic moved?

Seems like this a leftover, or don't remember anymore why I moved it, seems like there's no point in doing that. Reverted this now

styfle · 2024-07-03T21:11:17Z

packages/next/src/server/image-optimizer.ts

+  return (
+    previousCacheEntry?.value?.kind === 'IMAGE' &&
+    // If the cached image is optimized
+    previousCacheEntry.value.upstreamEtag !== previousCacheEntry.value.etag &&


Can you explain why upstreamEtag should not be equal to etag?

This seems like it will always return true when not equal and therefore it will never actually revalidate, which seems wrong to me.

What this PR tries to do is prevent hitting this specific path, with optimizeImage is the expensive operation.

upstreamEtag represents the original source image's etag (currently calculated via the same function getImageEtag), whereas the cache entry's etag represents the resulting image's etag, i.e. in the case image optimization is done, the etag will be the hash of that optimized image instead of the upstream image.

This condition exists to cause a revalidation on cases where the optimization did not happen previously (in this case we fallback to using upstreamBuffer and upstreamEtag for the image), so this will fail early on those cases and cause a new optimization attempt. Although now thinking back at it the likelyhood of the image optimization ending up with the same outcome is quite likely anyways.

When this condition is true, we'll check the next condition, comparing the previous upstream image's etag against the current upstream image's etag to determine if we should revalidate or not.

The optimization not happening could be because of a fallback (such as an error was thrown) or it could be because the image was vector form (svg doesnt need to be optimized).

Either way, we still need to revalidate those images because the upstream response could have changed since the previous request. So I suggest removing line 463 and that way the following line (465) can actually compare the new upstream etag.

Hmm, I'm not quite following, as the function is shouldUsePreviouslyCachedEntry, all of the conditions inside it need to return true for the revalidation not to occur.

This is how I see it:

If this comparison is false, we will revalidate the image

If this comparison is true, we will do the etag comparison (line 465) to determine if we want to revalidate the image.

By revalidate, I mean that the previous entry is not used and we'll fallback to previous behaviour

Am I misunderstanding something? What do you mean by revalidation? I'm fine with removing this line, but don't fully understand the reasoning 😅

So what is the scenario where the line is needed? Is there a test for this?

I can't see your link anymore because you force pushed.

(note: you don't need to force push because we squash on merging)

Oh I see now. I read through this again and I see that the line is handling the case when svg or animated. I think the comment should mention this.

styfle · 2024-07-03T21:14:19Z

packages/next/src/server/image-optimizer.ts

+    // If the cached image is optimized
+    previousCacheEntry.value.upstreamEtag !== previousCacheEntry.value.etag &&
+    // and the upstream etag is the same as the previous cache entry's
+    getImageEtag(upstreamImage.buffer) === previousCacheEntry.value.upstreamEtag


I think you could improve performance here by using the etag from the upstream response header and fallback to the hash if its missing.

I thought of using the upstream response header at first, but didn't like how the code turned out, or because of the weak validation syntax (it can start with W/, the etag value would need to be base64-encoded correctly to prevent any errors.

What I had previously was alter ImageUpstream to include etag and then get it from the response and have the hash as fallback. Didn't quite like the implementation then, but writing an ETag extractor helper function would probably make it a bit nicer and make it possible to write tests for it.

Added this now along with tests, not 100% happy with the tests but they should still be sufficient I think.

styfle · 2024-08-23T21:05:52Z

test/unit/image-optimizer/extract-etag.test.ts

+  it('should return base64url encoded etag if etag is provided', () => {
+    const etag = 'sample-etag'
+    const result = extractEtag(etag, Buffer.from(''))
+    expect(result).toMatch(/^([a-z]|[A-Z]|[0-9]|-|_)+$/)


Can we use exact string comparison instead of regex here?

styfle · 2024-08-23T21:06:02Z

test/unit/image-optimizer/extract-etag.test.ts

+  it('should not return weak etag identifier if etag is provided', () => {
+    const etag = 'W/"sample-etag"'
+    const result = extractEtag(etag, Buffer.from(''))
+    expect(result).not.toMatch(/^W\//)


Same here, can we use exact string comparison instead of regex here?

…tream image is identical

…shing if available

styfle · 2024-08-27T20:19:44Z

packages/next/src/server/image-optimizer.ts

+  etag: string | null | undefined,
+  imageBuffer: Buffer
+) {
+  if (!!etag) {


Suggested change

if (!!etag) {

if (etag) {

styfle · 2024-08-27T20:25:14Z

test/unit/image-optimizer/extract-etag.test.ts

+    expect(result).not.toContain('/')
+    expect(result).toMatch('Vy8ic2FtcGxlLWV0YWci')


Suggested change

expect(result).not.toContain('/')

expect(result).toMatch('Vy8ic2FtcGxlLWV0YWci')

expect(result).toEqual('Vy8ic2FtcGxlLWV0YWci')

styfle · 2024-08-27T20:25:22Z

test/unit/image-optimizer/extract-etag.test.ts

+  it('should return base64url encoded etag if etag is provided', () => {
+    const etag = 'sample-etag'
+    const result = extractEtag(etag, Buffer.from(''))
+    expect(result).toMatch('c2FtcGxlLWV0YWc')


Suggested change

expect(result).toMatch('c2FtcGxlLWV0YWc')

expect(result).toEqual('c2FtcGxlLWV0YWc')

styfle · 2024-08-27T20:49:41Z

packages/next/src/server/image-optimizer.ts

  const maxAge = getMaxAge(imageUpstream.cacheControl)
+
+  if (shouldUsePreviouslyCachedEntry(imageUpstream, previousCacheEntry)) {
+    // Return the cached optimized image


Suggested change

// Return the cached optimized image

You can remove this comment.

Alternatively, you can change shouldUsePreviouslyCachedEntry() from returning a boolean to returning the result and naming it something like getPreviouslyCachedOrNull()

styfle · 2024-08-27T20:50:45Z

packages/next/src/server/image-optimizer.ts

+    // Return the cached optimized image
+    return {
+      buffer: previousCacheEntry.value.buffer,
+      contentType: paramsResult.mimeType,


Are you sure the contentType is correct here?

We have a lot of logic to determine the contentType below so perhaps we should move the cached entry check below this:

next.js/packages/next/src/server/image-optimizer.ts

Lines 636 to 649 in ca73527

let contentType: string

if (mimeType) {

contentType = mimeType

} else if (

upstreamType?.startsWith('image/') &&

getExtension(upstreamType) &&

upstreamType !== WEBP &&

upstreamType !== AVIF

) {

contentType = upstreamType

} else {

contentType = JPEG

}

ijjk added tests type: next labels Jun 27, 2024

kahlstrm commented Jun 27, 2024

View reviewed changes

packages/next/src/server/image-optimizer.ts Show resolved Hide resolved

kahlstrm commented Jun 27, 2024

View reviewed changes

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch from bda2f1a to a1f627d Compare June 29, 2024 17:07

styfle reviewed Jul 3, 2024

View reviewed changes

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch from a1f627d to 39d6d5a Compare July 3, 2024 23:51

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch 3 times, most recently from 54d55bd to 3c09907 Compare July 21, 2024 00:00

kahlstrm requested a review from styfle July 21, 2024 00:01

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch from 3c09907 to fba6061 Compare August 6, 2024 00:14

styfle reviewed Aug 23, 2024

View reviewed changes

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch 2 times, most recently from f2acee3 to cd61981 Compare August 24, 2024 07:39

kahlstrm requested a review from styfle August 24, 2024 16:34

kahlstrm added 2 commits August 24, 2024 19:35

feat(image-optimizer): use previously cached image cache entry if ups…

6d73f6e

…tream image is identical

feat(image-optimizer): use upstream image etag directly instead of ha…

f406464

…shing if available

kahlstrm force-pushed the feat/use-previous-cache-entry-if-same-upstream-image branch from cd61981 to f406464 Compare August 24, 2024 16:35

styfle reviewed Aug 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

kahlstrm commented Jun 27, 2024 •

edited

Loading

ijjk commented Jun 27, 2024 •

edited

Loading

kahlstrm Jun 27, 2024

kahlstrm commented Jul 3, 2024

ijjk commented Jul 3, 2024 •

edited

Loading

styfle Jul 3, 2024

kahlstrm Jul 3, 2024 •

edited

Loading

styfle Jul 3, 2024

kahlstrm Jul 3, 2024 •

edited

Loading

styfle Jul 3, 2024 •

edited

Loading

kahlstrm Jul 4, 2024 •

edited

Loading

styfle Aug 27, 2024

styfle Aug 27, 2024

styfle Jul 3, 2024

kahlstrm Jul 3, 2024

kahlstrm Jul 21, 2024

styfle Aug 23, 2024

styfle Aug 23, 2024

styfle Aug 27, 2024

styfle Aug 27, 2024

styfle Aug 27, 2024

styfle Aug 27, 2024

styfle Aug 27, 2024 •

edited

Loading

		expect(result).not.toContain('/')
		expect(result).toMatch('Vy8ic2FtcGxlLWV0YWci')

	expect(result).not.toContain('/')
	expect(result).toMatch('Vy8ic2FtcGxlLWV0YWci')
	expect(result).toEqual('Vy8ic2FtcGxlLWV0YWci')

	expect(result).toMatch('c2FtcGxlLWV0YWc')
	expect(result).toEqual('c2FtcGxlLWV0YWc')

	let contentType: string

	if (mimeType) {
	contentType = mimeType
	} else if (
	upstreamType?.startsWith('image/') &&
	getExtension(upstreamType) &&
	upstreamType !== WEBP &&
	upstreamType !== AVIF
	) {
	contentType = upstreamType
	} else {
	contentType = JPEG
	}

feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

Are you sure you want to change the base?

feat(image-optimizer): use previously cached image cache entry if upstream image is identical #67257

Conversation

kahlstrm commented Jun 27, 2024 • edited Loading

What?

Why?

How?

ijjk commented Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

kahlstrm commented Jul 3, 2024

ijjk commented Jul 3, 2024 • edited Loading

Failing test suites

Choose a reason for hiding this comment

kahlstrm Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahlstrm Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

styfle Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

kahlstrm Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

kahlstrm commented Jun 27, 2024 •

edited

Loading

ijjk commented Jun 27, 2024 •

edited

Loading

ijjk commented Jul 3, 2024 •

edited

Loading

kahlstrm Jul 3, 2024 •

edited

Loading

kahlstrm Jul 3, 2024 •

edited

Loading

styfle Jul 3, 2024 •

edited

Loading

kahlstrm Jul 4, 2024 •

edited

Loading

styfle Aug 27, 2024 •

edited

Loading