Reduce per-request URL parsing and allocations in the SSR request pipeline#17227
Open
iseraph-dev wants to merge 3 commits into
Open
Reduce per-request URL parsing and allocations in the SSR request pipeline#17227iseraph-dev wants to merge 3 commits into
iseraph-dev wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: bc79d7d The changes in this PR will be included in the next version bump. This PR includes changesets to release 393 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
pkg: integration
Merging this PR will degrade performance by 17.69%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes |
iseraph-dev
force-pushed
the
perf/ssr-request-pipeline
branch
from
6385486 to
1813d4f
Compare
…eline
The SSR hot path (app.render -> FetchState -> AstroHandler, shared by
astro/fetch and astro/hono) parsed request.url up to three times per request
and made a handful of avoidable per-request allocations. This collapses it to
one URL parse per request across the core and every server adapter, and trims
the allocations. Internal plumbing only - no behavior change.
One URL parse per request:
- Render path: the domain-i18n probe now runs only when a domains-* strategy is
configured (isDomainI18nStrategy, computed once in the BaseApp constructor),
and TrailingSlashHandler reuses the raw pathname/search that FetchState
captures up front (refreshed when X-Forwarded-* rebuilds the request), leaving
FetchState as the single parse.
- app.match() accepts an optional pre-parsed URL and uses the same domain-probe
gate; RenderOptions.url lets FetchState reuse a caller-provided URL.
- Pre-matching adapters (Cloudflare, Netlify, Vercel) parse request.url once and
pass it to both app.match() and app.render(): Cloudflare and Vercel drop from
3 parses/request to 1, Netlify from 2 to 1. Cloudflare's matchStaticAsset()
takes the parsed URL too.
- @astrojs/node: createRequestFromNodeRequest already built a URL to construct
the Request, so it now returns { request, url }, threaded through serve-app,
serve-static and NodeApp into app.match() and app.render(). Node drops from
3 parses/request to 1.
Fewer per-request allocations:
- createOutgoingHttpHeaders builds the Node headers in one pass instead of
Object.fromEntries(headers.entries()) plus a separate keys() check.
- getParams short-circuits on the first matching pattern (no .map().map().find()
intermediate arrays).
- The memory cache provider reuses context.url instead of re-parsing
new URL(context.request.url).
- Domain-based i18n parses each domainLookupTable key once (memoized per table).
- Netlify's cache-control header check short-circuits with || instead of
allocating an array for .some().
Note: createRequestFromNodeRequest is exported from astro/app/node; its return
shape changes from Request to { request, url }, so external importers must now
destructure .request.
Covered by the existing astro unit (3014/0), astro integration (1175/0) and
@astrojs/{node,cloudflare,netlify,vercel} suites.
iseraph-dev
force-pushed
the
perf/ssr-request-pipeline
branch
from
1813d4f to
7bd6a5a
Compare
ematipico
requested changes
Jun 30, 2026
ematipico
left a comment
ematipico
left a comment
Member
There was a problem hiding this comment.
This PR adds too many things. I suggest splitting it in three PRs:
- the reducing of the parsing
- the addition of the new advanced API. Debatable, but with a new PR at least you can explain what's needed for
Comment on lines
+87
to
+95
| /** | ||
| * **Advanced API**: you probably do not need to use this. | ||
| * | ||
| * A pre-parsed `URL` for this request, so adapters that already parsed | ||
| * `request.url` can avoid a second parse. It **must** equal | ||
| * `new URL(request.url)`, and it is normalized in place during rendering, so | ||
| * the object must not be reused after calling `render`. | ||
| */ | ||
| url?: URL; |
Member
There was a problem hiding this comment.
The PR didn't mention this new API. It should be removed, or it should be documented and part of a minor changeset
| public match( | ||
| request: Request, | ||
| allowPrerenderedRoutes = false, | ||
| url = new URL(request.url), |
Member
There was a problem hiding this comment.
To remove in case we remove the "advanced" API
| } | ||
|
|
||
| private computePathnameFromDomain(request: Request): string | undefined { | ||
| private computePathnameFromDomain(request: Request, url = new URL(request.url)): string | undefined { |
Member
There was a problem hiding this comment.
To remove in case we remove the advanced api
Comment on lines
+17
to
+27
| // Copy the Web Headers into a plain object for Node. Iterating `headers` | ||
| // yields lowercased names with any multi-value header already comma-joined; | ||
| // `isEmpty` records whether anything was copied so a header-less response | ||
| // returns `undefined`. A comma-joined `set-cookie` is invalid, so it is | ||
| // rebuilt as an array below. | ||
| const nodeHeaders: OutgoingHttpHeaders = {}; | ||
| let isEmpty = true; | ||
| for (const [key, value] of headers) { | ||
| nodeHeaders[key] = value; | ||
| isEmpty = false; | ||
| } |
Member
There was a problem hiding this comment.
This should be reverted. This a runtime-agnostic code and "Node" things aren't allowed.
| const cookieHeaders = headers.getSetCookie(); | ||
| if (cookieHeaders.length > 1) { | ||
| // the Headers.entries() API already normalized all header names to lowercase so we can safely index this as 'set-cookie' | ||
| // the Headers API already normalized all header names to lowercase so we can safely index this as 'set-cookie' |
Comment on lines
+354
to
+368
| if (req instanceof Request) { | ||
| return super.match(req, allowPrerenderedRoutes); | ||
| } | ||
| return super.match(req, allowPrerenderedRoutes); | ||
| const { request, url } = createRequestFromNodeRequest(req, { | ||
| skipBody: true, | ||
| }); | ||
| return super.match(request, allowPrerenderedRoutes, url); | ||
| } | ||
|
|
||
| render(request: NodeRequest | Request, options?: RenderOptions): Promise<Response> { | ||
| if (!(request instanceof Request)) { | ||
| request = createRequestFromNodeRequest(request); | ||
| if (request instanceof Request) { | ||
| return super.render(request, options); | ||
| } | ||
| return super.render(request, options); | ||
| const { request: webRequest, url } = createRequestFromNodeRequest(request); | ||
| return super.render(webRequest, { ...options, url }); |
Member
There was a problem hiding this comment.
I don't understand why the logic of this code needed to be changed.
Contributor
Author
There was a problem hiding this comment.
createRequestFromNodeRequest already builds a URL to construct the Request, so I had it return { request, url } and passed that url into match/render to avoid re-parsing it in FetchState. The early return is just to bring url into scope. It depends on the url API, though, so I'll move it to the follow-up PR and
put node.ts back as it was.
Contributor
Author
|
You're right, it's doing too much. I'll split it. I'll keep this PR to the internal changes that don't touch any public API: the I'll move the url-threading part (RenderOptions.url, the optional url argument on I'll also revert the createOutgoingHttpHeaders change here. Does that work for you? |
Keep only the changes internal to astro core and drop the parts that add public API or help a single runtime, so this can land as a patch: - revert the RenderOptions.url / match(url) threading and the adapter wiring (cloudflare, netlify, vercel, node); it'll come back as a separate minor PR - revert createOutgoingHttpHeaders to its original form - keep the domains-* i18n gate, the trailing-slash raw-path reuse, the getParams cleanup, the memory cache provider url reuse, and the domainLookupTable memoization Reword the changeset to match and drop the adapters changeset.
github-actions
Bot
removed
the
pkg: integration
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
The core SSR request path parses
request.urland rebuilds a few small things more often than it has to. This cuts that repeated work on the hot path. Nothing about the rendered output changes, and no public API moves; it's all internal.This started as a bigger PR and was trimmed after review. The cross-adapter URL sharing (a new
RenderOptions.urloption) and thecreateOutgoingHttpHeadersrewrite are out, and the adapter side will come back on its own as aminor. What's left here is internal toastrocore and stays apatch.What changed
Hostheader when adomains-*strategy is configured. That condition is now worked out once in the constructor, so every other app skips the probe, and the URL parse behind it, in bothmatch()andrender().TrailingSlashHandleruses the raw pathname and search thatFetchStatecaptures before it normalizes the URL, instead of parsingrequest.urla second time to get them back.getParamschecks the route's own pattern and then its fallback routes, stopping at the first match, rather than building throwaway arrays with.map().map().find().URLalready sitting oncontext.url(and checks the request method first) instead of parsingcontext.request.urlagain.domainLookupTablekey once and keeps it per table, instead of re-parsing every key on every request.Testing
No behavior change, so nothing new to test. The paths above are covered by the existing unit suites (routing, i18n, app, cache, fetch), which pass, and the typecheck is clean.
Benchmarks
Micro-benchmarks, median of 7 runs on the same machine, for the changes that stayed in this PR:
getParamsmatchingOn a render-heavy page this is lost in the noise, since HTML rendering dominates. It shows up more on light responses like endpoints, redirects and 304s, where per-request setup is a bigger slice of the work.
Investigated and measured with help from Claude Opus 4.8.