Repository navigation
Conversation
…rce length P::init reserves its symbol tables from the length of the source, about 8 bytes for each byte of it. to_lazy_export_ast has no parse pass, so the module of an asset, a JSON file or a text file reserved that much for tables that stay empty. P::init_with_text_len takes the length as an argument. The lazy export passes 0.
An ASAN build keeps the copy for the output file out of mimalloc, so the growth there is 1.1 times the asset. A lower bound of the whole asset left a margin of 6%.
P::init sized the symbol tables of the parser from the length of the source, about 8 bytes for each byte of it. bun test builds one parser over the whole test file for each inline snapshot it writes, and parses only the arguments of one matcher call with it. A test file with many new inline snapshots reserved that much once for each snapshot, and more than half of it was never freed. P::init now passes 0 to init_with_text_len. The passes over the whole source (_parse, _scan_imports, parse_only) pass the length of the source themselves: init_p! always calls init_with_text_len.
|
Updated 1:44 AM PT - Oct 27th, 2026
❌ @robobun, your commit bcd1c02 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46404That installs a local version of the PR into your bun-46404 --bun |
|
Status How it was reproduced, on release builds of main 71bc818 with #46090, linux x64: # snap.test.ts: one comment line of 1,453,185 bytes, then 414 lines `expect(i).toMatchInlineSnapshot();`
# inside one test. That is 1,469,296 bytes, the shape of test/cli/check/check.test.ts.
env -u CI bun test ./snap.test.ts # peak RSS is VmHWM from /proc/<pid>/status
PR: #46404 CI. The change is green. The one red job is a failure of a CI machine.
|
…ne-snapshot-parser-presize # Conflicts: # test/js/bun/test/snapshot-tests/new-snapshot.test.ts
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughParser initialization now uses a supplied text length to estimate symbol and scope capacity. Parsing entry points pass the source length, and new bundler and snapshot tests measure memory use in those scenarios. ChangesParser Capacity Hint and Memory Tests
Suggested reviewers: Priority: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: P::init starts with empty symbol tables, and whole-file passes provide the source length. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description follows the repository template with both required sections: "What does this PR do?" (Problem, Fix, Background, Downsides) and "How did you verify your code works?" (Verified, test det… |
Full details: Description check
-
Explanation
The description follows the repository template with both required sections: "What does this PR do?" (Problem, Fix, Background, Downsides) and "How did you verify your code works?" (Verified, test details, suites run, measurements). It names the test added, the commands and build profiles used, and what was not verified (macOS, Windows). It also states the stacking dependency on
#46090and the remaining unfixed paths. The description is thorough and specific, and the claims are backed by measurements. Minor gap: the template's verification section is long and the core change is spread across a details block, which makes the reviewer work harder to find the key points. Nothing required is missing.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Merged main (8a0e49e) into this branch.
|
The merge of main kept the elided lifetime of this signature. main changed P::init to take &'a Source<'a> (#46834).
Stacked on #46090: this branch holds its commits. Merge that first.
Problem
bun testbuilds one parser over the whole test file for each inline snapshot it writes. The parser's constructor (P::init) sized its tables from the file: 8,389,376 bytes for 1 MiB, 4,849,856 never freed.ulimit -v 4000000it ends inoh no: Bun has run out of memory.and writes none.Fix
P::initpasses 0 toinit_with_text_len: the tables start empty, as in 1.3.x._parse,_scan_importsandparse_onlypass the source length.init_p!requires it.test/js/bun/test/snapshot-tests/new-snapshot.test.ts(1 new test, fails without it). Also ransnapshots/snapshot.test.ts,bundler_loader.test.ts,transpiler.test.js.snapshot.rs(separate change), js_parser: do not size a lazy-export AST's symbol tables from the source length #46090's branch as the base (CI cannot build it).Background
Pis the JavaScript parser. Its constructor sizes four tables from the source (Rewrite Bun in Rust #30412, bundler: let a nested binding keep its name unless it would capture a reference #41286). js_parser: do not size a lazy-export AST's symbol tables from the source length #46090 made that size an argument.Poutsidebun_js_parser.init_with_text_lenwith 0 from the test runner (not built).P::initkeeps the whole-file default then, and bun:test: remove unsafe from the test runner #40235 calls it.Downsides
_parsesizes the tables for a cached or// @bunmodule and parses nothing.Notes
Where this comes from. No user reported it. The review of #46090 found the snapshot writer as a second place that builds a
Pwith no whole-file pass: #46090 (comment). It is a developer-machine path: in CI,bun testrefuses a new inline snapshot without--update-snapshots.Stacking. This branch holds the commits of #46090, so this page shows its diff too:
bundler_loader.test.ts, thetext_lenargument and the 0 of the lazy export are from #46090. The change of this PR is the commit 4bc326e, 3 files, +55 -15:P::initpasses 0,init_p!has one arm, its four call sites, and the test innew-snapshot.test.ts. It removes the second arm that #46090 adds toinit_p!, and the whole-file default of itsP::init. The branch of #46090 cannot be the base of this PR: CI does not build a branch without commit b7efd52 of main, and that branch does not have it. After #46090 merges, I rebase the one commit onto main.Question for a maintainer. Which default should the public
P::inithave? Here it starts empty, and the three whole-file passes pass their length inside the crate. The review of #46090 named the other shape: keep the whole-file default, makeinit_with_text_lenpub, pass 0 fromsnapshot.rs. I did not build that shape. I expect the same release code from it, because the writer's call here is already a directinit_with_text_len(.., 0). The switch is about 3 lines. #41861 fixed the same class in the renamer: a table sized from the whole input, one for each unit of work.History. The writer had this shape in Zig too: one parser for each snapshot, one arena for each file. The Zig
P.initmade the tables empty. The Rust port sizesmembersandsymbol_usesfromsource.contents.len() / 16(#30412, in 1.4.0). #41286 (in 1.4.1) added the scope list atlen / 64and thesymbolslist atlen / 16.Where the bytes went. For a file of 1,048,762 bytes, each parser made six allocations:
symbolsmembersAstAllocsymbol_uses(3 columns)AstAllocwrite_inline_snapshotsinstalls no AST allocation scope.AstAllocthen uses the global heap, and itsdeallocatedoes nothing. hashbrown writes one control byte for each bucket ofmembers: that is the RSS.Measurements. Release builds, linux x64 only. Base is main 71bc818 with #46090 (fd66e0a71). Each test file is generated: one comment line for the size, and N calls of
expect(i).toMatchInlineSnapshot(). Peak RSS is VmHWM and the address space is VmPeak, both read from/proc/<pid>/statusat exit.test/cli/check/check.test.ts)ulimit -v 4000000ulimit -v 11000000--parallel=2, two files of 2000 snapshots andulimit -v 11000000, the workers of the base abort when they write their snapshots. The run still exits 0, with 0 of 4000 written. bun test --parallel: fail the run when a worker is killed or exits non-zero outside a test file #44431 is about that exit code. With this PR: 4000 of 4000.write_inline_snapshots: 21 (29,362,949 bytes) in the base, 3 (4,194,821 bytes) here. The 3 are the file and the result buffer. Blocks that nothing frees: 12 (14,549,568 bytes) in the base, 0 here.size: text 107,319,777 in both builds, data and bss the same. All 105,435 text symbols have the same address and size.init_with_text_len, both_parse, both_scan_imports, bothparse_onlyandto_lazy_export_asthave 0 instructions that differ, of 16,916.write_inline_snapshotshas 1 of 2,167:push QWORD PTR [rbp-0x1d8]is nowpush 0x0.transformSyncof a module of 6,400 bytes: 9 allocator calls before the parse pass in both builds. The constructor asks for 2400, 128, 14864, 3200, 1600, 1600 and 19200 bytes in both.perf_event_open, runs of base and PR interleaved): a coldrequireof 1000 modules of 15,403 bytes is +0.006% (15 runs each, two copies of the base differ by 0.021%).bun buildoftypescript.jsis +0.017% (7 runs each, two copies of the base differ by 0.028%)._parsepasses 0 is +0.666% on the 1000 modules. That is what the size is worth there.The test. The child
bun testruns a file with a comment of 512 KiB and 128 emptytoMatchInlineSnapshot()calls.BUN_TEST_DRAIN_EVENT_LOOP=1makesbun testrun theexitlisteners of the file, after it wrote the snapshots. The listener prints how muchheapStats().mimalloc.committed.peakgrew sinceafterAll. The test asserts the text of the rewritten file, and a growth below 128 times the comment (67,108,864 bytes).The test is in
new-snapshot.test.ts, not in theinline snapshotsblock ofsnapshots/snapshot.test.ts. That file passes only withFORCE_COLOR=1, which the CI runner sets: itserror snapshotstest has colors in its snapshot. A plainbun bd testof it fails with and without this change.Not changed. No issue tracks these yet.
_parsebuilds the parser, and then returns for a transpiler-cache hit or a// @bunfile with no parse pass. For a module of 1,048,576 bytes both make the 6 allocations, 8,388,624 bytes (gdb, on the build of this PR). transpiler cache: answer a hit before the parser and its heap are set up #43834 (draft) moves the cache lookup ahead of the parser._parseis the hot path, so a change there needs its own numbers.Snapshots::parse_fileparses each.snapfile with no AST allocation scope, so theAstAlloctables of that parse are not freed. 10 test files with a.snapof 1 MiB each leave 48,627,712 bytes of large blocks live at the end, 1 file leaves 7,929,856. It is the same in the base.{ a: expect.any(Number) }.bun replparses each input with no AST allocation scope too (gdb: the same four allocations come from the global heap).bun pm diffhas none either, by a read ofpm_diff_normalize.rs. Not measured.Suites run. On a debug build with ASAN:
test/js/bun/test/snapshot-tests/new-snapshot.test.ts,test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts(withFORCE_COLOR=1),test/bundler/bundler_loader.test.ts,test/bundler/transpiler/transpiler.test.js. The new test also ran on the release builds of the base and of this change.Not run. macOS, Windows. On Windows, mimalloc commits a large block when it allocates it (
has_overcommit = falseinvendor/mimalloc/src/prim/windows/prim.c), so each of these tables counts against the commit limit there. I did not measure that.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file