Repository navigation
Conversation
A guard. It changes no machine code on main. resolve_rope_if_needed and flattened are a check plus a call to flatten_rope. The printer, the parser and the linker call them for every string, and they inline there only while they are that small. Release builds share generic instances across crates. On main the inliner leaves flatten_rope out of line by a margin of 25 (cost 275, threshold 250). With the instance of ArenaVec<u8>::extend_from_slice in a crate above bun_ast, it folds flatten_rope into both callers, and every string pays a call.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. Walkthrough
ChangesEString annotation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change affects optimization rather than string behavior, and no concrete release risk is established. It is mergeable. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — a two-line codegen hint on a cold path, no logic or signature change.
What was reviewed:
#[inline(never)]onEString::flatten_rope(src/ast/e.rs:1920): only reached after thenext.is_none() || !is_utf8()early-return in its two callers, so an extra call is paid only on actual ropes; the non-rope common case is unchanged.- Both callers (
resolve_rope_if_needed,flattened) still call it with identical arguments; no ownership or lifetime change since the returned slice is bump-allocated either way. - Compared with the crate's other
#[inline(never)]sites (src/ast/lib.rs:1618, src/ast/known_global.rs:81): same placement pattern, so the attribute is consistent with local convention.
Extended reasoning...
The change touches only src/ast/e.rs, adding a #[inline(never)] attribute and a one-line PERF comment to the private flatten_rope helper; no security-sensitive surface is involved. Behavior, signatures, and ownership are unchanged, and the attribute only affects codegen of a path already gated behind a rope check, so it cannot regress the non-rope common case. No CODEOWNERS entry covers src/ast, no prior reviewer objections exist, and the bug hunt ran dry, which together decided approve.
|
Status: ready for review. The diff is 2 lines in CI (build 121596): 180 of 181 jobs pass. The job that fails is How the cost was reproduced (linux-x64, release profile, main 9f70da0):
PR: #44230 |
Problem
ArenaVec<u8>withextend_from_slicetobun_core, as the review on Decode text and md imports as UTF-8 instead of passing raw file bytes to the printer #38253 asks. This PR goes first.bun buildpays a call per string: +0.10 % instructions on a 9.1 MBtypescript.js, +1.98 % on a mime-db import, +28 per line of synthetic JS.EString::resolve_rope_if_neededandEString::flattened(src/ast/e.rs:1899,:1908) inline at every string only while small. In such a build the inliner foldsflatten_ropeinto both.Fix
EString::flatten_rope#[inline(never)]. Its two callers stay a check plus a call.Background
EStringthat constant folding joined from several literals.flatten_ropecopies the parts into one slice.-Zshare-generics=y(scripts/build/rust.ts:367). A generic function without#[inline]then has one instance, in the upstream-most crate that uses it.flatten_ropeby 25 (cost 275, threshold 250). A crate that sees only a declaration accepts it.Downsides
Behaviour change: none
Notes
Method. linux-x64, release profile (ThinLTO,
-Zshare-generics=y). Each build below is a full build. Functions are compared by a hash of their disassembly with addresses masked. Instruction counts are user-mode instructions per function, JIT off,GOMAXPROCS=2, three runs. A delta sums the functions whose count is the same in every run of both binaries. That leaves out allocator and thread pool timing. The counts come from a DynamoRIO client, because the machine deniesperf_event_openand has no valgrind. The output files are byte-identical in every build.Two bases. The function comparisons of this exact diff are against main 9f70da0. The instruction counts and the alternatives are against main 36cd151, with the same attribute. The 9 commits between them do not touch
src/ast/e.rs,src/bun_alloc/baby_vec.rsorsrc/js_printer/lib.rs.The trigger. One function in
src/bun_core/string/immutable.rsthat doesArenaVec::<u8>::with_capacity_inandextend_from_slice, with one call fromTranspiler::build_css_output. #42753 and #38253 add that same pair to that file.This diff against main 9f70da0.
flatten_roperesolve_rope_if_needed,flattenedout of lineflatten_rope.note.gnu.build-id..textis 58,148,853 B in both.flatten_ropeand the trigger's caller.Instruction counts against main 36cd151.
Bun.Transpiler, 1 MB of TypeScript#[inline]on the two callers#[cold]onflatten_rope#[inline]onBabyVec::extend_from_sliceonlyglobalThis.aN="x..x";(+0.34 % to +0.38 % with the trigger), a JSON import with as many keys (+1.27 % to +1.41 %), and JS that joins three literals on every line, built with--minify-syntax.bun run, 5,000 lines of JSbun runwith a JSON import, 5,000 keysbun build --sourcemap=linked, JSbun build --sourcemap=linked, JSON importbun build --target=bun, JSON importbun build --target=bun --sourcemap=linked, JSON importbun pm pkg geton apackage.jsonwith 5,000 keysbun build --target=bun typescript.js(9.1 MB)bun buildof an import of the mime-db JSON (204 KB)The values of this attribute that are not 0 are noise of the run: the machine code is the same as main's.
The margin on main. rustc's inline remarks for
bun_ast(-Cremark=inline, the release command line, at 36cd151) reportflatten_ropeas too costly for each of its callers: cost 275, threshold 250. With this PR they reportNeverInline.Where the slowdown was seen. In the pre-merge instruction check of #41801, and in the builds above. No user reported it, and no CI job counts instructions. The binary size does not show it:
.textshrinks by 1,536 B with the trigger.Not covered.
with_capacity_inandpushmovesgrow_toandgrow_exacttobun_coreand leavesextend_from_sliceinbun_ast. It does not change the rope path, with or without this PR. It changes 45 functions in the parsers named above. This PR does not change that.BufferWriter::write_all(src/js_printer/lib.rs:7499) callsVec<u8>::extend_from_sliceout of line, 108 instructions per JS line on main.EString::eql_comptimeandEString::stringare out of line on the JSON path, 60 and 34 per key.scripts/build/rust.ts:357says that the flag only removes duplicates of bodies that nobody inlines. The builds above are a case where it changes what is inlined. This PR does not edit the note.Tests on the debug build of this commit. All pass:
transpiler.test.js(237),bundler_dynamic_import_dce.test.ts(333),importstar.test.ts(99),bundler_string.test.ts(59),bundler_minify.test.ts(54),transpiler-enum-concat-chain-oom.test.ts(2), the rope test ofbun-build-api.test.ts, and the source lintprinter-rope-in-place.test.ts.Order. #42753 and #38253 keep their
ArenaVec<u8>when they merge after this PR. #41801 wrote its copy into an arena slice to avoid the cost (4175d77) and can useArenaVec<u8>again.no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check