perf: use conditional blob gets if same blob was fetched before for previous requests #3218
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report 0.08%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
perf/conditional-blob-get
branch
2 times, most recently
from
375cbb7 to
5bc519f
Compare
pieh
commented
Oct 29, 2025
| }, | ||
| ], | ||
| 'unicorn/filename-case': ['error', { case: 'kebabCase' }], | ||
| 'unicorn/no-nested-ternary': 'off', |
There was a problem hiding this comment.
Because of sindresorhus/eslint-plugin-unicorn#2604 where prettier is getting rid of extra parenthesis needed to satisfy this rule
pieh
force-pushed
the
perf/conditional-blob-get
branch
from
5bc519f to
346ed9f
Compare
pieh
commented
Oct 29, 2025
Comment on lines
+28
to
+32
| if ( | ||
| memoizedValue?.conditional === false && | ||
| typeof memoizedValue?.currentRequestValue !== 'undefined' | ||
| ) { | ||
| return memoizedValue.currentRequestValue as T | null | Promise<T | null> |
There was a problem hiding this comment.
inMemoryCache.get now will potentially return values from previous requests - the conditional property instruct wether we can just use it without checking blobs or if we need to to do roundtrip to blobs to check if we can reuse this via conditional store.getWithMetadata call
pieh
force-pushed
the
perf/conditional-blob-get
branch
from
346ed9f to
89a536f
Compare
pieh
commented
Oct 29, 2025
Comment on lines
+63
to
+64
| staleAt: 123, | ||
| expireAt: 456, |
There was a problem hiding this comment.
not related to this PR other than fact that I was touching this test and noticed types were no longer happy after TagManifest changes that were done in #3173 and this is just to satisfy types and otherwise doesn't have impact on actual tests
pieh
commented
Oct 29, 2025
Comment on lines
+136
to
197
| it('does not automatically reuse in-memory values when running in request contexts', async () => { | ||
| const store = getMemoizedKeyValueStoreBackedByRegionalBlobStore() | ||
| await runWithRequestContext(createRequestContext(), async () => { | ||
| await store.get(TEST_KEY, OTEL_SPAN_TITLE) | ||
| const get1 = await runWithRequestContext(createRequestContext(), async () => { | ||
| return await store.get(TEST_KEY, OTEL_SPAN_TITLE) | ||
| }) | ||
|
|
||
| await runWithRequestContext(createRequestContext(), async () => { | ||
| await store.get(TEST_KEY, OTEL_SPAN_TITLE) | ||
| const get2 = await runWithRequestContext(createRequestContext(), async () => { | ||
| return await store.get(TEST_KEY, OTEL_SPAN_TITLE) | ||
| }) | ||
|
|
||
| expect( | ||
| mockedStore.get, | ||
| mockedStore.getWithMetadata, | ||
| 'Blobs should be requested separately for each request context', | ||
| ).toHaveBeenCalledTimes(2) | ||
|
|
||
| // first request context assertions | ||
| expect(get1, 'store.get in first request should return expected value').toEqual( | ||
| TEST_DEFAULT_VALUE, | ||
| ) | ||
|
|
||
| expect( | ||
| mockedStore.getWithMetadata, | ||
| 'On first request context, we should not provide etag as we do not have any yet', | ||
| ).toHaveBeenNthCalledWith(1, expect.any(String), { | ||
| etag: undefined, | ||
| type: 'json', | ||
| }) | ||
|
|
||
| expect( | ||
| mockedStore.getWithMetadata, | ||
| 'should return full value from blobs as it is first time being requested', | ||
| ).toHaveNthResolvedWith( | ||
| 1, | ||
| expect.objectContaining({ | ||
| data: TEST_DEFAULT_VALUE, | ||
| }), | ||
| ) | ||
|
|
||
| // second request context assertions | ||
| expect(get2, 'store.get in second request should return expected value').toEqual( | ||
| TEST_DEFAULT_VALUE, | ||
| ) | ||
|
|
||
| expect( | ||
| mockedStore.getWithMetadata, | ||
| 'On second request context, we should provide an etag as first request fetched same blob', | ||
| ).toHaveBeenNthCalledWith(2, expect.any(String), { | ||
| etag: expect.any(String), | ||
| type: 'json', | ||
| }) | ||
|
|
||
| expect( | ||
| mockedStore.getWithMetadata, | ||
| 'On second request context, we should not get blob value, just indication that we can reuse blob', | ||
| ).toHaveNthResolvedWith( | ||
| 2, | ||
| expect.objectContaining({ | ||
| data: null, | ||
| etag: expect.any(String), | ||
| }), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
This is the expanded test for changes in this PR. Rest of changes in test suite are to fix types as mentioned in https://github.com/opennextjs/opennextjs-netlify/pull/3218/files#r2473214413 and to cover change from blobStore.get to blobStore.getWithMetadata
pieh
commented
Oct 29, 2025
| span?.setAttributes({ key, blobKey }) | ||
| const blob = (await store.get(blobKey, { type: 'json' })) as T | null | ||
| inMemoryCache.set(key, blob) | ||
| span?.addEvent(blob ? 'Hit' : 'Miss') |
There was a problem hiding this comment.
I converted from adding event to setting status attribute to make it easier for querying. The event was also always at the end of span anyway, so we don't get any extra value from using events in this case
pieh
commented
Oct 29, 2025
| span?.setAttributes({ | ||
| etag: result?.etag, | ||
| reusingPreviouslyFetchedBlob: shouldReuseMemoizedBlob, | ||
| status: blob ? (shouldReuseMemoizedBlob ? 'Hit, no change' : 'Hit') : 'Miss', |
There was a problem hiding this comment.
this is pretty much modeling 304, 200, 404 response status codes from underlying blobs response
hopefully this is temporary until we can nicely "merge" next-runtime produced span with blobs spans to avoid almost duplication (those spans below represent same thing)
without losing next.js specific context (primarly logical cache key vs blob key which is encoded and the context of blobs get - is it for tag manifest, static html blob or cachehandler entry)
mrstork
approved these changes
Oct 31, 2025
| return extendedGlobalThis[IN_MEMORY_LRU_CACHE] | ||
| } | ||
|
|
||
| export function clearInMemoryLRUCacheForTesting() { |
There was a problem hiding this comment.
I think ideally I would want this kind of helper to be in a test file rather than next to the real code, that said this is very much a preference and not an ask you have to actually do
| await store.get('heavy-route-1', OTEL_SPAN_TITLE) | ||
| expect(mockedStore.get, 'Value should be read from memory').toHaveBeenCalledTimes(0) | ||
| mockedStore.get.mockClear() | ||
| expect(mockedStore.getWithMetadata, 'Value should be read from memory').toHaveBeenCalledTimes( |
There was a problem hiding this comment.
There's not.toHaveBeenCalled if you'd like
| // operations in requestContext2 should result in evicting value for requestContext1 | ||
| expect(mockedStore.get, 'Value should be read from blobs').toHaveBeenCalledTimes(1) | ||
| mockedStore.get.mockClear() | ||
| expect(mockedStore.getWithMetadata, 'Value should be read from blobs').toHaveBeenCalledTimes( |
There was a problem hiding this comment.
There is a toHaveBeenCalledOnce if you'd like
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This introduces usage of conditional blob get requests if we fetched or stored given blob key on previous requests in given lambda instance. This allows to avoid fetching blob data if blob didn't change since last time given lambda.
This builds on top of memoization we already do within same request introduced in #2777
It does add global blobs map with weak refs to entries stored in our current LRU cache as a mechanism to offload memory management to what we already have and not having to do complex tracking of when global entries should be disposed.
Existing tests for memoized storage were updated and expanded to tests conditional blob gets scenario