-
Notifications
You must be signed in to change notification settings - Fork 91
perf: use conditional blob gets if same blob was fetched before for previous requests #3218
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
Merged
+298
−90
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
pieh
force-pushed
the
perf/conditional-blob-get
branch
2 times, most recently
from
October 29, 2025 12:23
375cbb7 to
5bc519f
Compare
pieh
pieh
commented
Oct 29, 2025
},
],
'unicorn/filename-case': ['error', { case: 'kebabCase' }],
'unicorn/no-nested-ternary': 'off',
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of sindresorhus/eslint-plugin-unicorn#2604 where prettier is getting rid of extra parenthesis needed to satisfy this rule
@pieh
pieh
force-pushed
the
perf/conditional-blob-get
branch
from
October 29, 2025 12:37
5bc519f to
346ed9f
Compare
pieh
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
...revious requests
@pieh
pieh
force-pushed
the
perf/conditional-blob-get
branch
from
October 29, 2025 12:55
346ed9f to
89a536f
Compare
pieh
pieh
commented
Oct 29, 2025
Comment on lines
+63
to
+64
staleAt: 123,
expireAt: 456,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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)
constget1=await runWithRequestContext(createRequestContext(), async () => {
returnawait store.get(TEST_KEY, OTEL_SPAN_TITLE)
})
await runWithRequestContext(createRequestContext(), async () => {
await store.get(TEST_KEY, OTEL_SPAN_TITLE)
constget2=await runWithRequestContext(createRequestContext(), async () => {
returnawait 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
image
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
mrstork
previously approved these changes
Oct 31, 2025
mrstork
mrstork
approved these changes
Nov 3, 2025
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.
Uh oh!
There was an error while loading. Please reload this page.
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