perf: reduce Response.clone() overhead in tiered cache#1090
perf: reduce Response.clone() overhead in tiered cache#1090hugo-ccabral wants to merge 1 commit intomainfrom
Conversation
Response.clone() creates new ReadableStreams and buffers for each tier. Instead, read the body once as ArrayBuffer and create lightweight Response objects from buffer slices for each tier. This avoids the stream tee machinery and reduces memory allocations. Memory profile showed Response cloning in the tiered cache at ~2 percent of allocations via readableStreamTee and cloneInnerResponse. Co-authored-by: Cursor <cursoragent@cursor.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe caching logic in tiered caches is refactored to materialize response bodies as arrayBuffer and reconstruct Response objects with captured headers, replacing reliance on clone() for writing to all targeted caches. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/tiered.ts">
<violation number="1" location="runtime/caches/tiered.ts:47">
P1: Reading `matched.arrayBuffer()` consumes the response body, so the `matched` response returned from `match()` will be unusable. Consider returning a new Response built from the buffered body (and using that for cache updates) or otherwise avoid consuming the response that is returned to callers.</violation>
<violation number="2" location="runtime/caches/tiered.ts:53">
P2: The new Response created for cache tiers omits `status` and `statusText`, which changes the cached response semantics (e.g., 404 becomes 200). Preserve the original status fields when constructing the Response.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| request: RequestInfo | URL, | ||
| matched: Response, | ||
| ) { | ||
| const body = await matched.arrayBuffer(); |
There was a problem hiding this comment.
P1: Reading matched.arrayBuffer() consumes the response body, so the matched response returned from match() will be unusable. Consider returning a new Response built from the buffered body (and using that for cache updates) or otherwise avoid consuming the response that is returned to callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/tiered.ts, line 47:
<comment>Reading `matched.arrayBuffer()` consumes the response body, so the `matched` response returned from `match()` will be unusable. Consider returning a new Response built from the buffered body (and using that for cache updates) or otherwise avoid consuming the response that is returned to callers.</comment>
<file context>
@@ -44,9 +44,14 @@ export function createTieredCache(
request: RequestInfo | URL,
matched: Response,
) {
+ const body = await matched.arrayBuffer();
+ const headers = matched.headers;
await Promise.all(
</file context>
| openedCaches[index].put(request, matched.clone()) | ||
| openedCaches[index].put( | ||
| request, | ||
| new Response(body.slice(0), { headers }), |
There was a problem hiding this comment.
P2: The new Response created for cache tiers omits status and statusText, which changes the cached response semantics (e.g., 404 becomes 200). Preserve the original status fields when constructing the Response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/tiered.ts, line 53:
<comment>The new Response created for cache tiers omits `status` and `statusText`, which changes the cached response semantics (e.g., 404 becomes 200). Preserve the original status fields when constructing the Response.</comment>
<file context>
@@ -44,9 +44,14 @@ export function createTieredCache(
- openedCaches[index].put(request, matched.clone())
+ openedCaches[index].put(
+ request,
+ new Response(body.slice(0), { headers }),
+ )
),
</file context>
| new Response(body.slice(0), { headers }), | |
| new Response(body.slice(0), { | |
| headers, | |
| status: matched.status, | |
| statusText: matched.statusText, | |
| }), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runtime/caches/tiered.ts (1)
113-117:⚠️ Potential issue | 🔴 Critical
matchedbody will be disturbed when returned to the caller.
updateTieredCachesis called withoutawaiton line 114. Inside it,await matched.arrayBuffer()immediately starts consumingmatched's body stream. Becausereturn matchedon line 117 executes synchronously in the same tick, the caller receives the sameResponseobject whose body is already locked/being consumed. Any attempt by the caller to read the body will encounter a disturbed stream.The fix is to eagerly materialise the body here in
match()so that a freshResponseis both returned to the caller and handed toupdateTieredCaches:🐛 Proposed fix – materialise body in `match()` and pass buffer to both paths
if (matched) { - updateTieredCaches(indexOfCachesToUpdate, request, matched); + if (indexOfCachesToUpdate.length > 0) { + const body = await matched.arrayBuffer(); + const { status, statusText, headers } = matched; + matched = new Response(body.slice(0), { status, statusText, headers }); + updateTieredCaches(indexOfCachesToUpdate, request, body, { status, statusText, headers }); + } } return matched;Then update
updateTieredCachesto accept the pre-read buffer and metadata instead of aResponse:- async function updateTieredCaches( - indexOfCachesToUpdate: number[], - request: RequestInfo | URL, - matched: Response, - ) { - const body = await matched.arrayBuffer(); - const headers = matched.headers; + async function updateTieredCaches( + indexOfCachesToUpdate: number[], + request: RequestInfo | URL, + body: ArrayBuffer, + init: { status: number; statusText: string; headers: Headers }, + ) { await Promise.all( indexOfCachesToUpdate.map((index) => openedCaches[index].put( request, - new Response(body.slice(0), { headers }), + new Response(body.slice(0), init), ) ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/caches/tiered.ts` around lines 113 - 117, In match(), eagerly read the matched Response body (e.g., await matched.arrayBuffer()) before returning so the returned Response has an unlocked fresh body; create a new Response for the caller (using the buffer and copying headers/status) and pass the buffer plus metadata to updateTieredCaches instead of passing the original Response; update updateTieredCaches signature to accept the pre-read body buffer and any needed headers/status and use those to construct the cached Response for storage so the original caller gets an unconsumed Response and caching still gets the body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/caches/tiered.ts`:
- Around line 47-56: When reconstructing Responses from a cached Response
(variable matched) before calling openedCaches[index].put, preserve
matched.status and matched.statusText in the Response init; specifically, pass
status: matched.status and statusText: matched.statusText (alongside headers)
when creating the new Response from body.slice(0). Apply the same change in the
other occurrence inside the put method (the code that creates new
Response(body.slice(0), { headers })), ensuring both read the original
matched.status and matched.statusText so non-200 and error responses are stored
with their original status.
- Around line 131-138: The code in the openedCaches.map where it does
cache.put(request, new Response(body.slice(0), { headers })) loses the original
response status and statusText, causing all cached entries to be 200 OK; update
that call in the same block (inside the openedCaches.map / putPromises creation)
to construct the new Response with the original response.status and
response.statusText passed in the options (e.g., new Response(body.slice(0), {
headers, status: response.status, statusText: response.statusText })) so each
tier preserves the original status and statusText.
---
Outside diff comments:
In `@runtime/caches/tiered.ts`:
- Around line 113-117: In match(), eagerly read the matched Response body (e.g.,
await matched.arrayBuffer()) before returning so the returned Response has an
unlocked fresh body; create a new Response for the caller (using the buffer and
copying headers/status) and pass the buffer plus metadata to updateTieredCaches
instead of passing the original Response; update updateTieredCaches signature to
accept the pre-read body buffer and any needed headers/status and use those to
construct the cached Response for storage so the original caller gets an
unconsumed Response and caching still gets the body.
| const body = await matched.arrayBuffer(); | ||
| const headers = matched.headers; | ||
| await Promise.all( | ||
| indexOfCachesToUpdate.map((index) => | ||
| openedCaches[index].put(request, matched.clone()) | ||
| openedCaches[index].put( | ||
| request, | ||
| new Response(body.slice(0), { headers }), | ||
| ) | ||
| ), | ||
| ); |
There was a problem hiding this comment.
status and statusText are silently dropped from reconstructed Responses.
new Response(body.slice(0), { headers }) defaults status to 200 and statusText to "". The Response constructor's init object accepts status and statusText as explicit fields. Any non-200 response (e.g., a cached 304, 206, or error response stored by the caller) will be written back to tiered caches with the wrong status, corrupting cache state.
The same defect is present in the put method (line 136).
🐛 Proposed fix – preserve status and statusText
- const body = await matched.arrayBuffer();
- const headers = matched.headers;
+ const body = await matched.arrayBuffer();
+ const { status, statusText, headers } = matched;
await Promise.all(
indexOfCachesToUpdate.map((index) =>
openedCaches[index].put(
request,
- new Response(body.slice(0), { headers }),
+ new Response(body.slice(0), { status, statusText, headers }),
)
),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = await matched.arrayBuffer(); | |
| const headers = matched.headers; | |
| await Promise.all( | |
| indexOfCachesToUpdate.map((index) => | |
| openedCaches[index].put(request, matched.clone()) | |
| openedCaches[index].put( | |
| request, | |
| new Response(body.slice(0), { headers }), | |
| ) | |
| ), | |
| ); | |
| const body = await matched.arrayBuffer(); | |
| const { status, statusText, headers } = matched; | |
| await Promise.all( | |
| indexOfCachesToUpdate.map((index) => | |
| openedCaches[index].put( | |
| request, | |
| new Response(body.slice(0), { status, statusText, headers }), | |
| ) | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/tiered.ts` around lines 47 - 56, When reconstructing Responses
from a cached Response (variable matched) before calling
openedCaches[index].put, preserve matched.status and matched.statusText in the
Response init; specifically, pass status: matched.status and statusText:
matched.statusText (alongside headers) when creating the new Response from
body.slice(0). Apply the same change in the other occurrence inside the put
method (the code that creates new Response(body.slice(0), { headers })),
ensuring both read the original matched.status and matched.statusText so non-200
and error responses are stored with their original status.
| const body = await response.arrayBuffer(); | ||
| const headers = response.headers; | ||
| const putPromises = openedCaches.map((cache) => | ||
| cache.put( | ||
| request, | ||
| new Response(body.slice(0), { headers }), | ||
| ) | ||
| ); |
There was a problem hiding this comment.
status and statusText are dropped — same defect as updateTieredCaches.
new Response(body.slice(0), { headers }) reconstructs the response without status/statusText, so every cache tier receives a 200 OK regardless of the original response status.
🐛 Proposed fix
- const body = await response.arrayBuffer();
- const headers = response.headers;
- const putPromises = openedCaches.map((cache) =>
- cache.put(
- request,
- new Response(body.slice(0), { headers }),
- )
- );
+ const body = await response.arrayBuffer();
+ const { status, statusText, headers } = response;
+ const putPromises = openedCaches.map((cache) =>
+ cache.put(
+ request,
+ new Response(body.slice(0), { status, statusText, headers }),
+ )
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = await response.arrayBuffer(); | |
| const headers = response.headers; | |
| const putPromises = openedCaches.map((cache) => | |
| cache.put( | |
| request, | |
| new Response(body.slice(0), { headers }), | |
| ) | |
| ); | |
| const body = await response.arrayBuffer(); | |
| const { status, statusText, headers } = response; | |
| const putPromises = openedCaches.map((cache) => | |
| cache.put( | |
| request, | |
| new Response(body.slice(0), { status, statusText, headers }), | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime/caches/tiered.ts` around lines 131 - 138, The code in the
openedCaches.map where it does cache.put(request, new Response(body.slice(0), {
headers })) loses the original response status and statusText, causing all
cached entries to be 200 OK; update that call in the same block (inside the
openedCaches.map / putPromises creation) to construct the new Response with the
original response.status and response.statusText passed in the options (e.g.,
new Response(body.slice(0), { headers, status: response.status, statusText:
response.statusText })) so each tier preserves the original status and
statusText.
1 participant
Summary
put(): Read response body once asArrayBuffer, then create newResponseobjects frombody.slice(0)for each tier. Previously calledresponse.clone()per tier, creating new ReadableStreams with stream tee machinery.updateTieredCaches(): Same pattern — read matched response body once, distribute via buffer slices instead of cloning.Context
Memory profile showed
Response.clone()in the tiered cache contributing ~2% of allocations viareadableStreamTee->readableStreamDefaultTee->createReadableStream->cloneInnerResponse. Each clone creates a new ReadableStream plus internal buffers.ArrayBuffer.slice(0)creates a copy of the buffer data (required since each tier'sput()will consume the body), but avoids the overhead of the full stream cloning machinery (tee state, cancel functions, pull algorithms, etc.).Test plan
Summary by cubic
Reduces memory and CPU in the tiered cache by replacing Response.clone() with a single body read and lightweight Response objects per tier. This removes ReadableStream tee overhead while preserving headers.
Written for commit f8cdd06. Summary will update on new commits.
Summary by CodeRabbit