-
Notifications
You must be signed in to change notification settings - Fork 365
fix(build): skip unhelpful precompressed variants #2712
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
Changes from all commits
b1ebedd
4927851
3a6168a
bfb3710
309c72d
eaf5715
00f9bc5
4d9626f
bf287c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,13 @@ import fsp from "node:fs/promises"; | |
| import os from "node:os"; | ||
| import path from "pathslash"; | ||
| import zlib from "node:zlib"; | ||
| import { randomUUID } from "node:crypto"; | ||
| import { promisify } from "node:util"; | ||
| import { ASSET_PREFIX_URL_DIR } from "../utils/asset-prefix.js"; | ||
| import { | ||
| isPrecompressedVariantBeneficial, | ||
| type PrecompressedEncoding, | ||
| } from "../utils/precompressed-variant.js"; | ||
|
|
||
| const brotliCompress = promisify(zlib.brotliCompress); | ||
| const gzip = promisify(zlib.gzip); | ||
|
|
@@ -47,7 +52,10 @@ const CONCURRENCY = Math.min(os.availableParallelism(), 8); | |
| type PrecompressResult = { | ||
| filesCompressed: number; | ||
| totalOriginalBytes: number; | ||
| /** Sum of brotli-compressed sizes (used for compression ratio reporting). */ | ||
| /** | ||
| * Sum of the representation sizes Brotli negotiation would serve. Falls | ||
| * back to the original size when Brotli is not beneficial for a file. | ||
| */ | ||
| totalBrotliBytes: number; | ||
| }; | ||
|
|
||
|
|
@@ -126,7 +134,10 @@ export async function precompressAssets( | |
| // readFile already done before this check — stat()-first would save | ||
| // the read for tiny files but costs an extra syscall per file; | ||
| // sub-1KB hashed assets are rare enough that read-first is cheaper. | ||
| if (content.length < MIN_SIZE) return; | ||
| if (content.length < MIN_SIZE) { | ||
| await removeCompressedVariants(fullPath); | ||
| return; | ||
| } | ||
|
|
||
| // Compress all variants concurrently within each file | ||
| const compressions: Promise<Buffer>[] = [ | ||
|
|
@@ -146,20 +157,21 @@ export async function precompressAssets( | |
| const results = await Promise.all(compressions); | ||
| const [brContent, gzContent, zstdContent] = results; | ||
|
|
||
| const writes = [ | ||
| fsp.writeFile(fullPath + ".br", brContent), | ||
| fsp.writeFile(fullPath + ".gz", gzContent), | ||
| ]; | ||
| if (zstdContent) { | ||
| writes.push(fsp.writeFile(fullPath + ".zst", zstdContent)); | ||
| } | ||
| await Promise.all(writes); | ||
| const outcomes = await Promise.all([ | ||
| writeBeneficialVariant(fullPath + ".br", brContent, content.length, "br"), | ||
| writeBeneficialVariant(fullPath + ".gz", gzContent, content.length, "gzip"), | ||
| zstdContent | ||
| ? writeBeneficialVariant(fullPath + ".zst", zstdContent, content.length, "zstd") | ||
| : removeFileIfPresent(fullPath + ".zst").then(() => false), | ||
| ]); | ||
|
|
||
| // Increment counters only after all writes succeed, so partial | ||
| // failures (e.g. ENOSPC mid-write) don't inflate the reported totals. | ||
| result.filesCompressed++; | ||
| result.totalOriginalBytes += content.length; | ||
| result.totalBrotliBytes += brContent.length; | ||
| if (outcomes.some(Boolean)) { | ||
| result.filesCompressed++; | ||
| result.totalOriginalBytes += content.length; | ||
| result.totalBrotliBytes += outcomes[0] ? brContent.length : content.length; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| }), | ||
| ); | ||
| // Report progress once per chunk to avoid non-deterministic ordering | ||
|
|
@@ -172,3 +184,37 @@ export async function precompressAssets( | |
|
|
||
| return result; | ||
| } | ||
|
|
||
| async function writeBeneficialVariant( | ||
| destination: string, | ||
| compressed: Buffer, | ||
| originalSize: number, | ||
| encoding: PrecompressedEncoding, | ||
| ): Promise<boolean> { | ||
| if (!isPrecompressedVariantBeneficial(compressed.length, originalSize, encoding)) { | ||
| await removeFileIfPresent(destination); | ||
| return false; | ||
| } | ||
|
|
||
| const temporary = `${destination}.${process.pid}.${randomUUID()}.tmp`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The temp-then-rename approach is correct for atomicity, but if the process crashes between That matters because the serving side ( Two mitigations to consider: (1) skip
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 3a6168a. The startup cache now excludes only vinext’s exact
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The serve-time guard in Consider a cheap sweep at the start of |
||
| try { | ||
| await fsp.writeFile(temporary, compressed); | ||
| await fsp.rename(temporary, destination); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } catch (error) { | ||
| await removeFileIfPresent(temporary); | ||
| throw error; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| async function removeCompressedVariants(fullPath: string): Promise<void> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On-disk The serve-time guard in
So after a crash between |
||
| await Promise.all([ | ||
| removeFileIfPresent(fullPath + ".br"), | ||
| removeFileIfPresent(fullPath + ".gz"), | ||
| removeFileIfPresent(fullPath + ".zst"), | ||
| ]); | ||
| } | ||
|
|
||
| async function removeFileIfPresent(filePath: string): Promise<void> { | ||
| await fsp.rm(filePath, { force: true }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| import fsp from "node:fs/promises"; | ||
| import path, { toSlash } from "pathslash"; | ||
| import { ASSET_PREFIX_URL_DIR } from "../utils/asset-prefix.js"; | ||
| import { isPrecompressedVariantBeneficial } from "../utils/precompressed-variant.js"; | ||
|
|
||
| /** Content-type lookup for static assets. Shared with prod-server.ts. */ | ||
| export const CONTENT_TYPES: Record<string, string> = { | ||
|
|
@@ -64,6 +65,10 @@ export function contentTypeForPath(filePath: string): string { | |
| */ | ||
| const BUFFER_THRESHOLD = 64 * 1024; | ||
|
|
||
| /** Temp files left by an interrupted atomic precompression write. */ | ||
| const PRECOMPRESSION_TEMP_FILE_RE = | ||
| /\.(?:br|gz|zst)\.\d+\.[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}\.tmp$/i; | ||
|
|
||
| /** A servable file variant with pre-computed response headers. */ | ||
| type FileVariant = { | ||
| /** Absolute file path (used for streaming large files). */ | ||
|
|
@@ -136,8 +141,6 @@ export class StaticFileCache { | |
| // Skip .vite/ internal directory | ||
| if (relativePath.startsWith(".vite/") || relativePath === ".vite") continue; | ||
|
|
||
| const ext = path.extname(relativePath); | ||
| const contentType = contentTypeForPath(relativePath); | ||
| // Files under Vite's `assetsDir` are content-hashed. The default | ||
| // layout writes to `<ASSET_PREFIX_URL_DIR>/` (Next.js's canonical | ||
| // convention); when `assetPrefix` is a path prefix the layout | ||
|
|
@@ -154,6 +157,14 @@ export class StaticFileCache { | |
| const isHashed = | ||
| relativePath.startsWith(`${ASSET_PREFIX_URL_DIR}/`) || | ||
| relativePath.includes(`/${ASSET_PREFIX_URL_DIR}/`); | ||
|
|
||
| // A hard process exit can strand the temporary side of an atomic | ||
| // precompression write. Never register that internal artifact as a URL, | ||
| // but preserve matching user files outside the precompression target. | ||
| if (isHashed && PRECOMPRESSION_TEMP_FILE_RE.test(relativePath)) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tmp-artifact guard is gated on |
||
|
|
||
| const ext = path.extname(relativePath); | ||
| const contentType = contentTypeForPath(relativePath); | ||
| const cacheControl = isHashed | ||
| ? "public, max-age=31536000, immutable" | ||
| : "public, max-age=3600"; | ||
|
|
@@ -190,17 +201,17 @@ export class StaticFileCache { | |
|
|
||
| // Pre-compute compressed variant headers (with Content-Encoding, Vary, correct Content-Length) | ||
| const brInfo = allFiles.get(relativePath + ".br"); | ||
| if (brInfo) { | ||
| if (brInfo && isPrecompressedVariantBeneficial(brInfo.size, fileInfo.size, "br")) { | ||
| entry.br = buildVariant(brInfo, baseHeaders, "br"); | ||
| } | ||
|
|
||
| const gzInfo = allFiles.get(relativePath + ".gz"); | ||
| if (gzInfo) { | ||
| if (gzInfo && isPrecompressedVariantBeneficial(gzInfo.size, fileInfo.size, "gzip")) { | ||
| entry.gz = buildVariant(gzInfo, baseHeaders, "gzip"); | ||
| } | ||
|
|
||
| const zstInfo = allFiles.get(relativePath + ".zst"); | ||
| if (zstInfo) { | ||
| if (zstInfo && isPrecompressedVariantBeneficial(zstInfo.size, fileInfo.size, "zstd")) { | ||
| entry.zst = buildVariant(zstInfo, baseHeaders, "zstd"); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| export type PrecompressedEncoding = "br" | "gzip" | "zstd"; | ||
|
|
||
| const VARY_HEADER_BYTES = "Vary: Accept-Encoding\r\n".length; | ||
| const RESPONSE_HEADER_OVERHEAD: Record<PrecompressedEncoding, number> = { | ||
| br: "Content-Encoding: br\r\n".length + VARY_HEADER_BYTES, | ||
| gzip: "Content-Encoding: gzip\r\n".length + VARY_HEADER_BYTES, | ||
| zstd: "Content-Encoding: zstd\r\n".length + VARY_HEADER_BYTES, | ||
| }; | ||
|
|
||
| /** | ||
| * Return whether an encoded representation reduces conservative HTTP/1 wire | ||
| * bytes after accounting for the response fields needed to negotiate it. | ||
| */ | ||
| export function isPrecompressedVariantBeneficial( | ||
| compressedSize: number, | ||
| originalSize: number, | ||
| encoding: PrecompressedEncoding, | ||
| ): boolean { | ||
| return originalSize - compressedSize > RESPONSE_HEADER_OVERHEAD[encoding]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The overhead model counts both
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The overhead model charges the variant for a full |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,15 +11,32 @@ import fs from "node:fs"; | |
| import path from "node:path"; | ||
| import os from "node:os"; | ||
| import zlib from "node:zlib"; | ||
| import { createHash } from "node:crypto"; | ||
| import { precompressAssets } from "../packages/vinext/src/build/precompress.js"; | ||
| import { isPrecompressedVariantBeneficial } from "../packages/vinext/src/utils/precompressed-variant.js"; | ||
|
|
||
| /** Write a file with repeated content to ensure it exceeds compression threshold. */ | ||
| async function writeAsset(clientDir: string, relativePath: string, content: string): Promise<void> { | ||
| async function writeAsset( | ||
| clientDir: string, | ||
| relativePath: string, | ||
| content: string | Uint8Array, | ||
| ): Promise<void> { | ||
| const fullPath = path.join(clientDir, relativePath); | ||
| await fsp.mkdir(path.dirname(fullPath), { recursive: true }); | ||
| await fsp.writeFile(fullPath, content); | ||
| } | ||
|
|
||
| function deterministicHighEntropyBytes(size: number): Buffer { | ||
| const chunks: Buffer[] = []; | ||
| let bytes = 0; | ||
| for (let index = 0; bytes < size; index++) { | ||
| const chunk = createHash("sha256").update(`vinext-precompress-${index}`).digest(); | ||
| chunks.push(chunk); | ||
| bytes += chunk.length; | ||
| } | ||
| return Buffer.concat(chunks).subarray(0, size); | ||
| } | ||
|
|
||
| describe("precompressAssets", () => { | ||
| let clientDir: string; | ||
|
|
||
|
|
@@ -82,6 +99,64 @@ describe("precompressAssets", () => { | |
| expect(result.filesCompressed).toBe(0); | ||
| }); | ||
|
|
||
| it("does not emit compressed representations that are not smaller", async () => { | ||
| const content = deterministicHighEntropyBytes(4096); | ||
| await writeAsset(clientDir, "_next/static/random-aaa111.wasm", content); | ||
|
|
||
| const result = await precompressAssets(clientDir); | ||
|
|
||
| expect(result.filesCompressed).toBe(0); | ||
| expect(fs.existsSync(path.join(clientDir, "_next/static/random-aaa111.wasm.br"))).toBe(false); | ||
| expect(fs.existsSync(path.join(clientDir, "_next/static/random-aaa111.wasm.gz"))).toBe(false); | ||
| expect(fs.existsSync(path.join(clientDir, "_next/static/random-aaa111.wasm.zst"))).toBe(false); | ||
| }); | ||
|
|
||
| it("does not emit a marginally smaller representation with larger wire overhead", async () => { | ||
| const content = deterministicHighEntropyBytes(4096); | ||
| content.copy(content, content.length - 48, 0, 48); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test depends on brotli q5 output for this specific input landing in the narrow "smaller than original but within header overhead" window. It's guarded by the explicit assertions on lines 120-121, so it will fail loudly (not silently pass) if a zlib/brotli version bump shifts the output size out of that window — which is the right call. Just noting it may need a nudge to the input/mutation if brotli's behavior changes across Node versions in CI. |
||
| const brotli = zlib.brotliCompressSync(content, { | ||
| params: { [zlib.constants.BROTLI_PARAM_QUALITY]: 5 }, | ||
| }); | ||
| expect(brotli.length).toBeLessThan(content.length); | ||
| expect(isPrecompressedVariantBeneficial(brotli.length, content.length, "br")).toBe(false); | ||
|
|
||
| const relativePath = "_next/static/marginal-aaa111.wasm"; | ||
| await writeAsset(clientDir, relativePath, content); | ||
| await precompressAssets(clientDir); | ||
|
|
||
| expect(fs.existsSync(path.join(clientDir, relativePath + ".br"))).toBe(false); | ||
| }); | ||
|
|
||
| it("removes obsolete compressed representations on a later run", async () => { | ||
| const relativePath = "_next/static/changing-aaa111.wasm"; | ||
| const fullPath = path.join(clientDir, relativePath); | ||
| await writeAsset(clientDir, relativePath, "compressible\n".repeat(500)); | ||
| await precompressAssets(clientDir); | ||
| expect(fs.existsSync(fullPath + ".br")).toBe(true); | ||
|
|
||
| await writeAsset(clientDir, relativePath, deterministicHighEntropyBytes(4096)); | ||
| await precompressAssets(clientDir); | ||
|
|
||
| expect(fs.existsSync(fullPath + ".br")).toBe(false); | ||
| expect(fs.existsSync(fullPath + ".gz")).toBe(false); | ||
| expect(fs.existsSync(fullPath + ".zst")).toBe(false); | ||
| }); | ||
|
|
||
| it("removes obsolete variants when an asset falls below the size threshold", async () => { | ||
| const relativePath = "_next/static/shrinking-aaa111.js"; | ||
| const fullPath = path.join(clientDir, relativePath); | ||
| await writeAsset(clientDir, relativePath, "const value = 1;\n".repeat(500)); | ||
| await precompressAssets(clientDir); | ||
| expect(fs.existsSync(fullPath + ".br")).toBe(true); | ||
|
|
||
| await writeAsset(clientDir, relativePath, "const value = 1;"); | ||
| await precompressAssets(clientDir); | ||
|
|
||
| expect(fs.existsSync(fullPath + ".br")).toBe(false); | ||
| expect(fs.existsSync(fullPath + ".gz")).toBe(false); | ||
| expect(fs.existsSync(fullPath + ".zst")).toBe(false); | ||
| }); | ||
|
|
||
| it("skips non-compressible file types (images, fonts)", async () => { | ||
| // PNG file (binary, already compressed) | ||
| const pngHeader = Buffer.alloc(2048, 0x89); // fake PNG data, above threshold | ||
|
|
@@ -229,3 +304,17 @@ describe("precompressAssets", () => { | |
| expect(result.totalBrotliBytes).toBeLessThan(result.totalOriginalBytes); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isPrecompressedVariantBeneficial", () => { | ||
| it.each([ | ||
| ["br", 45], | ||
| ["gzip", 47], | ||
| ["zstd", 47], | ||
| ] as const)( | ||
| "requires %s to save more than its response-header overhead", | ||
| (encoding, overhead) => { | ||
| expect(isPrecompressedVariantBeneficial(1000 - overhead, 1000, encoding)).toBe(false); | ||
| expect(isPrecompressedVariantBeneficial(999 - overhead, 1000, encoding)).toBe(true); | ||
| }, | ||
| ); | ||
| }); | ||
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.
Minor semantics change worth confirming is intentional:
filesCompressednow increments when any variant is beneficial, andtotalBrotliBytesfalls back tocontent.lengthwhen brotli specifically isn't beneficial. In the edge case where brotli is never beneficial for any file but gzip/zstd are,totalBrotliBytes === totalOriginalBytes, so the summary line inindex.ts:6546prints0.0% smaller with brotliwhile still reporting a nonzerofilesCompressed. Not a bug, but the label may read oddly. Consider reporting the best-variant ratio, or renaming the metric, if you want the summary to stay meaningful in that case.