Repository navigation
Conversation
…d(An-B) selector FileSystemRouter kept a boxed arena that nothing read. RouteConfig owns its strings, so the arena held only a temporary copy of fileExtensions, and reload() created a second arena that it never allocated from. Remove the field and copy each extension into its Box once. nth::parse_number_saturate created an arena for the tokenizer that reads the digits of an An-B argument written without spaces (2n-1). Pass the arena of the calling parser instead.
|
Updated 6:09 AM PT - Sep 16th, 2026
✅ @robobun, your commit a481c3b527d38f178032616e9880bdbbc3ef5955 passed in 🧪 To try this PR locally: bunx bun-pr 42920That installs a local version of the PR into your bun-42920 --bun |
|
Status: ready for review. How to reproduce on main. // router.js: prints 1 heap per call for both on main, 0 with this PR
const fs = require("fs"), path = require("path"), os = require("os"), { heapStats } = require("bun:jsc");
const heaps = () => heapStats().mimalloc.heaps.total;
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "r-"));
fs.writeFileSync(path.join(dir, "index.tsx"), "export default 1");
const router = new Bun.FileSystemRouter({ style: "nextjs", dir });
let before = heaps();
for (let i = 0; i < 100; i++) new Bun.FileSystemRouter({ style: "nextjs", dir });
console.log((heaps() - before) / 100, "heaps per constructor call");
before = heaps();
for (let i = 0; i < 100; i++) router.reload();
console.log((heaps() - before) / 100, "heaps per reload()");
fs.rmSync(dir, { recursive: true });// nth.js: prints about 16 then 2016 heaps per build on main. With this PR the two counts are equal (16 to 18).
const fs = require("fs"), path = require("path"), os = require("os"), { heapStats } = require("bun:jsc");
const heaps = () => heapStats().mimalloc.heaps.total;
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nth-"));
for (const arg of ["2n+1", "2n-1"]) {
const file = path.join(dir, "a.css");
fs.writeFileSync(file, Array.from({ length: 2000 }, (_, i) => `.a${i}:nth-child(${arg}){color:red}`).join("\n"));
for (let i = 0; i < 3; i++) await Bun.build({ entrypoints: [file] });
const before = heaps();
await Bun.build({ entrypoints: [file] });
console.log(`:nth-child(${arg}) x 2000:`, heaps() - before, "heaps per build");
}
fs.rmSync(dir, { recursive: true });The new tests assert the same counts: |
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change reuses existing arenas in CSS ChangesAllocator cleanup
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The allocator changes preserve selector parsing, route behavior, and reload replacement while removing redundant heap creation. No blocking risk is identified. 🚥 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.
I reviewed this PR and didn't find any bugs; a quick human look at the test metric would still be worthwhile since I could not run the new tests in this checkout.
What was reviewed:
FileSystemRouter: confirmed the removed arena had exactly one consumer (thefileExtensionscopies, which were already re-boxed intoRouteConfig.extensions),Router::Routercarries no lifetime so nothing inroutercan borrow from it, andreload()never touched it beyond the swap — the "old router borrows from old arena" comment was already stale.- CSS
nth: the sub-tokenizer input is guaranteed-<digits>byparse_n_dash_digits, so nothing is allocated in the caller's arena and no output changes; the three call sites andParser::arena()(css_parser.rs:3108) line up. - Tests: both fail on main by construction (100 vs 1 heaps; 50 vs 0). The one thing I could not verify here is
heapStats().mimalloc.heaps.totalitself — the vendored mimalloc source is absent from this checkout, so whether that counter also increments for thread-default heap init (which would make the exact0/ equal-count assertions flaky if any helper thread starts mid-loop under debug/ASAN CI) is worth a human confirming.
Extended reasoning...
Overview
Two independent allocator cleanups. In src/runtime/api/filesystem_router.rs the arena: JsCell<Box<ArenaAllocator>> field is deleted along with the unsafe detach_lifetime block that copied fileExtensions entries into it; the entries are now collected directly as Vec<Box<[u8]>> and moved into RouteConfig.extensions via into_boxed_slice(). reload() no longer allocates a fresh arena per call. In src/css/css_parser.rs (mod nth), parse_n_dash_digits and parse_number_saturate gain an arena: &Bump parameter and the three call sites pass input.arena() instead of constructing a throwaway Bump::new() (which is bun_alloc::Arena, i.e. an mi_heap_new/mi_heap_destroy pair). Two tests are added that count mimalloc heaps via heapStats().mimalloc.heaps.total.
Security risks
None. No user-controlled data changes its validation or path; the [1..] slicing of extension strings is preserved verbatim from the old code (with the existing get_length == 0 guard), and the CSS change only substitutes which arena backs a tokenizer whose input is already constrained to -<digits>.
Level of scrutiny
Moderate for the Rust changes, which I read fully: I traced every prior use of the router arena (only the extension copies), confirmed Router::Router has no lifetime parameter so JsCell<Router::Router> cannot borrow from the removed field, and checked that reload()'s snapshot-and-swap still works. For CSS, parse_n_dash_digits verifies bytes[2..] are all ASCII digits before calling parse_number_saturate with &str[1..], so the sub-parser only ever sees -<digits> and produces a Token::Number, which does not allocate from the arena; even if it did, the caller's arena outlives the stylesheet parse. Both changes are behavior-preserving by inspection.
Other factors
No debug build exists in this checkout and Bash was denied for build/run, so I could not execute the new tests. The vendored mimalloc tree is also absent, so I could not confirm from source that heaps.total exists in the oven-sh fork's mi_stats_get_json output or whether it counts only mi_heap_new heaps versus thread-default heap initialization. The author reports 20 clean runs on a release build; the residual risk is a flaky exact-0/equal-count assertion under debug/ASAN CI if a helper thread's heap init is counted. That is a CI-visible concern rather than a correctness bug in the runtime change, which is why I defer rather than approve or stay silent.
|
On the open question in the review above: Source (
The first allocation of a new thread creates a theap. That increments a different counter, Run. The script reads both counters, starts work pool threads (64
Scriptconst { heapStats } = require("bun:jsc");
const crypto = require("crypto"), fs = require("fs"), os = require("os"), path = require("path");
const snap = () => { const m = heapStats().mimalloc; return { heaps: m.heaps.total, theaps: m.theaps.total }; };
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "t-"));
fs.writeFileSync(path.join(dir, "index.tsx"), "export default 1");
const before = snap();
await Promise.all([
...Array.from({ length: 64 }, (_, i) => new Promise((resolve, reject) => crypto.pbkdf2("p" + i, "salt", 2000, 32, "sha256", (e, k) => (e ? reject(e) : resolve(k))))),
...Array.from({ length: 64 }, () => fs.promises.readFile(path.join(dir, "index.tsx"))),
]);
const afterThreads = snap();
for (let i = 0; i < 50; i++) new Bun.FileSystemRouter({ style: "nextjs", dir });
const afterRouters = snap();
console.log(JSON.stringify({ before, afterThreads, afterRouters }));
fs.rmSync(dir, { recursive: true });So only a I added one comment line to each test that says what the counter counts. |
This branch has not been deployed
2 participants
Problem
new Bun.FileSystemRouter()androuter.reload()each create one mimalloc heap that nothing uses. Construction takes 5.6 us on main and 2.1 us on 1.3.14. The cause isBox::new(ArenaAllocator::new())atsrc/runtime/api/filesystem_router.rs:171and:439. Thearenafield is dead:RouteConfigowns its strings asBox<[u8]>.:nth-child(An-B)selector written without spaces (2n-1).Bun.buildof 2,000 such rules takes 10.7 ms, and 6.1 ms with2n+1. The cause isBump::new()innth::parse_number_saturate(src/css/css_parser.rs:4027).Fix
arenafield ofFileSystemRouter. The constructor copies eachfileExtensionsentry into itsBox<[u8]>one time. Oneunsafeblock goes away.parse_n_dash_digitsandparse_number_saturatetakeinput.arena()from the calling parser, as the Zig parser in 1.3.14 did.-<digits>only, and a number token allocates nothing.test/js/bun/util/filesystem_router.test.tsandtest/js/bun/css/nth-anplusb-ident.test.ts. The 5 new tests fail on main. Alsocss.test.ts,doesnt_crash.test.tsandtest/regression/issue/18242.test.ts.Background
bun_alloc::ArenaisMimallocArena.Arena::new()callsmi_heap_newandDropcallsmi_heap_destroy. The pair costs more than 1 us with no allocation. The Zig code used a bump arena, which is free to create.heapStats().mimalloc.heaps.totalfrombun:jsccounts every heap that the process created. The tests read it before and after a loop.2n-1as one dimension token with the unitn-1. The parser reads the digits aftern-with a second tokenizer. A tokenizer uses its arena only for a name or a string that has an escape.Notes
Scope. Other host functions also call
Arena::new()one time per call. This PR changes only these two sites. Both changes stay correct under a wider fix (a pooled arena, or a heap that is created on the first allocation): the router arena is dead code, and the CSS arena is the arena of the caller.Measurements. Release builds of c6b7fcb with this diff ("PR") and without it ("main"), plus the 1.3.14 release from npm. Each number is the median of 15 fresh processes per build, with the builds interleaved. The machine was loaded (load1 60 to 98), so compare the columns, not the absolute values.
new Bun.FileSystemRouter({ style, dir }), 1 routefileExtensions: [".tsx", ".ts"]router.reload()router.match("/")(control)reload()Bun.build, 2,000 x.aN:nth-child(2n-1){color:red}Bun.build, 2,000 x.aN:nth-child(2n+1){color:red}(control)Bun.build, 2,000 x.aN:hover{color:red}(control)2n-1fileThe parser alone (
cssInternals.minifyTest, 2,000 rules, one thread):2n-17.11 ms to 3.56 ms,2n+13.60 ms to 3.51 ms.An A/A run of the
Bun.buildbenchmark (the main binary against a copy of itself) gave ratios from 0.91 to 1.07 on this machine. The controls above are inside that band.The constructor with
fileExtensionsis the slowest cell on main because it is the only one that writes to the new heap, and each heap stays alive until the GC collects its router.Why the heap count is exact in the tests. Both tests run synchronous code on the JS thread with no other work in the process. The router test expects 0 heaps for 50 constructor calls and for 50
reload()calls. The CSS test expects the same count for 100 rules as for 1 rule, for each of the three call sites ofparse_n_dash_digits(2n-3,n-5and-n-5,+n-7). I ran both 20 times on the release build with no failure.Other suites.
test/js/bun/css/css-fuzz.test.tspasses on the release build with and without this diff. On the debug build its 10 s limits expire on this machine, and CI does not run that file (if (!isCI)).no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts