Stop disabling transparent huge pages in every process bun spawns - #36990
Conversation
mimalloc (built with MI_DEFAULT_ALLOW_THP=0) used prctl(PR_SET_THP_DISABLE) to keep its arenas off transparent huge pages. That flag is process-wide, inherited across fork and preserved across execve, so every child of a bun process on Linux -- Bun.spawn, bun run, lifecycle scripts -- has been running with THP disabled, including its own MADV_HUGEPAGE requests. Bump mimalloc to a revision that opts out per mapping (MADV_NOHUGEPAGE) instead, and do the same for bun_alloc's lazily-faulted arena, which the process-wide flag had been covering incidentally.
No-Verification-Needed: dependency pin bump
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesTransparent huge-page policy
Pinned dependency revisions
Possibly related PRs
Suggested reviewers: 🚥 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 `@src/bun_alloc/lib.rs`:
- Around line 1613-1620: Update the platform-gated madvise block in the arena
mapping flow to check the return value of madvise. If it returns nonzero, retain
the mapping and log the corresponding errno; treat MADV_NOHUGEPAGE as a
best-effort hint rather than an allocation failure.
In `@test/js/bun/spawn/spawn.test.ts`:
- Around line 1544-1555: The THP test setup currently conflates missing,
malformed, and inaccessible `/proc` data with the intentional unavailable-system
case. Update `thpEnabled` and `initThp` so malformed or unexpected status
content and non-`hidepid` I/O or permission errors are surfaced, while only the
documented `hidepid` condition remains skippable; preserve the `it.if(initThp()
=== "1")` behavior and provide an explicit reason for that expected skip.
🪄 Autofix
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: b12a66d7-edce-4b0a-b0a6-0de7f8259306
📒 Files selected for processing (4)
scripts/build/deps/mimalloc.tsscripts/build/deps/webkit.tssrc/bun_alloc/lib.rstest/js/bun/spawn/spawn.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…t gate
Thread stacks were the one large region the old process-wide
PR_SET_THP_DISABLE covered that no allocator owns. Kernels before 6.7
don't imply VM_NOHUGEPAGE for MAP_STACK, so on THP enabled=always hosts a
>=2 MiB stack could fault 2 MiB at a time. madvise it at thread start from
configure_named_thread{,_no_js}, which every long-lived spawned thread
already calls.
Also update the pinned mimalloc sha in process.versions' test, and gate
the spawn THP test on the parent's THP_enabled rather than PID 1's so an
older bun wrapping the test binary skips instead of failing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/bun_core/Global.rs`:
- Around line 562-570: Update the transparent huge-page setup around
pthread_attr_getstack, madvise, and pthread_attr_destroy to inspect each return
status, while keeping pthread_attr_destroy unconditional. Ignore only documented
unsupported THP errors; report all other failures through the existing logging
mechanism, without propagating them or failing thread startup.
🪄 Autofix
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: 4ea48a48-e775-4d95-857c-798deb2e64f6
📒 Files selected for processing (4)
src/bun_core/Global.rssrc/bun_core/output.rstest/js/bun/spawn/spawn.test.tstest/js/node/process/process.test.js
The stack madvise runs on every thread start against memory bun doesn't map itself and only matters on kernels before 6.7, which we can't exercise here. Split it out so it can be measured on its own. No-Verification-Needed: removes code added in the previous commit; bun_core matches main again
No-Verification-Needed: dependency pin bumps
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/bun_alloc/lib.rs (1)
1613-1620:⚠️ Potential issue | 🟡 MinorHandle the
madviseresult explicitly.The return value of
libc::madviseis ignored. If the syscall fails,bss_mmap_noreservereturns a mapping without the requestedMADV_NOHUGEPAGEpolicy and emits no diagnostic. Keep the mapping usable because this is an advisory hint, but preserveerrnoand route the failure through the established best-effort policy.As per coding guidelines, syscall failures must be handled explicitly.
🤖 Prompt for 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. In `@src/bun_alloc/lib.rs` around lines 1613 - 1620, Handle the return value of libc::madvise in bss_mmap_noreserve explicitly: on failure, capture errno, preserve it, and report the error through the existing best-effort policy while still returning the usable mapping. Keep MADV_NOHUGEPAGE advisory and limit the change to the Linux/Android block.Source: Coding guidelines
🤖 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 `@test/js/bun/spawn/spawn.test.ts`:
- Around line 1555-1560: Extend the spawn THP test to inspect the anonymous
mapping created by bss_mmap_noreserve and assert its VmFlags include nh, rather
than relying only on THP_enabled. Preserve the existing child-policy and
exit-code assertions while adding equivalent mapping-level coverage for
MADV_NOHUGEPAGE.
---
Duplicate comments:
In `@src/bun_alloc/lib.rs`:
- Around line 1613-1620: Handle the return value of libc::madvise in
bss_mmap_noreserve explicitly: on failure, capture errno, preserve it, and
report the error through the existing best-effort policy while still returning
the usable mapping. Keep MADV_NOHUGEPAGE advisory and limit the change to the
Linux/Android block.
🪄 Autofix
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: 9cfdb693-289a-4edf-a12a-8131ccf87b2b
📒 Files selected for processing (5)
scripts/build/deps/mimalloc.tsscripts/build/deps/webkit.tssrc/bun_alloc/lib.rstest/js/bun/spawn/spawn.test.tstest/js/node/process/process.test.js
There was a problem hiding this comment.
Both earlier concerns are resolved — the test now gates on /proc/${process.ppid}/status, and WEBKIT_VERSION is pinned to the merged 40-hex SHA rather than the preview tag. The in-repo diff looks correct to me; deferring only because the substance of the change lives in the mimalloc and WebKit bumps (allocator/JSC THP behavior on Linux), which is dep-bump + perf territory that warrants a maintainer's sign-off.
What was reviewed
bss_mmap_noreserve:madvise(p, len, MADV_NOHUGEPAGE)is Linux/Android-gated, runs after theMAP_FAILEDcheck, and the discarded return is justified on the thread.- New spawn test: gate reads the parent's
THP_enabled(not PID 1), skips on non-Linux/hidepid, asserts child + self both read1, exit code last. process.versionsmimalloc hash matches the newMIMALLOC_COMMIT;WEBKIT_VERSIONis a plain SHA soprebuiltUrl()producesautobuild-<sha>.
Extended reasoning...
Overview
Five files touched: two dep-pin bumps (mimalloc.ts commit hash + comment rewrite, webkit.ts version SHA), an 8-line addition to src/bun_alloc/lib.rs calling madvise(MADV_NOHUGEPAGE) on the lazily-faulted BSS arena right after mmap, a new Linux-gated regression test in spawn.test.ts, and the matching process.versions.mimalloc hash in process.test.js. The behavioral change — swapping process-wide prctl(PR_SET_THP_DISABLE) for per-VMA MADV_NOHUGEPAGE — lives in oven-sh/mimalloc#12 and oven-sh/WebKit#388, both now merged and pinned by SHA (commit f2baed1).
Security risks
None identified. madvise(MADV_NOHUGEPAGE) is an advisory VMA flag on memory the process just mapped; no untrusted input, no privilege change, no data exposure. The test reads /proc/<pid>/status for its own parent and a child cat.
Level of scrutiny
The in-repo Rust/TS delta is small and mechanical, but the effective change is to the global allocator's THP policy on every Linux build plus JSC's reservation behavior — squarely in the Dependencies & vendoring and Performance sections of .claude/docs/landing-prs.md. The RSS table in the description shows parity with main (30.3 MB vs 30.2 MB for bun -e 1), which is the right evidence, but I can't independently verify the vendored diffs or the per-platform artifact availability from here. That's the piece a maintainer should confirm.
Other factors
Since my last review: the preview-PR WebKit pin I flagged has been replaced with the merged ddea71318fec… SHA, and the test gate I flagged now reads process.ppid instead of PID 1. All CodeRabbit threads were resolved by dylan-conway with reasoning I agree with (the unchecked madvise return matches mimalloc/WebKit/tcmalloc; the anonymous arena can't be reliably identified in smaps for a VmFlags: nh assertion). The earlier Global.rs thread-stack change was dropped from this PR entirely. No outstanding threads and no new findings this run — the remaining ask is just an explicit maintainer OK on the two dep bumps.
### What does this PR do? `scripts/build/deps/mimalloc.ts` passes `-DMI_NO_SET_VMA_NAME=1` on Linux with a comment claiming it disables mimalloc's `prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, ...)` after each mmap. No revision of oven-sh/mimalloc has ever referenced that macro (`git log --all -S MI_NO_SET_VMA_NAME` is empty; the call in `src/prim/unix/prim.c` is gated only on `defined(PR_SET_VMA)`), so it has been a no-op since it was added and mimalloc's regions have always shown up as `[anon:mimalloc]` in `/proc/<pid>/maps`. That labelling is genuinely useful for RSS attribution (it's how the numbers in #36990 were broken down), and the cost is one prctl per arena reservation, so this keeps the behavior and deletes the dead define plus its misleading comment. Same for the copy in `scripts/bench-json-rust.sh`. No functional change: the compiled mimalloc object is identical with or without the define. ### How did you verify your code works? `grep -rn MI_NO_SET_VMA_NAME` over the repo and over oven-sh/mimalloc history is empty after this change; a Linux release build still shows `[anon:mimalloc]` VMAs in `/proc/self/maps`, same as before. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
What does this PR do?
On Linux, mimalloc (built with
MI_DEFAULT_ALLOW_THP=0) kept its arenas off transparent huge pages by callingprctl(PR_SET_THP_DISABLE)at startup. That flag is process-wide, inherited acrossforkand preserved acrossexecve, so every process bun has spawned since April 2024 —Bun.spawn,bun run, lifecycle scripts — ran with THP disabled, including its own explicitMADV_HUGEPAGErequests:This bumps mimalloc to oven-sh/mimalloc#12 (merged), which opts out per mapping (
MADV_NOHUGEPAGE, a VMA flag dropped atexecve) instead, and does the same forbun_alloc's lazily-faulted 4 MB arena, which the process-wide flag had covered incidentally. JSC's own reservations get the same treatment in oven-sh/WebKit#388 (merged;WEBKIT_VERSIONbumped to its main autobuildddea71318fec). Unlike #35691 there is no per-spawn work and no vfork shared-mm restore.RSS on a THP
enabled=[always]host (x64 release;madvisehosts are unaffected either way):bun -e 1peakBun.servehelloTHP_enabledThe 3 MB between rows 2 and 3 is entirely
[anon:JSStructureHeap].How did you verify your code works?
New case in
test/js/bun/spawn/spawn.test.ts(gated on the parent'sTHP_enabledbeing 1; the spawned child must report 1 too): fails against a main build (Received: "0"), passes on this branch, on an Arch box with THP[always]. Per-region RSS/AnonHugePages compared via/proc/self/smapsfor each configuration above.