Repository navigation
Revert "perf(standalone): madvise(MADV_DONTNEED) embedded source pages after entrypoint load" (#29320) - #37356
cirospaciari wants to merge 1 commit into
Conversation
…s after entrypoint load" (#29320) Removes the post-entrypoint MADV_DONTNEED hint over the embedded __BUN/.bun section in compiled standalone binaries, together with the StandaloneModuleGraph debug log scope that #34807 added for it, its call site in run_command.rs, and the test that asserted the hint fired. The original change has since been ported from Zig to Rust, so this removes the equivalent Rust code rather than reverting c14e37e directly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
WalkthroughThe standalone run path no longer performs source-page memory reclamation after entrypoint evaluation. The related graph API, scope registration, integration test, and expected-duration entries are removed. ChangesStandalone graph hint removal
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean revert of the madvise(MADV_DONTNEED) hint and its supporting code.
What was reviewed:
- Confirmed no remaining references to
hint_source_pages_dont_need/hintSourcePagesDontNeedorscoped_log!(StandaloneModuleGraph, ...)anywhere in the tree, so removingdeclare_scope!(StandaloneModuleGraph)leaves nothing orphaned. - Checked that
macho::get_data/elf::get_dataare still used byfrom_executable(), so no dead helpers are left behind. libc::andbun_alloc::were referenced fully-qualified inline (no top-leveluse), so no unused imports remain.
Extended reasoning...
Overview
This PR reverts #29320, which added a best-effort madvise(MADV_DONTNEED) over the embedded __BUN/.bun section of compiled standalone binaries after entrypoint evaluation. The revert is pure deletion across four files: the Graph::hint_source_pages_dont_need() function and its declare_scope! in StandaloneModuleGraph.rs, its single call site in run_command.rs, the dedicated test file standalone-madvise-tla.test.ts, and that test's expected-durations.json entry.
Security risks
None. This removes a kernel memory-advisory hint; the pre-#29320 behavior (no hint) is strictly the more conservative state.
Level of scrutiny
Low. This is a mechanical revert with no new logic — every hunk is a deletion. The only correctness question for a revert like this is whether it leaves anything dangling, and I verified it does not: no remaining callers of the removed function, no remaining users of the removed debug scope, no orphaned imports (the libc::madvise / bun_alloc::page_size calls were fully-qualified), and the platform section-data helpers (macho::get_data, elf::get_data) remain live via from_executable().
Other factors
The PR description explains this is a manual re-application of the revert against the post-Zig→Rust port, and that the stderr assertions from #34807 in bundler_compile.test.ts / bun-build-compile.test.ts remain valid (nothing to log now, so empty stderr still holds). The deleted test existed solely to prove the removed call site was reached, so deleting it alongside the code is the correct scope per the repo's "delete dead code in the same PR that makes it dead" rule.
…ce (#37357) ### What does this PR do? Adds `BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE`, a runtime opt-out for the behavior introduced in #29320: a `bun build --compile` executable calls `madvise(MADV_DONTNEED)` on its embedded source section once the entrypoint has loaded, so anything that touches the source afterwards (a lazy `require()`, resolving a stack trace) pages it back in from the executable on disk. Deployments where that re-read is undesirable can set the flag in the environment of the running executable to keep the pages resident: ```sh BUN_FEATURE_FLAG_DISABLE_STANDALONE_MADVISE=1 ./myapp ``` The compiled binary reads it at runtime, nothing is baked in at compile time. It follows the existing `BUN_FEATURE_FLAG_DISABLE_*` escape hatches: registered in `env_var.rs`, checked in `hint_source_pages_dont_need()` (which returns early, sharing the existing `len == 0` return), falsy values such as `0` keep the default. No effect on Windows or on the plain interpreter, which never issue the hint. Like the other `BUN_FEATURE_FLAG_DISABLE_*` escape hatches it is not documented. #37356 is the alternative of removing the madvise call altogether; the two are mutually exclusive. ### How did you verify your code works? `test/js/bun/compile/standalone-madvise-tla.test.ts` now runs the same compiled binary three times with `BUN_DEBUG_StandaloneModuleGraph=1`: with the variable unset (explicitly removed from the child env so an ambient value on the host cannot leak in), set to `0`, and set to `1`. The first two must log a `hintSourcePagesDontNeed:` line, the last must not log one at all, since the flag path returns before logging anything. The test stays debug-only because it relies on the scoped logger. Against a debug build without this change the `1` case fails as expected: ``` Expected to not contain: "hintSourcePagesDontNeed:" Received: "before-await\nafter-await\n[standalonemodulegraph] hintSourcePagesDontNeed: MADV_DONTNEED 4096 bytes\n" ``` With the change, `bun bd test test/js/bun/compile/standalone-madvise-tla.test.ts` passes (22 assertions). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/compile/standalone-madvise-tla.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
1 participant
What does this PR do?
Reverts #29320.
Removes the
madvise(MADV_DONTNEED)hint that compiled standalone binaries issued over the embedded__BUN,__bun/.bunsection after the entrypoint finished its initial evaluation:Graph::hint_source_pages_dont_need()insrc/standalone_graph/StandaloneModuleGraph.rs, along with theStandaloneModuleGraphdebug log scope that standalone: route madvise hint logging through a scoped logger #34807 added solely for itsrc/runtime/cli/run_command.rstest/js/bun/compile/standalone-madvise-tla.test.tsand itstest/expected-durations.jsonentryThe original change has since been ported from Zig to Rust, so this is a manual revert of the equivalent code rather than a
git revertof c14e37e. Thestderrassertions #34807 added tobundler_compile.test.ts/bun-build-compile.test.tsstill hold (there is no longer anything to log), so they are left in place.How did you verify your code works?
bun bdbuilds on macOS;nm/stringson the resulting binary no longer containhint_source_pages_dont_need/hintSourcePagesDontNeed. A--compiled top-level-await entrypoint built with that binary runs to completion with exit 0 and empty stderr, including withBUN_DEBUG_StandaloneModuleGraph=1set.