Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesIdle allocator cleanup
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete merge-blocking regression was identified in the allocator cleanup scheduling or its Linux-specific test coverage. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status #42853 merged the pin alone, so this PR is rebased and now carries what main does not have: the test for oven-sh/mimalloc#52 and the forced collect at the idle full collection. How the idle cost reproduces on main (release build, Linux x64): start How the test for oven-sh/mimalloc#52 fails with the pin before #42853 (11 of 12 bursts fault all 512 pages in again, none with the current pin): // MIMALLOC_PURGE_HOLES_MIN_INTERVAL=10 bun repro.mjs
import { readFileSync } from "node:fs";
const faults = () => {
const stat = readFileSync("/proc/self/stat", "utf8");
const fields = stat.slice(stat.lastIndexOf(")") + 2).split(" ");
return Number(fields[7]) + Number(fields[9]);
};
const burst = () => {
const buffers = [];
for (let i = 0; i < 4; i++) buffers.push(new Uint8Array(512 * 1024).fill(1));
for (const b of buffers) b.buffer.transfer(0);
};
for (let i = 0; i < 3; i++) { burst(); await Bun.sleep(30); }
const perBurst = [];
for (let i = 0; i < 12; i++) {
const before = faults();
burst();
perBurst.push(faults() - before);
await Bun.sleep(60);
Bun.gc(false); // mi_collect(false), the collect JavaScriptCore runs after a full collection
await Bun.sleep(40);
}
console.log(JSON.stringify(perBurst));
// before #42853: [6,513,512,512,512,512,512,512,512,512,512,512]
// now: [4,0,0,0,0,0,0,0,0,0,0,0]This PR: #42832 CI The diff is green. The new tests pass on every lane that runs them (builds 116178, 116207, 116213 and 116395). The one red test in each build is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new mimalloc_cleanup(true) call under ASAN builds — bun_core::Global::mimalloc_cleanup is gated on USE_MIMALLOC (cfg!(not(bun_asan))), so it is a no-op there rather than a call into a non-mimalloc allocator; and mi_collect acts on the calling thread's heap, so running it on the JS thread from the controller does not cross a thread boundary.
Extended reasoning...
Findings were posted inline covering the ordering of the forced collect relative to the async idle collection, the behavioral change of the mimalloc pin for threads without a GC controller, and the page-size and base-branch-validity concerns in the new tests. Separately I confirmed from src/bun_core/Global.rs:823 and src/bun_core/lib.rs:2323 that the new call is compiled out under ASAN and only touches the calling thread's mimalloc heap, so those two potential concerns (allocator mismatch under ASAN, cross-thread heap access) are not issues with this change. The dep bump itself and the semantics of the new upstream commit remain unverifiable from the diff alone and warrant a human look alongside the inline comments.
Findings marked 🟡 are optional suggestions and need no follow-up push.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 12:43 AM PT - Sep 16th, 2026
❌ @robobun, your commit dfba93c has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42832That installs a local version of the PR into your bun-42832 --bun |
…e sweep kept (oven-sh/mimalloc#52) Bursts of four 512 KiB buffers with Bun.gc(false) between them, which is mi_collect(false), the collect that JavaScriptCore runs after each full collection. With the pin before #42853, 11 of 12 bursts fault all 512 pages of their buffers in again. With the current pin none does. Linux only, not ASAN, and only where a page is 4 KiB (the test counts faults in pages of that size). linuxPageSize() in the harness reads AT_PAGESZ from /proc/self/auxv.
…een quiet Since oven-sh/mimalloc#52 (#42853) the collect at the end of a JavaScriptCore collection leaves the large pages that mimalloc keeps for the next burst (up to 4 MiB). That collect was what gave them back in an idle process, so an idle server now holds them for as long as it is idle. GarbageCollectionController runs mi_collect(true) when it requests an idle full collection (after BUN_IDLE_GC_SECONDS in which the JS heap did not grow, at most three times in a quiet period, main thread only), and once more at the next timer fire, for what that collection has freed since. Test: the page of six 512 KiB buffers stays while the process is active, also across Bun.gc(false), and is gone after the idle collection. Linux only, not ASAN, 4 KiB pages.
robobun
changed the title
82a0db3 to
dfba93c
Compare
This branch has not been deployed
2 participants
Problem
Bun.serveecho server now keeps them: 5680 KiB of anonymous memory 70 s after 512 KiB bodies, 2424 KiB before the pin.Fix
GarbageCollectionControllerruns a forcedmi_collectwhen it requests an idle full collection, and once more at the next timer fire, for what that collection has freed since. The same server is at 2456 KiB 70 s after the load.Bun.gc(false)between them. 11 of 12 bursts re-fault with the pin before deps: mimalloc to oven-sh/mimalloc#52: a kept large page survives the collect after a garbage collection #42853, none now.test/js/bun/jsc/heapStats-mimalloc.test.tsandtest/js/bun/gc/gc-controller-cadence.test.ts(its new test fails on main). Release build, Linux x64.Background
BUN_IDLE_GC_SECONDS(10 s, then 65 s, 65 s) in which the JS heap did not grow.Notes
This PR had the pin bump too. #42853 merged the pin alone, so the branch is rebased and keeps what main does not have: the two tests and the idle collect. The commits are independent. Drop the second one to keep the idle cost that #42853 states.
Release builds, Linux x64. "pin only" is main since #42853.
Idle echo server (
Bun.servethat answers with the request body, 100 requests at 5 a second, then idle; anonymous RSS in KiB at +10 s / +70 s / +310 s):(The "this PR" column was measured on a later main, so its baseline is about 25 KiB higher.)
What the forced collects cost a server that is not idle. The controller counts a heap as quiet when the JS heap does not grow, so a server that only moves request bodies is quiet while it serves requests. It loses the kept pages at the idle collections (at most three in a quiet period, each with two forced collects). Echo server, 512 KiB body, one request every 2 s, 40 requests, 3 runs, faults per request: before #42853 129.6 / 122.0 / 144.3, pin only 5.7 / 5.1 / 5.5, this PR 22.2 / 23.1 / 21.9. In each run of this PR one request of the 40 re-faults its page (321 faults). The others are as with the pin only.
Why two forced collects.
collect_async_idleonly requests the collection. A forced collect at that moment returns what is already free. The second one, at the next fire of the GC timer (1 s later in fast mode, up to 30 s in slow mode), returns what the collection has freed since. I did not shorten that fire:idle_tickcounts quiet time in nominal tick intervals, and an extra short fire would move the later idle collections closer together than the CodeBlock aging lease.The tests. Both are Linux only, skip under ASAN (malloc is not mimalloc there) and skip when a page is not 4 KiB, because they count faults in pages of that size.
Bun.gc(false)ismi_collect(false), the same collect that JavaScriptCore runs after a full collection, without a dependency on when JavaScriptCore decides to collect. The idle test fails with the pin before #42853 (the page is gone while the process is active) and on main (the page stays after the idle collection).Two older tests in
gc-controller-cadence.test.ts("idle release lets FTL code age out") time out under a debug ASAN build. They do the same with a debug build from before this branch.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/jsc/heapStats-mimalloc.test.ts, test/js/bun/gc/gc-controller-cadence.test.ts