Repository navigation
build: sort startup-hot functions to the front of .text on linux (-9 MB RSS) - #33302
Conversation
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a Linux-only symbol ordering pipeline: a page-tracing LD_PRELOAD library and generator script produce a ChangesOrderfile generation and build integration
Sequence Diagram(s)sequenceDiagram
participant buildTs as scripts/build.ts
participant ciTs as scripts/build/ci.ts
participant ninja
participant generateOrderFile
participant pagetraceSo as pagetrace.so
participant bunProfile as bun-profile
participant buildkite as Buildkite
buildTs->>ciTs: inheritOrderFile(cfg) if eligible
ciTs->>buildkite: download prior .order artifact
buildTs->>ninja: runNinja(build)
alt should generate
buildTs->>ciTs: regenerateOrderFile(cfg)
ciTs->>generateOrderFile: generateOrderFile(options)
generateOrderFile->>pagetraceSo: build and LD_PRELOAD trace
generateOrderFile->>bunProfile: run representative workloads
bunProfile-->>generateOrderFile: traced page addresses
generateOrderFile-->>ciTs: write linker.order or throw
ciTs->>buildkite: reportOrderFileFailure(error) on failure
buildTs->>ninja: runNinja(relink)
end
buildTs->>ciTs: verifyOrderFileApplied(cfg, exe, strict)
ciTs->>ciTs: packageAndUpload includes and uploads .order artifact
Related Issues: None specified. Related PRs: None specified. Suggested labels: area:build, area:ci, linux Suggested reviewers: Determined by repository conventions; consider a build/CI infrastructure maintainer familiar with 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/orderfile/generate.ts`:
- Around line 63-196: The generate.ts workflow currently calls process.exit()
from multiple failure branches inside the try block, which bypasses the finally
cleanup and leaves the scratch directory behind. Update the failure paths in the
tracer build, workload execution, symbol lookup, and empty-symbol checks to
throw errors or otherwise abort without exiting immediately, so the finally
block in the main generation flow always runs and rmSync(scratch, ...) can clean
up. Use the existing run, lowerBound, and output-writing flow as the reference
points when replacing the exits.
In `@scripts/orderfile/pagetrace.c`:
- Around line 46-62: The shared `record` counter update in `on_fault` is racy
across threads, so replace the read-modify-write sequence on `record[0]` with an
atomic fetch-add when assigning the next slot. Keep the existing `record[1 + n]
= page` write and bounds check around `MAX_HITS`, but ensure the index comes
from an atomic increment so concurrent faults cannot overwrite the same entry.
Use the `on_fault` handler and `record` access pattern as the place to make this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1e4ae06-819a-4545-855a-86603c311582
📒 Files selected for processing (5)
package.jsonscripts/build/flags.tsscripts/orderfile/generate.tsscripts/orderfile/pagetrace.csrc/linker.order
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Review feedback is in (18db9bf):
Regenerating with the fixed tracer against the now-ordered build converges, which is worth a few more MB: Not taking the |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Thanks — the page-size, signal-reset, atomic counter, and cleanup-on-error fixes all look right now. No further issues from me, but since this changes the Linux release link step and checks in a large generated artifact, I'll leave the sign-off to a human.
Extended reasoning...
Overview
This PR adds an lld --symbol-ordering-file to the Linux release link so startup-hot functions cluster at the front of .text, cutting resident binary pages ~34%. Concretely it touches: scripts/build/flags.ts (new linker flag + linkDepends entry, Linux release only), scripts/orderfile/pagetrace.c (new LD_PRELOAD SIGSEGV/SIGBUS tracer that mprotects text/rodata and records first-touch pages), scripts/orderfile/generate.ts (new script that runs four workloads under the tracer, maps pages → symbols via nm -S, and writes the order file), src/linker.order (~30k generated function names), and a one-line package.json script.
Since my last review, commit 18db9bf addressed every open comment: the tracer now uses sysconf(_SC_PAGESIZE) instead of hardcoded 4096, resets signal(sig, …) for the actual arriving signal, checks mprotect's return, and uses __atomic_fetch_add for the shared counter; generate.ts clamps lowerBound to ≥0, throws instead of process.exit() inside the try/finally, and reads the page size from the trace header. No new issues surfaced this round.
Security risks
None material. The tracer and generator are offline developer tools invoked manually via bun run orderfile; neither ships in the binary nor runs in CI builds. The only production-facing change is a linker flag that reorders input sections — no code, ABI, or symbol-visibility change — and --no-warn-symbol-ordering makes unresolved names a silent no-op rather than a build break.
Level of scrutiny
Medium-high, and beyond what I should auto-approve. The generator/tracer are dev-only and the failure mode is graceful, but the change still (a) alters the link step for every shipped Linux release binary, (b) commits a ~30k-line generated artifact whose regeneration/staleness workflow is a repo-hygiene decision, and (c) introduces native signal-interposition code that future maintainers will need to reason about. Those are design/process calls a maintainer should ratify — they're not "simple, mechanical, or obvious."
Other factors
The PR description is thorough with before/after numbers and a convergence argument, all four prior review threads (mine and CodeRabbit's) are resolved and marked as such, and the flag is confined by c.linux && c.release so debug/other-OS builds are untouched. There's no automated test coverage for the generated file's effect (nor an obvious way to add one), which is another reason a human should confirm the RSS numbers and the decision to gate release links on this file.
|
Added The failure mode this guards is silence: lld skips names it can't resolve and we pass Fails without the order file, passes with it. |
|
Moved the order file to
|
|
Good catch on the CRLF case — fixed in 94e14be. The filter gated on Also took the second half of the suggestion and pinned the file in |
There was a problem hiding this comment.
Thanks — 94e14be addresses the CRLF concern and I have nothing further; leaving final sign-off to a human since this changes the Linux release link (new --symbol-ordering-file flag + 30k-line generated artifact + LD_PRELOAD tracer).
Extended reasoning...
Overview
This PR adds an lld --symbol-ordering-file to the Linux release link so startup-hot functions cluster at the front of .text, cutting cold-start RSS by ~25–37%. It touches: scripts/build/flags.ts (new linker flag + linkDepends entry), a new 30k-line generated scripts/orderfile/linker.order, the generator (scripts/orderfile/generate.ts + pagetrace.c LD_PRELOAD shim), package.json (orderfile script), .gitattributes (LF pin), and test/js/bun/perf/linker-order.test.ts guarding the file's shape.
Security risks
None identified. The tracer is a dev-only regeneration tool (never shipped); the order file is a plain symbol list consumed by lld with --no-warn-symbol-ordering, so unresolved names are silently skipped rather than affecting the link. No auth, crypto, network, or user-input parsing is touched.
Level of scrutiny
This is a build-system change that alters how the shipped Linux release binary is linked. It's designed to degrade gracefully (stale/malformed order file → lost RSS win, not a broken build) and the stripped binary is claimed byte-identical in size, but it's still a novel mechanism (SIGSEGV-based page tracing, hand-generated 30k-symbol layout hint) applied to the production link path. That's beyond "simple/mechanical" and warrants a maintainer's eyes on the linker-flag wiring and the order file's provenance.
Other factors
All three of my earlier nits (page-size hardcoding, SIGBUS reset, atomic counter, process.exit in try, lowerBound(-1), and CRLF in the test) were addressed across 18db9bf and 94e14be, and CodeRabbit's two findings are marked resolved. The bug-hunting pass on the current head found nothing. The added test guards against silent rot of the order file. Given the scope — release link flags + a large generated artifact — I'm deferring rather than auto-approving.
bun -e 'console.log(1)' only executes about 8.5 MB worth of pages, but they
are spread across a ~50 MB .text, and the kernel faults in 64 KB around every
one of them. The result is ~27 MB of the binary resident for a hello world,
roughly three times the true working set.
Feed lld a --symbol-ordering-file listing the functions a run actually
executes, so their input sections land together at the front of .text. The
binary is byte-for-byte the same size and the code is unchanged; only the
layout moves.
src/linker.order is generated by `bun run orderfile`. scripts/orderfile/
pagetrace.c is an LD_PRELOAD shim that mprotects the executable's own
text+rodata to PROT_NONE and unprotects one page per SIGSEGV, which records
the exact pages a run touches without the kernel's fault-around speculation.
generate.ts runs four representative workloads through it, maps each touched
page back to the function that owns it, and writes the names in first-touch
order. Names lld cannot resolve are skipped, so the file degrades gracefully
as code moves.
maxrss on linux-x64, release:
before after
bun -e 'console.log(1)' 33.1 MB 21.8 MB -34%
bun hello.ts 35.2 MB 22.9 MB -35%
bun serve + 50 fetches 38.3 MB 23.9 MB -38%
bun nodeish.js 46.5 MB 39.3 MB -16%
With LTO (what CI ships): 30.0 MB -> 22.6 MB, -25%.
Startup time is unchanged to slightly faster, and fib/object-churn and HTTP
throughput benchmarks are within noise.
…anup - pagetrace.c rounds and re-maps with sysconf(_SC_PAGESIZE) instead of a hardcoded 4096, so it works on 16K/64K-page aarch64 kernels, and bails to SIG_DFL if mprotect still fails rather than spinning on the same fault. - The fault handler runs on whichever thread faulted, so the record counter is now an atomic fetch-add. - Reset the signal that actually arrived, not always SIGSEGV; the handler is installed for SIGBUS too. - generate.ts throws instead of calling process.exit() inside the try, so the scratch directory always gets cleaned up, clamps the fault count to the record's capacity, and no longer drops the symbols on the first .text page (lowerBound returns -1 there). Regenerating against the now-ordered build converges: cold functions that only made the list by sharing a page with a hot one stop being touched and drop out. 29970 functions, and maxrss for bun -e 'console.log(1)' goes 33.1 MB -> 20.7 MB.
Nothing in the build fails if the order file goes missing or malformed: lld skips names it cannot resolve and we pass --no-warn-symbol-ordering, so the only symptom is ~12 MB of resident binary pages coming back. Assert the file exists, holds the startup hot set, parses the way lld parses it (one whitespace-free symbol per line, '#' comments), has no duplicates, and is still wired into the linux release link.
src/ holds hand-written linker inputs (linker.lds, symbols.dyn). linker.order is a 30k-line machine-generated artifact that only scripts/orderfile/generate.ts ever writes, so it belongs beside it rather than in src/. No behavior change: the link still reads it, linkDepends still relists it, and maxrss for bun -e 'console.log(1)' stays at 20.7 MB with a byte-identical binary.
The filter gated on line.trim() but kept the untrimmed line, so a CRLF checkout (git for windows defaults core.autocrlf=true, and nothing in .gitattributes covered the file) made all ~30k entries look malformed. Trim first, matching the lld behavior the comment already claimed, and pin the file to LF.
darwin-14-x64 timed out on test/js/bun/terminal/terminal.test.ts (pty test, 90s timeout). 283/284 jobs green, and the order file is gated on c.linux && c.release, so the darwin binary is unchanged — the build's own size annotation reports +0.0 KB on every platform.
6ffb6d5 to
cecb87f
Compare
…ing it
The lld --symbol-ordering-file was a 30k-line checked-in file generated once
by hand. It rots silently: lld skips names it cannot resolve, so a stale file
gives the RSS back without breaking the link, and it was generated on one
machine for every linux target.
Generate it per-target instead, and never commit it:
- release builds trace their own pass-1 binary and relink against the result,
so what ships is ordered by exactly the functions it runs. Costs a second
link. Any commit can opt in with [generate symbol order].
- other builds inherit the last successful build's file for their target and
link once, so nothing is added to the critical path. ~90% of symbols still
resolve a commit later (100% of the hottest 1000); lld skips the rest.
- nothing runs on pull requests.
The file lives at <buildDir>/linker.order, seeded empty at configure time so
the flag is unconditional and both link passes share one build.ninja. It is a
link dependency, so regenerating it relinks and nothing else.
Each lane builds natively, so x64, x64-baseline and aarch64 each trace and
publish their own. musl (static, no LD_PRELOAD) and android (cannot run the
binary) fall back to an unordered link.
Verification is built in: an order file the linker ignored produces a binary
indistinguishable from an unordered one, so the build compares where the hot
functions landed against where a typical function landed and fails the release
if they are the same. A failed trace is not fatal — the order file is an
optimization, not correctness — but it annotates the build.
Measured on a linux-x64 release+LTO build, `bun -e 'console.log(1)'`:
29,228 KB -> 19,900 KB resident (-32%), same-size binary, hot functions at a
median 0.10MB into .text versus 22.1MB for a typical one.
Jarred-Sumner
changed the title
| export function shouldGenerateOrderFile(cfg: Config): boolean { | ||
| if (!orderFileEligible(cfg)) return false; | ||
| if (!cfg.canary) return true; | ||
| return /\[generate symbol order\]/i.test(process.env.BUILDKITE_MESSAGE ?? ""); | ||
| } |
There was a problem hiding this comment.
🟡 Android release builds satisfy usesOrderFile() (they have cfg.linux=true, asan=false) but the binary is cross-compiled and cannot run on the build host, so on a non-canary release shouldGenerateOrderFile() returns true → generateOrderFile() spawns the Android bun-profile → ENOEXEC → reportOrderFileFailure() posts a 'shipped unordered' warning annotation on every Android release. The PR description says android/musl 'fall back to an unordered link', but the actual path is attempt→fail→annotate — permanent noise that undermines the annotation's stated anti-rot purpose (canary Android also wastes time in inheritOrderFile() looking for an artifact that never exists). Gate usesOrderFile() (or orderFileEligible()) on cfg.crossTarget === undefined / cfg.abi === 'gnu' so these lanes skip cleanly.
Extended reasoning...
What the bug is
shouldGenerateOrderFile() gates on orderFileEligible() → usesOrderFile(), which checks only cfg.linux && cfg.release && !cfg.asan && !cfg.valgrind. orderFileEligible() adds isBuildkite, mode, and !PR; shouldGenerateOrderFile() adds !cfg.canary. None of these check cfg.crossTarget or cfg.abi.
For Android release builds, config.ts sets os = 'linux' (so cfg.linux = true, line 695), abi = 'android', crossTarget = '<arch>-unknown-linux-android<N>' (line 953), and forces asan = false (line 745). So usesOrderFile() returns true for Android release even though the binary cannot run on the build host.
The PR description explicitly says "musl (statically linked, so LD_PRELOAD cannot load the tracer) and android (the binary cannot run on the build host) fall back to an unordered link" — reading as a clean skip. But the code implements the fallback via attempt → fail → catch → reportOrderFileFailure(), which posts a Buildkite warning annotation.
Step-by-step (Android non-canary release, link-only step)
orderFileEligible(cfg):usesOrderFile→true && true && !false && !false= true;isBuildkite= true;mode === 'link-only'; not a PR → true.shouldGenerateOrderFile(cfg): eligible,!cfg.canary→ true.- build.ts calls
regenerateOrderFile(cfg)→generateOrderFile({buildDir, exeName}). generateOrderFilepassesprocess.platform !== 'linux'(host is Linux/amazonlinux), buildspagetrace.sowith the hostcc(succeeds — glibc), thenspawnSync(bunProfile, ...)wherebunProfileis an aarch64 bionic ELF.spawnSyncfails:r.errorisENOEXEC(or the exec fails with a loader error) →throw r.error.- build.ts catches it, sets
traced = false, callsreportOrderFileFailure(error)→ posts astyle: "warning"annotation titled "symbol order file not generated — shipped unordered". - Build continues and succeeds; unordered binary ships (correct behaviour).
Result: every Android non-canary release carries this warning annotation. The comment on reportOrderFileFailure says the annotation exists "so this cannot rot into a permanently unordered release nobody notices" — but a warning that fires on every build for a target that structurally cannot trace is exactly the noise that trains people to ignore it, defeating the purpose for the gnu targets where it matters.
Secondary effects
- Canary Android takes the inherit path:
inheritOrderFile()walks up to 50 previous builds callingbuildkite-agent artifact download bun-linux-<arch>-android.order, which never exists (no Android build ever traces successfully, so none uploads it). Each not-found lookup returns quickly, so this is tens of seconds of wasted CI, not minutes — but wasted every build. - musl likely hits the same class of failure (LD_PRELOAD into a static binary is a no-op → the trace file records zero faults →
generateOrderFilethrows"recorded no faults — is LD_PRELOAD working?"→ same annotation), though musl builds natively so the exact failure mode differs. usesOrderFile()also gates the--symbol-ordering-filelink flag andlinkDepends(); these fire for Android too, but harmlessly — configure seeds an empty file and lld no-ops on it.
Why nothing prevents it
The only platform check anywhere in the chain is process.platform !== 'linux' inside generateOrderFile(), which checks the host, not the target. packageAndUpload() in the same file already special-cases cfg.crossTarget !== undefined for exactly this reason ("cross-compiled binaries can't run on the build host") when generating features.json — the order-file path just doesn't apply the same gate.
Impact and fix
Nothing breaks: the build succeeds, the shipped binary is correct, and the gnu x64/aarch64 targets (the ones that matter for the RSS win) work as intended. This is annotation noise + wasted CI seconds, so nit. But it's a description/code mismatch the author clearly intended to gate, and the fix is one line — add && cfg.crossTarget === undefined (or && cfg.abi === 'gnu', which also excludes musl) to usesOrderFile() in flags.ts, or to orderFileEligible() if you want the link flag to stay on for those targets.
| * The file is never committed. Release builds generate it from their own pass-1 | ||
| * binary and relink against it; canary builds link against the last successful | ||
| * build's file (see store.ts) and publish a fresh one. Locally: |
There was a problem hiding this comment.
🟡 The header comment references (see store.ts) for the canary inherit/publish logic, but no store.ts exists in the repo — that logic lives in scripts/build/ci.ts (inheritOrderFile / packageAndUpload). The "publish a fresh one" clause is also slightly off: canary re-publishes the inherited file, not a freshly-traced one. Doc-only nit — worth fixing so readers aren't sent looking for a file that doesn't exist.
Extended reasoning...
What the issue is
The module-level docstring at scripts/orderfile/generate.ts:20-22 reads:
The file is never committed. Release builds generate it from their own pass-1 binary and relink against it; canary builds link against the last successful build's file (see store.ts) and publish a fresh one.
There is no store.ts anywhere in the repository — Glob('scripts/**/store.ts') returns nothing, and scripts/orderfile/ contains only generate.ts and pagetrace.c. The inherit-from-previous-build and publish-as-artifact logic actually lives in scripts/build/ci.ts: inheritOrderFile() walks previous builds and downloads the .order artifact, and packageAndUpload() uploads it standalone via orderFileArtifact(cfg).
Why it's stale
The PR timeline shows this PR went through several structural iterations — an earlier version committed linker.order to the tree, then it was moved to scripts/orderfile/, then it became a build artifact with inherit/publish wiring folded into ci.ts. The store.ts reference is a leftover from an intermediate iteration that was never landed.
The "publish a fresh one" phrasing is also inaccurate for the final design: shouldGenerateOrderFile() in ci.ts returns false for canary builds unless [generate symbol order] appears in BUILDKITE_MESSAGE, so a default canary inherits the previous build's file and re-uploads that same inherited file (via packageAndUpload, which uploads whatever is at orderFilePath(cfg) when orderFileFunctionCount > 0). It does not trace and publish a fresh one. The correct summary — the one ci.ts itself gives in its section header — is "Canary inherits; release generates."
Step-by-step proof
scripts/orderfile/contains exactly two files:generate.tsandpagetrace.c. Nostore.ts.grep -r 'store.ts' scripts/returns nothing.inheritOrderFileand the standalone-artifact upload both live inscripts/build/ci.ts(visible in this PR's diff at ci.ts:686 and ci.ts:428 respectively).shouldGenerateOrderFile(cfg)at ci.ts:645 gates regeneration on!cfg.canary || /\[generate symbol order\]/i.test(BUILDKITE_MESSAGE)— so ordinary canary builds do not "publish a fresh one"; they re-publish what they inherited.
Impact
Doc-only. No runtime, build, or CI behavior is affected. The only cost is that a reader following the pointer to understand where inheritance is implemented will look for a file that doesn't exist. Per CLAUDE.md's "Comments carry only durable non-obvious content" and "One source of truth; update every consumer atomically", the dangling cross-reference is worth fixing but not worth blocking merge.
Fix
Replace the sentence with something like:
* The file is never committed. Release builds generate it from their own pass-1
* binary and relink against it; canary builds inherit the last successful build's
* file and re-publish it (see scripts/build/ci.ts — inheritOrderFile / packageAndUpload).
* Locally:
usesOrderFile() only checked linux+release, so an android release satisfied it and then tried to run its cross-compiled bun-profile on the build host, and a musl release LD_PRELOADed a tracer into a static binary. Both fail, and both then annotate the build with "shipped unordered" — exactly the warning that is supposed to mean something when a gnu release loses its order file. Gate on abi === "gnu". Those two targets now skip the flag, the seed, the inherit and the verify, which is what the commit that introduced this said they would do. Also point the generate.ts header at scripts/build/ci.ts instead of a store.ts that never existed, and say what canary actually does (re-publish what it inherited, not trace a fresh file).
|
Both findings were right, fixed in 0bb93da. musl/android: store.ts: gone, the header now points at Also ran the new two-pass flow end to end on a local |
Jarred-Sumner
changed the title
| size_t bytes = (size_t)(HEADER_WORDS + MAX_HITS) * 8; | ||
| int fd = open(out, O_CREAT | O_TRUNC | O_RDWR, 0644); | ||
| if (fd < 0) return; | ||
| if (ftruncate(fd, (off_t)bytes) != 0) { | ||
| close(fd); | ||
| return; | ||
| } | ||
| record = mmap(NULL, bytes, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); | ||
| close(fd); | ||
| if (record == MAP_FAILED) { | ||
| record = NULL; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 The constructor opens BUN_PAGETRACE_OUT with O_CREAT|O_TRUNC and ftruncates it before reading /proc/self/maps to check whether this process is actually the traced binary (the region_count == 0 bail at line 120 comes after). LD_PRELOAD and both BUN_PAGETRACE_* env vars inherit across exec, so if a workload ever spawns a dynamically-linked subprocess (/bin/sh, another bun, etc.), that child re-runs the constructor, truncates the parent's MAP_SHARED trace file to zero, and wipes every fault recorded so far — the earliest-touched (hottest) pages — before bailing. Latent today (none of the four workloads spawn), but it will silently degrade the trace the moment someone adds a Bun.spawn/child_process workload; move the /proc/self/maps scan above the open() and return on region_count == 0 before touching the file.
Extended reasoning...
What the bug is
pagetrace_init() performs its side effects in the wrong order: it opens BUN_PAGETRACE_OUT with O_CREAT | O_TRUNC | O_RDWR (line 88), ftruncates it to 8 MB of zeros (line 90), and mmaps it (line 94) — all before it reads /proc/self/maps (line 102) to decide whether this process is the binary being traced. The if (!region_count) return; guard at line 120 correctly no-ops in a non-target process, but by then the output file has already been truncated and zero-filled.
The code path
generate.ts's run() helper spawns each workload with env: { ...process.env, LD_PRELOAD: tracer, BUN_PAGETRACE_BIN: bunProfile, BUN_PAGETRACE_OUT: out, ... }. All three of LD_PRELOAD, BUN_PAGETRACE_BIN, and BUN_PAGETRACE_OUT are ordinary environment variables, so any child the workload spawns without an explicit env (Bun's spawn APIs inherit env by default) gets them too. Every dynamically-linked child therefore loads pagetrace.so and re-runs pagetrace_init() before main.
Why nothing prevents it
The only guard that could stop a non-target process from touching the file is the region_count == 0 check — a child like /bin/sh has no mapping whose path contains bun-profile, so strstr(path, binary) never matches and region_count stays 0. But that check is at line 120, after the open(O_TRUNC) at line 88. There is no PID check, no "is this the target" check before the open, and the env-var-presence check at line 82 passes because the child inherited both.
Step-by-step proof
Suppose a future workload does Bun.spawn(["/bin/echo", "hi"]) mid-run:
- Parent
bun-profilestarts underLD_PRELOAD=pagetrace.so;pagetrace_init()openstrace-N.binwithO_TRUNC, ftruncates to 8 MB, mmapsMAP_SHARED, finds its own text/rodata regions, arms the tracer, and mprotects. The parent then faults through startup, atomically incrementingrecord[1]and writing page addresses atrecord[2..]. Sayrecord[1] = 3000by the time it reaches the spawn. - Parent execs
/bin/echo. The child inheritsLD_PRELOAD,BUN_PAGETRACE_BIN=.../bun-profile,BUN_PAGETRACE_OUT=.../trace-N.bin. - Child's dynamic loader runs
pagetrace_init(). Line 82: both env vars set → continue. Line 88:open("trace-N.bin", O_CREAT|O_TRUNC|O_RDWR)— truncates the file to length 0. On Linux this discards every page-cache page backing the parent'sMAP_SHAREDmapping. Line 90:ftruncateback to 8 MB — the file is now a zero-filled hole. Line 94: mmaps it. Line 100: writesrecord[0] = page_size. - Child reads
/proc/self/maps; no line containsbun-profile(it's/bin/echo), soregion_countstays 0. Line 120:return. Child never arms, runs normally, exits. - Parent continues faulting. Its
MAP_SHAREDview now seesrecord[1] == 0(the truncate+ftruncate zeroed it), so the next__atomic_fetch_add(&record[1], 1)returns 0 and writes into slot 0. All 3000 pre-spawn faults — the hottest startup path by construction — are gone. Post-spawn faults are recorded from index 0. generate.tsreads the trace:header[1]is non-zero (post-spawn faults), so therecorded no faultscheck passes. If the surviving workloads yield ≥MIN_FUNCTIONS, the whole run passes. The order file just quietly omits the earliest-touched functions of the clobbered workload — the exact ones the ordering is supposed to prioritize.
Impact
Latent today. All four current workloads (bun -e, hello.ts, Bun.serve+fetch, single-file bun test) run in-process and don't spawn, so nothing triggers this in the PR as landed and the measured −32% RSS is real. This is a build-time dev tool, not shipped code. But the failure mode is silent quality degradation with no error, and Bun.spawn/child_process is a very natural startup path someone will eventually want ordered (or bun test isolate mode may become default). Not merge-blocking, but worth the one-move fix.
Fix
Move the /proc/self/maps scan (lines 102–120) above the open() at line 88, so the constructor returns on region_count == 0 before touching the output file. Alternatively, drop O_TRUNC and only ftruncate after confirming region_count > 0 — but reordering is simpler and also avoids the pointless mmap in every child.
|
|
||
| function run(cmd: string[], env?: Record<string, string | undefined>) { | ||
| const r = spawnSync(cmd[0]!, cmd.slice(1), { | ||
| env: { ...process.env, ...env }, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| maxBuffer: 1 << 29, // nm prints ~10 MB of symbols | ||
| }); | ||
| if (r.error) throw r.error; | ||
| return r; |
There was a problem hiding this comment.
🟡 The traced workloads at line 135 go through run()'s spawnSync with no timeout, so a hung workload (e.g. server.js doing Bun.serve + 50 fetches under a tracer that mprotects .text to PROT_NONE and swallows Bun's own SIGSEGV/SIGBUS handler installs) blocks forever — build.ts's try/catch never fires and the Buildkite step timeout kills the whole build, which is exactly the outcome the comment there says must not happen ("a flaky trace workload must not fail a release build 40 minutes in — link unordered, loudly"). Pass timeout: 60_000 to the workload spawnSync and treat r.signal as a failure so the catch fires and the release ships unordered instead of failing. CI-robustness only, not shipped-binary correctness.
Extended reasoning...
What the bug is
generateOrderFile()'s run() helper at generate.ts:87-95 wraps spawnSync with no timeout option, and the four traced workloads (including server.js, which does Bun.serve + 50 fetches + server.stop(true)) are launched through it at line 135. The build.ts caller wraps regenerateOrderFile() in a try/catch whose comment states the design invariant verbatim: "The order file is an optimization, not correctness: a flaky trace workload must not fail a release build 40 minutes in. Link unordered, loudly." But that catch only fires when spawnSync returns and run() throws (r.error set, or the caller checks r.status !== 0). A workload that hangs never returns from spawnSync, so nothing is thrown, the catch never fires, and the Buildkite step's own timeout eventually hard-fails the whole build — the exact outcome the fallback was designed to prevent.
The code path
- Release link-only step:
shouldGenerateOrderFile(cfg)→ true. - build.ts calls
regenerateOrderFile(cfg)insidetry { … } catch { traced = false; reportOrderFileFailure(error) }. regenerateOrderFile→generateOrderFile({buildDir, exeName})→ for each workload,run([bunProfile, ...args], { LD_PRELOAD: tracer, ... }).run()callsspawnSync(cmd[0], cmd.slice(1), { env, stdio, maxBuffer })— notimeout.spawnSyncis synchronous: it does not return until the child exits. If the child never exits, the JS thread is blocked here indefinitely.- The Buildkite step timeout fires → job killed → hard release-build failure.
reportOrderFileFailure()never runs, no "shipped unordered" annotation is posted, the pass-1 binary is never packaged.
Why a hang is plausible
The workloads run in an unusual environment: pagetrace.c mprotects the binary's own text+rodata to PROT_NONE, installs a SIGSEGV/SIGBUS handler with SA_SIGINFO | SA_ONSTACK | SA_NODEFER (but not SA_RESTART), and interposes sigaction to swallow Bun's own crash-reporter and JSC WasmFaultSignalHandler installs. server.js then does real networking (50 loopback fetches) under those conditions. Any of: an EINTR that isn't retried somewhere in the I/O path (no SA_RESTART), server.stop(true) not fully draining under the swallowed-handler environment, or a re-entrant fault while a lock is held, would produce a deadlock rather than an exit. These are precisely the "flaky trace workload" cases the design comment names.
Why nothing else prevents it
The only timeout anywhere in the order-file path is ARTIFACT_DOWNLOAD_TIMEOUT_MS on the inherit-side buildkite-agent spawns (ci.ts:707), whose comment even says "50 hops × a hung agent must not blow the step's budget" — the same reasoning applies here but wasn't applied. run() also doesn't check r.signal, so even if a timeout were added, a SIGTERM'd workload would report exited null (falling through the r.status !== 0 check since null !== 0 is true, so it would still throw — but with a confusing message).
Step-by-step proof
Suppose the server.js workload deadlocks (fetch #37 never completes because a SIGSEGV interrupted a syscall that wasn't retried):
spawnSync(bunProfile, ["server.js"], {...})at generate.ts:88 blocks — no timeout.run()never returns →generateOrderFile()never returns →regenerateOrderFile()never returns.- build.ts's
try { regenerateOrderFile(result.cfg) } catch { ... }never reaches either the end of the try or the catch. - After the Buildkite step's configured timeout (typically 60-90 min), the agent SIGKILLs the job.
- Result: the linux release build is red. No annotation, no unordered fallback, no packaged binary. This is exactly what the code comment says must not happen.
Impact and fix
Nit severity: this is release-CI robustness for a plausible-but-not-yet-observed failure mode, not shipped-binary correctness. But it directly contradicts an invariant the PR author wrote a paragraph justifying, and the fix is one line. Pass timeout: 60_000 (the workloads normally finish in <5s even under the tracer) to the workload spawnSync, and treat r.signal / r.error?.code === 'ETIMEDOUT' the same as a non-zero exit so the caller's catch fires and the release ships unordered instead of failing:
function run(cmd: string[], env?: Record<string, string | undefined>, timeout?: number) {
const r = spawnSync(cmd[0]!, cmd.slice(1), {
env: { ...process.env, ...env },
stdio: ["ignore", "pipe", "pipe"],
maxBuffer: 1 << 29,
timeout,
});
if (r.error) throw r.error;
if (r.signal) throw new Error(`${cmd[0]} killed by ${r.signal}` + (timeout ? ` (timed out after ${timeout}ms)` : ``));
return r;
}
// ...
const r = run([bunProfile, ...workload.args], { LD_PRELOAD: tracer, ... }, 60_000);
…file (#33341)w Follow-up to #33302. The `--symbol-ordering-file` packs the functions a run executes to the front of `.text`. It was traced from four workloads (`bun -e`, a script, `Bun.serve`, `bun test`), which left two large chunks of what people actually run scattered across the binary: `bun install`, and anything attached to a terminal. ### What the new workloads add Measured on a linux-x64 release build (`bun run orderfile`), counting only functions no earlier workload had already touched: | workload | pages touched | new functions | | --- | --- | --- | | `bun -e` | 2,187 | 22,456 | | `bun hello.ts` | 2,250 | 680 | | `bun server.js` | 2,689 | 5,224 | | `bun test` | 3,153 | 5,065 | | **`bun install`** | 636 | **1,471** | | **`bun install` (cached)** | 545 | **64** | | **`bun cli.js` (pipe)** | 3,144 | **2,014** | | **`bun cli.js` (tty)** | 3,463 | **2,193** | 33,428 functions before, 39,167 after. ### `bun install` Offline by construction: the app's one dependency is a tarball that `bun pm pack` produces from a fixture, packed by the very binary being traced. A slow or unreachable registry must not be able to cost a release its order file. It still walks the real path (manifest, lockfile, tarball extraction, node_modules linking), and it runs a second time because a re-install that finds everything already in place is the common one. It touches far fewer pages than the runtime workloads because it never starts JavaScriptCore, but 1,471 of the functions it does touch were in nobody else's trace. ### The interactive CLI `scripts/orderfile/cli-fixture.js` reads stdin, writes stdout, and drives `node:readline`. It is traced twice: - on a pipe, fed `world\none\ntwo\nquit\n` on stdin - on a pseudo-terminal, typed the same thing The terminal run is the point: `isatty`, `TIOCGWINSZ`, raw mode, and readline's line editor with its cursor escapes are 2,193 functions that the pipe run never reaches, even after everything above it. `scripts/orderfile/ptyrun.c` is what provides the terminal: `forkpty`, relay our stdin in as keystrokes, drain the child's output so a full pty buffer cannot block it, exit with its status. `PTYRUN_PRELOAD` becomes the child's `LD_PRELOAD`, because the page tracer has to load into the binary being traced and not into the helper that spawns it. The fixture exits on `quit` rather than on end-of-input, so it never waits for an EOF a terminal might not deliver. Workloads now also run under a 120s timeout, so an interactive one that does hang fails the trace instead of hanging the release. ### Placement The four existing workloads still run first, and the order file is first-touch order with dedup, so the functions that land at the very front of `.text` are the same ones as before. The first 1,000 entries (what `verifyOrderFileApplied` samples) are byte-identical across a before/after run. ### Verification ``` $ bun run orderfile bun -e 2187 pages touched, +22456 functions bun hello.ts 2250 pages touched, +680 functions bun server.js 2689 pages touched, +5224 functions bun test 3153 pages touched, +5065 functions bun install 636 pages touched, +1471 functions bun install (cached) 545 pages touched, +64 functions bun cli.js (pipe) 3144 pages touched, +2014 functions bun cli.js (tty) 3463 pages touched, +2193 functions wrote build/release/linker.order (39167 functions) ``` `test/js/bun/perf/linker-order.test.ts` gains a check on the new C helper, since a pty that silently degrades to a pipe would quietly drop 2,193 functions from the file: it compiles `ptyrun.c`, runs bun under it, and asserts the child sees `isTTY`, an 80 column window, and the preload it was handed, while the same command on a pipe sees none of it. ``` $ bun bd test test/js/bun/perf/linker-order.test.ts 8 pass 1 skip 0 fail ```
|
Follow-up is up: #33347. Fixed both halves of it — the constructor now finds its regions before it creates the output file, and it drops No workload spawns today, so the order file is unchanged (same per-workload page counts, 37.9k functions). The regression test traces a fixture that reads 32 pages, execs a child, then reads one more: before the fix it recorded 1 page, after it records all 33. |
…rit (#33345) Follow-up to #33302. **Inheriting never actually worked**, and even fixed it could never have started. ### Inheriting never worked `previousBuilds()` walked `prev_branch_build` from Buildkite's public build JSON. That field no longer exists, so the walk found nothing and every build linked unordered: ``` Inherit symbol order file Looking for bun-linux-aarch64.order in the last 50 builds on this branch... ~ symbol order: no previous build on this branch (0s) — linking unordered ``` Zero seconds, because it never made a request. Note this also means `utils.getLastSuccessfulBuild()` always returns `undefined` — it reads the same field — so it can't be used here either, and `skip-builds` presumably hasn't worked for a while. What does work, unauthenticated, is `builds/latest?branch=X&state=passed`, which 302s at the build. The redirect drops the `.json` on the way, so we read the `location` header rather than following it. Older builds are found by probing build numbers, but that's only a safety net for a run of builds that published nothing — candidates are produced lazily, so the normal path is one lookup (measured: 2 requests, 420ms, against the live API). ### The chain could never start A build that inherits nothing has no order file, so `packageAndUpload` publishes none, so the next build inherits nothing either. Nothing seeds it short of a release. So a build that inherits nothing now traces its own binary and relinks. That bootstraps the chain and heals it after any gap. It costs a second link, so it announces itself with a warning annotation — seeing it once is expected, seeing it on **every** build means inheriting is broken again and someone should look. That's the alarm this pipeline was missing the first time. ### Also - An empty or truncated artifact no longer counts as inherited. It would have published nothing, silently breaking the chain for the next build. - Probing starts from *this* build rather than from the newest passed one, so a build that failed its tests but still linked and published is considered. - Cross-compiled targets never trace — they can't run the binary they just linked. They can still inherit and link ordered. ### Tests `test/js/bun/perf/linker-order.test.ts` now covers the decision rules: releases always generate, canaries don't unless asked, PRs never do and never publish, a canary that inherited nothing generates anyway, and cross-compiled targets never generate. I verified each is load-bearing by breaking the rule and watching a test fail. The lookup itself was verified against the live Buildkite API using the exact env of the build that failed (#68425): `latest?branch=main&state=passed` → 302 → #68409 → JSON. That's the call the broken code never made. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…der on macOS too (#35085) ## What Replaces the page-fault order-file tracer with a function-entry tracer (INT3 on x86-64, BRK on arm64) and extends symbol ordering to macOS arm64 via `-order_file`. The page tracer lists every function that shares a 4 KB page with one that runs, so the order file carried ~38k names of which ~23k never execute; those cold functions sat at the front of `.text` and diluted the hot set. Recording exact entries lists only the ~14k that actually run. ## Result Linux x86-64 release, `bun -e` RSS (median of 5): | ordering | VmRSS | vs unordered | | --- | ---: | ---: | | unordered | 38,260 KB | | | page-order (before, #33302) | 31,080 KB | -7.2 MB | | function-order (this PR) | **22,096 KB** | **-16.2 MB** | So about 9 MB on top of what #33302 already ships, for the same-size binary. macOS arm64 had no ordering at all before this. The tracer was verified against a release `bun-profile` on a darwin-arm64 box: 9,612 functions traced across the eight workloads, first entries `_mi_process_attach → _mi_lock_acquire → ...` as expected. ## How `scripts/orderfile/functrace.c` is a preloaded library (`LD_PRELOAD` / `DYLD_INSERT_LIBRARIES`) that: - finds the main executable's text (the `PT_LOAD X` segment on Linux, the `__TEXT,__text` section on macOS, so the Mach-O header and read-only data `nm` lists as `T` are never touched), - remaps it so a separate RW alias of the same pages exists (memfd on Linux; `VM_PROT_COPY` + `mach_vm_remap` on macOS, since RWX on `__TEXT` is refused even with COPY), - reads the list of function starts the generator wrote from `nm`, drops anything outside the text bounds or already a breakpoint (JSC's LLInt places literal `int3` at never-taken bytecode labels), plants a breakpoint at each, - records the address on first SIGTRAP, restores the instruction through the RW alias, rewinds the PC and resumes. `sigaction` is interposed so nothing replaces the SIGTRAP handler. `pthread_sigmask` / `sigprocmask` are interposed to strip SIGTRAP from any block set: mimalloc's scavenger thread blocks every signal, and a blocked synchronous SIGTRAP makes the kernel reset the handler to `SIG_DFL` and kill the process, which would end the trace at the first breakpoint that thread touches. `scripts/orderfile/generate.ts` keeps the same eight workloads and the same `generateOrderFile()` / `runCommand()` exports. It now writes a starts file from `nm --defined-only`, runs each workload under `functrace.c`, and maps recorded addresses straight back to names; no page-to-function walk. ## Wiring - `usesOrderFile()` now returns true for darwin arm64 too (both Apple ld and ld64.lld take `-order_file`). - A `-Wl,-order_file,<buildDir>/linker.order` flag entry sits next to the existing `--symbol-ordering-file` one, gated to darwin. - `linkDepends()` lists the order file on darwin so regenerating it relinks, and only relinks. - `ptyrun.c` sets `DYLD_INSERT_LIBRARIES` on macOS so the tty workload still reaches the tracer. Every build lane cross-compiles from the aarch64 `buildHostPlatform`, so only linux-aarch64 can trace its own binary. For the other two order-file targets a new `-trace-order` step (`.buildkite/ci.mjs`) runs on the target-arch test fleet after `build-bun`, downloads `bun-profile`, runs `generate.ts` against it, and uploads the `.order` artifact: - `darwin-aarch64-trace-order` on the bare-metal mac queue - `linux-x64-trace-order` on the x64 debian test agent `inheritOrderFile()` now searches every step of a previous build (the artifact name is target-unique), so build N+1's cross-compile link picks up build N's trace. `packageAndUpload()` only re-uploads the standalone `.order` from lanes that traced it themselves, so each target has exactly one publisher. Non-PR only and soft-fail, since the order file is an optimization. ## Tests `test/js/bun/perf/linker-order.test.ts` covers: - `usesOrderFile` / `linkDepends` / the linker flag for both linux and darwin arm64, and that neither flag appears on the other platform or on debug/sanitizer links - the `functrace.c` entry count surviving a child exec, on linux glibc and darwin arm64 (replaces the old page-tracer fixture) - the existing generator-under-node and ptyrun checks <!-- robobun:evidence:begin --> --- **no test proof** · iteration 5 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
2 participants
What
Starting bun up only executes about 8.5 MB worth of pages, but they are scattered across a ~50 MB
.text, and Linux faults in 64 KB around each one. So abun -e 'console.log(1)'ends up with far more of the binary resident than it actually runs.This passes lld a
--symbol-ordering-filelisting the functions bun executes while starting up, in first-touch order, so they land together at the front of.text.Measured on a linux-x64 release + LTO build. The saving is a roughly constant ~9 MB of resident memory, so it is a large fraction of a short-lived process and a smaller one of a long-running program — but the megabytes do not come back:
bun -e 'console.log(1)'bun hello.tsBun.serve+ regexThe binary is byte-for-byte the same size (76,877,584 B) and runs the same code — only the order changed. Minor faults on the trivial case drop from 1,679 to 1,535.
Why it does not decay: the saving is the fault-around waste around the startup set — pages the kernel dragged in around scattered hot functions that are never executed. Packing those functions together eliminates that once. Code a heavier program additionally runs is code it genuinely needs, and those pages are about as dense either way. The win only vanishes for a process that eventually executes most of
.text, which nothing we ship does.For reference, a shipped
bun-1.3.14on the same machine uses 29,116 KB on the trivial case — within 0.3% of today's canary, despite a binary 21% larger. The startup footprint has not moved in a long time; it is the scatter, and nothing was addressing it.How the order file is produced
scripts/orderfile/pagetrace.cis anLD_PRELOADshim that mprotects the executable's own text toPROT_NONEand unprotects one page perSIGSEGV, recording exactly the pages a run touches. A handful of representative workloads (bun -e, a script,Bun.serve,bun test) run under it; every touched page maps back to the function that owns it, and those names are emitted in first-touch order.Re-running against a build that already has an order file converges rather than drifting: a cold function only ever gets listed because it shares a page with a hot one, and once the hot functions are packed together those cold neighbours stop being touched and drop out.
It is not committed
An earlier version of this checked a 30k-line
linker.orderinto the tree. That rots silently — lld skips names it cannot resolve and we pass--no-warn-symbol-ordering, so a stale file quietly gives the RSS back instead of breaking the link — and one file cannot serve x64, x64-baseline and aarch64, whose symbol sets differ.Now it is a build artifact:
[generate symbol order]in the message.Inheriting walks back through previous builds and takes the first one that published the artifact. The artifact's existence is the success signal — only a build that linked and packaged uploads one — so cancelled and failed builds are skipped without having to ask which of them passed.
Each linux lane builds natively, so x64, x64-baseline and aarch64 each trace and publish their own. musl (statically linked, so
LD_PRELOADcannot load the tracer) and android (the binary cannot run on the build host) fall back to an unordered link.Mechanics
The file lives at
<buildDir>/linker.orderand configure seeds an empty one, which is a no-op for lld. That keeps the flag unconditional so both link passes share a singlebuild.ninja, and it makes the second pass a pure relink: the order file is a link dependency, so overwriting it dirties exactly one ninja edge. Verified — after the relink, ninja reports no work to do.Verification
An order file the linker ignored produces a binary indistinguishable from an unordered one, which is the failure mode that would let this rot unnoticed. So the build measures it: it compares where the order file's hottest functions landed against where a typical function landed. Ordered, the hot median is 0.10 MB into
.textagainst 22.1 MB for a typical function; unordered, the two numbers are the same. A release whose order file had no effect fails the build.A failed trace is not fatal — the order file is an optimization, not correctness, and a flaky workload should not kill a release 40 minutes in. It links unordered and annotates the build instead.
Every step logs what it did and how long it took, so
symbol orderin a job log tells the whole story.test/js/bun/perf/linker-order.test.tsguards the wiring at PR time: that the flag reaches the linux release link, that it is a link dependency (which is what makes the relink work), and that it stays off for debug and sanitizer builds.