perf: build-time precompression + startup metadata cache for static serving#641
perf: build-time precompression + startup metadata cache for static serving#641NathanDrake2406 wants to merge 7 commits intocloudflare:mainfrom
Conversation
eca889d to
1798289
Compare
commit: |
1798289 to
2fd407c
Compare
Generate .br (brotli q11), .gz (gzip l9), and .zst (zstandard l19) files alongside compressible assets in dist/client/assets/ during vinext build. These are served directly by the production server, eliminating per-request compression overhead for immutable build output. Only targets assets/ (hashed, immutable). Public directory files still use on-the-fly compression since they may change between deploys.
StaticFileCache walks dist/client/ once at server boot and caches: - File metadata (path, size, content-type, cache-control, etag) - Pre-computed response headers per variant (original, br, gz, zst) - In-memory buffers for small files (< 64KB) for res.end(buffer) - Precompressed variant paths and sizes Per-request serving is Map.get() + res.end(buffer) with zero filesystem calls, zero object allocation, and zero header construction. Modeled after sirv's production mode but with in-memory buffering for small files which eliminates createReadStream fd overhead.
Refactor tryServeStatic to use StaticFileCache for the hot path: - Pre-computed response headers (zero object allocation per request) - In-memory buffer serving for small precompressed files - 304 Not Modified via ETag + If-None-Match - HEAD request optimization (headers only, no body) - Zstandard serving (zstd > br > gzip > original fallback chain) - Async filesystem fallback for non-cached files (replaces blocking existsSync + statSync) - Skip decodeURIComponent for clean URLs (no % in path) Wire StaticFileCache.create() into both startAppRouterServer and startPagesRouterServer at startup. Integrate precompressAssets() into the vinext build pipeline. CONTENT_TYPES is now a single source of truth exported from static-file-cache.ts (was duplicated in prod-server.ts).
75a0c8f to
df77821
Compare
|
Nice idea! Thanks for the PR! I wonder if we should gate this behind an option instead of being on-by-default, considering that Next.js doesn't do this... I wonder if compression level is something we should let users control as well. It would be good to get a progress bar like we have in pre-rendering too if there isn't one already. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Build-time precompression + startup metadata cache
Strong PR overall. The architecture (build-time precompression → startup cache → zero-allocation hot path) is sound, the test coverage is thorough (52 new tests), and the performance wins are real. The code quality is high — clear separation of concerns between precompress.ts, static-file-cache.ts, and the refactored tryServeStatic.
I have a few issues to flag, ranging from a correctness bug to design questions.
Key issues
-
Missing
Vary: Accept-Encodingon original variant when compressed variants exist — When a file has.br/.gz/.zstvariants but the client doesn't accept any compression, theoriginalvariant is served without aVaryheader. This means caches between the server and client could cache the uncompressed response and serve it to clients that do accept compression. The compressed variant headers correctly includeVary: Accept-Encoding, but the original variant needs it too when sibling compressed variants exist. -
Mtime-based ETag is fragile for containerized/CI deployments —
W/"${size}-${mtimeMs}"uses the file modification time. In Docker builds, CI artifact uploads, orcp -rwithout--preserve, mtime changes between deploys even when file content is identical. This causes unnecessary cache invalidation for returning users. Hashed assets (which are the primary target) have content hashes in their filenames so this is less impactful in practice, but for non-hashed public directory files it matters. A content-hash-based ETag (or at least using the filename hash when available) would be more robust. -
Maintainer feedback on opt-in — @james-elicx raised making this opt-in/configurable. The precompression step adds meaningful build time (brotli q11 is slow), and since Next.js doesn't do this, it could surprise users. Worth addressing before merge.
See inline comments for specifics.
| const original: FileVariant = { | ||
| path: fileInfo.fullPath, | ||
| headers: { ...baseHeaders, "Content-Length": String(fileInfo.size) }, | ||
| }; |
There was a problem hiding this comment.
Bug: Missing Vary: Accept-Encoding on the original variant when compressed siblings exist.
When a file has .br/.gz/.zst precompressed variants, the original variant's headers omit Vary: Accept-Encoding. If a CDN or shared cache sits between the server and client, it could cache this uncompressed response and serve it to clients that accept compression (or vice versa).
The fix: when compressed variants exist for a file, add Vary: Accept-Encoding to the original variant's headers too.
| }; | |
| const original: FileVariant = { | |
| path: fileInfo.fullPath, | |
| headers: { ...baseHeaders, "Content-Length": String(fileInfo.size) }, | |
| }; |
After the compressed variant detection block (after line ~180), add:
if (entry.br || entry.gz || entry.zst) {
original.headers["Vary"] = "Accept-Encoding";
}
| const cacheControl = isHashed | ||
| ? "public, max-age=31536000, immutable" | ||
| : "public, max-age=3600"; | ||
| const etag = `W/"${fileInfo.size}-${fileInfo.mtimeMs}"`; |
There was a problem hiding this comment.
Mtime-based ETag is fragile in container/CI deployments.
W/"${size}-${mtimeMs}" changes whenever the file is copied without preserving timestamps (common in Docker multi-stage builds, CI artifact caching, rsync without -t, etc.). For hashed assets this is mitigated by the immutable cache-control + filename hash, but for non-hashed public directory files (favicon.ico, robots.txt, etc.) this means unnecessary 200s instead of 304s across deploys.
Consider:
- For files under
assets/(already content-hashed): extract the hash from the filename for the ETag, or just use the size (since the filename changes on content change anyway). - For other files: a fast hash of the file content (e.g., the first 8 bytes of a SHA-256, hex-encoded) would be more stable.
Not a blocker since sirv uses the same strategy, but worth noting as a follow-up improvement.
| if (entry.isDirectory()) { | ||
| yield* walkFilesWithStats(fullPath, base); | ||
| } else if (entry.isFile()) { | ||
| const stat = await fsp.stat(fullPath); |
There was a problem hiding this comment.
Unnecessary sequential stat() — could batch for faster startup.
readdir({ withFileTypes: true }) doesn't provide size/mtimeMs, so the stat() call is indeed necessary. However, since each stat() is awaited sequentially inside the for loop, a directory with 500 files would make 500 sequential syscalls during startup.
Consider collecting the file paths from readdir and batching the stat() calls with Promise.all (perhaps in chunks of 50-100 to avoid fd exhaustion). This would meaningfully reduce startup latency for large dist/client/ directories.
Non-blocking — the current approach is correct, just slower than it needs to be.
|
|
||
| // Pick the best precompressed variant: zstd → br → gzip → original. | ||
| // Each variant has pre-computed headers — zero string building. | ||
| // Accept-Encoding is already lowercase in Node.js IncomingMessage. |
There was a problem hiding this comment.
Nit: Comment is incorrect. Accept-Encoding is NOT guaranteed to be lowercase in Node.js IncomingMessage. Node.js lowercases header names (keys), but header values preserve their original casing. So req.headers["accept-encoding"] gives you the value as-sent by the client.
In practice this is fine because .includes("zstd") etc. will match the standard lowercase tokens that browsers send. But the comment is misleading.
| // Accept-Encoding is already lowercase in Node.js IncomingMessage. | |
| // Accept-Encoding header values from browsers use lowercase tokens. |
| if (variant.buffer) { | ||
| res.end(variant.buffer); | ||
| } else { | ||
| fs.createReadStream(variant.path).pipe(res); |
There was a problem hiding this comment.
No error handling on createReadStream().pipe().
If the file is deleted between startup (when it was cached) and request time, createReadStream will emit an error that crashes the connection without a proper HTTP response. The slow path at line 490 uses pipeline() which handles errors; this fast path should too.
| fs.createReadStream(variant.path).pipe(res); | |
| pipeline(fs.createReadStream(variant.path), res, () => {}); |
(Or alternatively, wrap in a try/catch and send a 500 if the stream errors.)
|
|
||
| const brotliCompress = promisify(zlib.brotliCompress); | ||
| const gzip = promisify(zlib.gzip); | ||
| const zstdCompress = promisify(zlib.zstdCompress); |
There was a problem hiding this comment.
Node.js version concern: zlib.zstdCompress is experimental before Node 22.15.
The repo requires engines.node >= 22, but zlib.zstdCompress was experimental until Node 22.15.0 (May 2025). Users on Node 22.0–22.14 will get ExperimentalWarning runtime warnings during builds.
Options:
- Bump
engines.nodeto>=22.15(aligns with stable zstd) - Wrap in try/catch and gracefully skip
.zstgeneration if the API isn't available or is experimental - Accept the warnings (since the API does work)
Worth documenting the decision either way.
| totalCompressedBytes: 0, | ||
| }; | ||
|
|
||
| const compressionWork: Promise<void>[] = []; |
There was a problem hiding this comment.
Unbounded concurrency for large asset directories.
All compression work is pushed to compressionWork and then awaited with Promise.all. For a large app with hundreds of assets, this means hundreds of concurrent brotli q11 compressions (CPU-intensive) running simultaneously, which could spike memory usage and starve the event loop.
Consider limiting concurrency (e.g., process 8-16 files at a time). A simple approach:
const CONCURRENCY = Math.min(os.cpus().length, 16);with a semaphore or chunked processing.
packages/vinext/src/cli.ts
Outdated
| // .br/.gz/.zst files are served directly by the production server — | ||
| // zero per-request compression overhead for immutable build output. | ||
| const clientDir = path.resolve("dist", "client"); | ||
| const precompressResult = await precompressAssets(clientDir); |
There was a problem hiding this comment.
No opt-out mechanism. As @james-elicx noted, this runs unconditionally on every build. For users who deploy behind a CDN that handles compression (e.g., Cloudflare), the build-time compression adds overhead with no benefit. Brotli q11 is particularly slow — a large app with many JS bundles could add 10+ seconds to build time.
Suggestions:
- Add a CLI flag:
vinext build --no-precompress - Add a plugin option:
vinext({ precompress: false })in vite.config.ts - Default to enabled but respect a config option to disable
The Cloudflare Workers deployment target is particularly relevant here — Cloudflare's CDN handles compression at the edge, so precompressing for the Node.js prod server doesn't help Workers users at all.
| if (!entry) return false; | ||
|
|
||
| // 304 Not Modified: string compare against pre-computed ETag | ||
| if (req.headers["if-none-match"] === entry.etag) { |
There was a problem hiding this comment.
304 response includes headers that should arguably be excluded.
The notModifiedHeaders include Content-Type, which RFC 9110 says SHOULD NOT be sent on a 304. In practice browsers handle this fine, but it's worth being aware of. The test at line 362 of serve-static.test.ts correctly verifies that Content-Length is absent, which is good.
Non-blocking, but for strict RFC compliance, notModifiedHeaders should only include ETag, Cache-Control, Vary, and Date.
| /** Zstandard precompressed variant, if .zst file exists. */ | ||
| zst?: FileVariant; | ||
|
|
||
| // Legacy accessors for backwards compatibility with tests |
There was a problem hiding this comment.
"Legacy accessors" smell — consider removing if this is new code.
These fields (resolvedPath, size, contentType, cacheControl, brPath, brSize, gzPath, gzSize, zstPath, zstSize) duplicate information already available through the original/br/gz/zst variant objects. The comment says they're for "backwards compatibility with tests," but since the tests in this PR are all new, there is no backwards compatibility requirement.
Removing them would simplify the interface and eliminate the dual source of truth. The tests in static-file-cache.test.ts could be updated to access entry.original.headers["Content-Length"] instead of entry.size, etc.
|
Review posted on PR #641. Here's a summary of what I flagged: Correctness issues:
Design/robustness concerns: Code quality: |
- Add Vary: Accept-Encoding to original variant when compressed siblings exist - Use pipeline() instead of .pipe() for stream error safety - Remove legacy accessor fields from StaticFileEntry (all new code) - Exclude Content-Type from 304 responses per RFC 9110 - Fix misleading Accept-Encoding comment in prod-server - Batch stat() calls in walkFilesWithStats (chunks of 64) - Bound precompression concurrency to availableParallelism (max 16) - Gracefully skip zstd when zlib.zstdCompress unavailable (Node <22.15) - Add --no-precompress CLI flag to skip build-time compression - Drop brotli to q5 (fast, nearly same ratio as q11 at build time) - Max out zstd at level 22 (build time, no reason to hold back)
I added a CLI option for it. Tbh I was just in the mood to optimise and half way through I realised it doesn't matter for Workers anyway lmao. It's okay tho, someday when a dev who isn't on Workers will benefit form it :copium: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Build-time precompression + startup metadata cache
Strong contribution. The architecture is well-designed — build-time compression, startup cache, zero-allocation hot path — and the previous review's main correctness issue (missing Vary on original variant, batched stat() calls) has been addressed. Test coverage is thorough (52 new tests). I have a few remaining issues, one of which is a correctness bug.
Issues
1. Doc comment / constant mismatch on compression levels (misleading, multiple locations)
The file header comment in precompress.ts says "brotli q5" and "zstd l22", and the CLI comment says "brotli q5 + gzip l9 + zstd l22", but the PR title and description claim "brotli q11" and "zstd l19". The actual code uses brotli q5 and zstd l22. The PR description benchmarks cite "brotli q11" results. Either the code or the description is wrong — this needs to be reconciled so users know what quality level they're getting.
2. createZstdCompress called unconditionally in createCompressor — crashes on Node < 22.15
The build-time code in precompress.ts correctly feature-detects zlib.zstdCompress before using it. But the runtime createCompressor function in prod-server.ts calls zlib.createZstdCompress() unconditionally in the "zstd" case. If negotiateEncoding returns "zstd" on a Node version where zstd isn't available (or is experimental), this will throw at runtime. The negotiation should check zstd availability before advertising support.
3. pipeline error callback silently swallows errors on the fast path
At prod-server.ts:447, pipeline(fs.createReadStream(variant.path), res, () => {}) silently ignores all errors. If the file is deleted between cache creation and request time, createReadStream emits an error that never gets handled — the client gets a broken connection with no HTTP error response. The slow path and the compressed-response path both use the same pattern, but those at least have compressor in the pipeline which triggers a res close. The fast path (buffer miss, large file) should handle errors — at minimum log, ideally send 500 if headers haven't been sent.
4. Slow path doesn't serve HEAD correctly
The fast path (with cache) correctly returns headers-only for HEAD requests. The slow path (no cache, lines 452-500) has no HEAD check — it always streams the full body. This means if StaticFileCache.create() ever returns an empty cache (race, error), HEAD requests regress to streaming the full file.
5. resolvedClient traversal check is skipped on the fast path
The fast path trusts the cache completely — if a pathname is in the cache, it's served. But the cache is keyed by "/{relativePath}" where relativePath comes from path.relative(base, fullPath). If walkFilesWithStats somehow yields a path outside clientDir (e.g., symlink), the traversal guard is bypassed. Low risk since readdir + isFile() won't follow cross-device symlinks by default, but worth noting — the slow path has an explicit startsWith(resolvedClient + path.sep) check.
| /** | ||
| * Build-time precompression for hashed static assets. | ||
| * | ||
| * Generates .br (brotli q5), .gz (gzip l9), and .zst (zstd l22) files |
There was a problem hiding this comment.
Doc/code mismatch on compression levels. The header says "brotli q5" and "zstd l22", but the PR title and description benchmarks claim "brotli q11" and "zstd l19". The actual code uses BROTLI_PARAM_QUALITY: 5 (line 117) and ZSTD_c_compressionLevel: 22 (line 124).
The PR description's benchmark results cite "brotli q11, max quality" — if those numbers were measured with q11, they don't reflect what this code actually produces. Either:
- Update the code to use q11/l19 to match the PR description, or
- Update the PR description to accurately reflect q5/l22
q5 is a reasonable default (much faster than q11), but the discrepancy between docs and code will confuse anyone reading the PR description alongside the source.
| const accept = req.headers["accept-encoding"]; | ||
| if (!accept || typeof accept !== "string") return null; | ||
| const lower = accept.toLowerCase(); | ||
| if (lower.includes("zstd")) return "zstd"; |
There was a problem hiding this comment.
Bug: negotiateEncoding advertises zstd without checking runtime availability.
This returns "zstd" if the client sends it in Accept-Encoding, but createCompressor then calls zlib.createZstdCompress() unconditionally. On Node versions where zstd isn't available (or is experimental and disabled), this will throw.
The build-time code in precompress.ts correctly feature-detects zstdCompress before using it. The same pattern should apply here:
| if (lower.includes("zstd")) return "zstd"; | |
| if (lower.includes("zstd") && typeof zlib.createZstdCompress === "function") return "zstd"; |
Alternatively, hoist a const HAS_ZSTD = typeof zlib.createZstdCompress === "function" constant and use it in both negotiateEncoding and createCompressor.
| if (variant.buffer) { | ||
| res.end(variant.buffer); | ||
| } else { | ||
| pipeline(fs.createReadStream(variant.path), res, () => {}); |
There was a problem hiding this comment.
Error swallowed silently — broken connections on file-not-found.
If the cached file is deleted between startup and request time (e.g., a deploy that replaces dist/ while the server is running), createReadStream emits an error. The empty callback () => {} silently discards it, and the client gets a broken/hung connection with no HTTP response.
Consider:
| pipeline(fs.createReadStream(variant.path), res, () => {}); | |
| pipeline(fs.createReadStream(variant.path), res, (err) => { | |
| if (err && !res.headersSent) { | |
| res.writeHead(500); | |
| res.end("Internal Server Error"); | |
| } | |
| }); |
Note: res.headersSent will be true here since writeHead was called above. So the realistic fix is to just not crash silently — at minimum a console.error so operators can diagnose the issue. The same pattern exists on line 490 and 499 in the slow path.
|
|
||
| res.writeHead(200, baseHeaders); | ||
| fs.createReadStream(resolvedStaticFile).pipe(res); | ||
| res.writeHead(200, { |
There was a problem hiding this comment.
Slow path missing HEAD request handling.
The fast path (cache hit) correctly returns headers-only for HEAD requests at line 437-440. The slow path here always streams the full body regardless of req.method. If the cache is empty or unavailable, HEAD requests will stream the entire file.
| res.writeHead(200, { | |
| res.writeHead(200, { | |
| ...baseHeaders, | |
| "Content-Length": String(resolved.size), | |
| }); | |
| if (req.method === "HEAD") { | |
| res.end(); | |
| return true; | |
| } | |
| pipeline(fs.createReadStream(resolved.path), res, () => {}); | |
| return true; |
|
|
||
| // Pick the best precompressed variant: zstd → br → gzip → original. | ||
| // Each variant has pre-computed headers — zero string building. | ||
| // Accept-Encoding header values from browsers use lowercase tokens. |
There was a problem hiding this comment.
Nit: the comment says "Accept-Encoding header values from browsers use lowercase tokens" which is accurate in practice but slightly misleading — the HTTP spec says encoding tokens are case-insensitive, and browsers happen to send lowercase. The code handles this correctly by using .includes("zstd") etc. on the raw value (which browsers always send lowercase). Just noting for accuracy since the earlier review flagged the same comment.
| const encoding = negotiateEncoding(req); | ||
| if (encoding) { | ||
| const fileStream = fs.createReadStream(resolvedStaticFile); | ||
| const compressor = createCompressor(encoding); |
There was a problem hiding this comment.
Slow path also missing HEAD handling for compressed responses.
When the slow path serves a compressed response, it unconditionally pipes the compressed stream. HEAD requests should get headers only.
| const compressor = createCompressor(encoding); | |
| const compressor = createCompressor(encoding); | |
| res.writeHead(200, { | |
| ...baseHeaders, | |
| "Content-Encoding": encoding, | |
| Vary: "Accept-Encoding", | |
| }); | |
| if (req.method === "HEAD") { | |
| res.end(); | |
| return true; | |
| } | |
| pipeline(fs.createReadStream(resolved.path), compressor, res, () => {}); |
| prerenderResult = await runPrerender({ root: process.cwd() }); | ||
| } | ||
|
|
||
| // Precompress hashed assets (brotli q5 + gzip l9 + zstd l22). These |
There was a problem hiding this comment.
Same doc/code mismatch: comment says "brotli q5 + gzip l9 + zstd l22" but the PR description benchmarks claim "brotli q11". Should match whatever the actual values are in precompress.ts.
| continue; | ||
|
|
||
| const fullPath = path.join(assetsDir, relativePath); | ||
| const content = await fsp.readFile(fullPath); |
There was a problem hiding this comment.
Minor: all file contents are eagerly read into memory before compression begins. For a project with many large compressible assets, this could spike memory. Consider reading and compressing files within the chunked loop instead of pre-reading all of them. Currently:
- Read ALL files into
files[]array (potentially hundreds of MB) - Then compress in chunks
If you move the readFile into the chunk processing loop, peak memory drops from O(total_assets) to O(CONCURRENCY * max_file_size).
| const cacheControl = isHashed | ||
| ? "public, max-age=31536000, immutable" | ||
| : "public, max-age=3600"; | ||
| const etag = `W/"${fileInfo.size}-${fileInfo.mtimeMs}"`; |
There was a problem hiding this comment.
Previous review flagged mtime-based ETag fragility in containerized deployments. This is still using W/"${size}-${mtimeMs}". Since this PR already addressed other review feedback (Vary on original, batched stats), noting this as a known follow-up — it's not a blocker since sirv uses the same approach, but worth tracking.
| await captured.ended; | ||
| expect(captured.headers["Content-Encoding"]).toBe("br"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Good coverage overall. Two test gaps to consider adding:
-
Slow path test (no cache): All tests pass a
StaticFileCachetotryServeStatic. There are no tests for the cache-less fallback path (lines 452-500 of prod-server.ts), which has the HEAD and error handling issues I flagged. At minimum, one test callingtryServeStatic(req, res, clientDir, pathname, compress)without a cache argument would exercise that path. -
Encoded pathname test: The fast path has special handling for
%-encoded pathnames (lines 391-403), including adecodeURIComponentthat can throw. A test with a malformed percent-encoded pathname (e.g.,"/%E0%A4%A") would verify thecatchreturnsfalsegracefully.
|
Review posted. Here's a summary of what I flagged: Key issues
Positive notes
|
|
@james-elicx should I use 11 or 5? |
|
Higher level of compression would be good at built-time but 11 can also be a bit slow, so 5 is probably a good starting point. |
|
Can the CLI flag be opt-in instead of opt-out as well? |
This PR implements a static file serving method that is better than Next.js 16.2.
Bugs fixed
existsSync+statSyncran on every static file request, blocking SSR responses behind synchronous filesystem calls. Now zero FS calls per request (metadata cached at startup).If-None-Matchsupport. Every repeat visit re-downloaded every asset in full. Now returns 304 (200 bytes) when the browser already has the asset.Optimizations
.br(brotli q11),.gz(gzip l9),.zst(zstd l19) generated at build time. Zero compression CPU per request.res.end(buffer)instead ofcreateReadStream().pipe(), eliminating file descriptor overhead..zstassets. 3-5x faster client-side decompression than brotli (Chrome 123+, Firefox 126+).fsp.stat()instead of blockingstatSync.Real-world impact
Benchmark
Averaged over 10 runs. 5000 requests per run, 10 concurrent connections, 5 × 50KB JS bundles. vinext NEW won every single run.
Throughput (Accept-Encoding: zstd, br, gzip)
sirv3.0.2send1.2.1304 Not Modified (conditional request with matching ETag)
sirv3.0.2Transfer size (5000 requests)
sirvsendvinext serves zstd when accepted (trades ~12% larger output for 3-5x faster client-side decompression). When serving brotli, vinext produces the smallest output of all — brotli q11 (93 B) vs old vinext's q4 (94 B).
Feature comparison
sirvsendstat)existsSync+statSync)Why vinext beats sirv
sirv always uses
createReadStream().pipe()— even for a 100-byte precompressed file, this opens a file descriptor, creates a ReadStream, sets up pipe plumbing, reads 100 bytes, and closes the fd. vinext buffers small files (< 64KB) in memory at startup and serves them withres.end(buffer)— a single write to the socket. For large files, it still streams. Best of both worlds.Architecture
New modules:
src/build/precompress.ts— build-time compression (brotli q11, gzip l9, zstd l19)src/server/static-file-cache.ts— startup cache with pre-computed headers + in-memory buffersprod-server.tsrefactoredtryServeStatic— async, cache-aware, precompressed variant servingTest plan
precompressAssets: generates .br/.gz/.zst, skips small files, skips non-compressible types, handles missing dirs, idempotent, correct decompression (13 tests)StaticFileCache: scan, lookup, HTML fallbacks, .vite/ blocking, etag, variant detection, nested dirs (20 tests)tryServeStatic: precompressed serving (zstd/br/gz fallback chain), 304 Not Modified, HEAD, Content-Length, Vary, extra headers, traversal protection (19 tests)