-
Notifications
You must be signed in to change notification settings - Fork 4.9k
build: enable LTO for bun-zig.o #29618
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
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 |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| import { mkdir, readdir, rename, rm, writeFile } from "node:fs/promises"; | ||
| import { availableParallelism, homedir } from "node:os"; | ||
| import { join, resolve } from "node:path"; | ||
| import type { Config, OS } from "./config.ts"; | ||
| import type { Config } from "./config.ts"; | ||
| import { downloadWithRetry, extractZip } from "./download.ts"; | ||
| import { assert } from "./error.ts"; | ||
| import { fetchCliPath } from "./fetch-cli.ts"; | ||
|
|
@@ -31,38 +31,8 @@ | |
| * Zig compiler commit — determines compiler download + bundled stdlib. | ||
| * Override via `--zig-commit=<hash>` to test a new compiler. | ||
| * From https://github.com/oven-sh/zig releases. | ||
| * | ||
| * TEMPORARY SPLIT: ZIG_COMMIT is the pre-parallel-sema compiler, kept | ||
| * for Windows hosts only (COFF shard emission isn't implemented and | ||
| * the build path needs a single object). Everything else — local and | ||
| * CI, all targets — uses ZIG_COMMIT_PARALLEL (parallel sema is | ||
| * deterministic; codegen-unit count is decided separately by | ||
| * codegenThreads()). Once Windows is supported, collapse both back to | ||
| * one constant. | ||
| */ | ||
| export const ZIG_COMMIT = "365343af4fc5a1a632e6b54aadd0b87be30edd81"; | ||
| export const ZIG_COMMIT_PARALLEL = "0bcf4c3d998133e724d27e9fd783172ffed4c943"; | ||
|
|
||
| /** | ||
| * The one place that picks which compiler to use. The parallel compiler | ||
| * is used everywhere except Windows (its sharded-codegen object emission | ||
| * for COFF is unimplemented). Parallel SEMA is deterministic and changes | ||
| * no output, so CI gets it too — only the codegen-unit count differs by | ||
| * config (see codegenThreads()). | ||
| */ | ||
| export function defaultZigCommit(hostOs: OS): string { | ||
| if (hostOs === "windows") return ZIG_COMMIT; | ||
| return ZIG_COMMIT_PARALLEL; | ||
| } | ||
|
|
||
| /** | ||
| * True iff `cfg` is using the parallel-sema compiler. Gates | ||
| * ZIG_PARALLEL_SEMA and the `llvm_no_merge_shards` build.zig path — | ||
| * the stable compiler doesn't understand either. | ||
| */ | ||
| function usingParallelCompiler(cfg: Config): boolean { | ||
| return cfg.zigCommit !== ZIG_COMMIT; | ||
| } | ||
| export const ZIG_COMMIT = "04e7f6ac1e009525bc00934f20199c68f04e0a24"; | ||
|
|
||
| /** | ||
| * Number of LLVM codegen units. >1 splits the build into N independent | ||
|
|
@@ -73,15 +43,17 @@ | |
| * - Non-ASAN CI: shipped releases want full IPO; cg=1 keeps that and | ||
| * keeps the upload/download contract a single file. | ||
| * - Windows targets: COFF shard emission is unimplemented in oven-sh/zig. | ||
| * - LTO: zig_llvm.cpp gates SplitModule on !lto, so cg>1 would emit one | ||
| * .o instead of N and the no_merge_shards path would expect missing files. | ||
| * | ||
| * ASAN CI uses a FIXED count (CI_ASAN_CODEGEN_THREADS) so zig-only and | ||
| * link-only — which run on different machines — agree on the artifact | ||
| * names. Local builds shard at availableParallelism(); benchmark against | ||
| * a non-ASAN CI artifact if cross-unit inlining matters. | ||
| */ | ||
| function codegenThreads(cfg: Config): number { | ||
| if (!usingParallelCompiler(cfg)) return 0; | ||
| if (cfg.windows) return 1; | ||
| if (cfg.lto) return 1; | ||
| if (cfg.ci) { | ||
| // ASAN is a test-only build (not shipped), so cross-shard IPO loss is | ||
| // fine and the speedup is worth it. The count is FIXED so zig-only and | ||
|
|
@@ -291,7 +263,7 @@ | |
| // our fork (upstream added Feb 2026, not backported). | ||
| const interleave = false; | ||
| const consoleMode = !interleave || hostWin; | ||
| const parallelSema = usingParallelCompiler(cfg) ? " --env=ZIG_PARALLEL_SEMA=1" : ""; | ||
| const parallelSema = " --env=ZIG_PARALLEL_SEMA=1"; | ||
| n.rule("zig_build", { | ||
| command: `${stream} ${consoleMode ? "--console" : "--zig-progress"} --env=ZIG_LOCAL_CACHE_DIR=$zig_local_cache --env=ZIG_GLOBAL_CACHE_DIR=$zig_global_cache${parallelSema} $zig build $step $args`, | ||
| description: "zig $step → $out", | ||
|
|
@@ -467,6 +439,7 @@ | |
| `-Denable_fuzzilli=${bool(cfg.fuzzilli)}`, | ||
| `-Denable_valgrind=${bool(cfg.valgrind)}`, | ||
| `-Denable_tinycc=${bool(cfg.tinycc)}`, | ||
| `-Dlto=${bool(cfg.lto)}`, | ||
|
Check warning on line 442 in scripts/build/zig.ts
|
||
|
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 comment at lines 446-448 still says "MUST be 0 on the stable compiler — see codegenThreads()", but this PR removed the stable/parallel compiler split and Extended reasoning...What the issue isThe comment immediately following the new // Sharded LLVM codegen — one shard per host core on the parallel
// compiler. Zig has no "auto" value (0 = single-threaded). MUST be 0
// on the stable compiler — see codegenThreads().
`-Dllvm_codegen_threads=${codegenThreads(cfg)}`,This comment is now stale documentation that this PR's own changes orphaned. Why it's staleThis PR collapsed the
After these removals, Step-by-step proof
ImpactNo behavioral impact — this is purely misleading documentation. A future reader trying to understand why How to fixUpdate the comment to reflect the current state, e.g.: // Sharded LLVM codegen — see codegenThreads() for when sharding is
// gated off (Windows, LTO, non-ASAN CI). Zig has no "auto" value.Or simply drop the second sentence entirely, since |
||
| // Always ON — bun uses mimalloc as its default allocator. The flag | ||
| // exists for experimentation; in practice it's never OFF. | ||
| `-Duse_mimalloc=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.
🧩 Analysis chain
🌐 Web query:
Does Zig supportobj.lto = .fullwhenuse_llvm = false/ the self-hosted backend is enabled?💡 Result:
No, Zig does not support obj.lto = .full when use_llvm = false / the self-hosted backend is enabled.
Citations:
Compilation: Enable LTO for libraries by default ziglang/zig#22228🏁 Script executed:
Repository: oven-sh/bun
Length of output: 69
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 869
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 2849
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 1783
Reject
-Dno_llvm=truetogether with-Dlto=true.Full LTO is an LLVM-only path, but the code still allows both flags to be set independently. When both are true,
obj.use_llvmbecomes false whileobj.lto = .fullis set, which Zig does not support. Add a guard to fail fast instead of producing confusing Zig errors.Suggested fix
obj.use_llvm = !opts.no_llvm; obj.use_lld = if (opts.os == .mac or opts.os == .linux) false else !opts.no_llvm; if (opts.lto) { + if (opts.no_llvm) { + const fail_step = b.addFail("LTO requires the LLVM backend"); + obj.step.dependOn(&fail_step.step); + return; + } obj.lto = .full; obj.use_lld = true; }📝 Committable suggestion
🤖 Prompt for AI Agents