diff --git a/src/css_jsc/color_js.rs b/src/css_jsc/color_js.rs index d39572fd154f..bf34b0e5c177 100644 --- a/src/css_jsc/color_js.rs +++ b/src/css_jsc/color_js.rs @@ -196,6 +196,48 @@ fn zero_if_none(component: f32) -> f32 { if component.is_nan() { 0.0 } else { component } } +/// Run `f` with a borrow of a reused per-thread scratch arena, returning +/// whatever it produces. Used for both halves of `Bun.color` on a string — +/// the CSS parse and the CSS-string serialization. +/// +/// Neither half allocates into the arena on the common path: the tokenizer +/// threads `&'a Arena` through `ParserInput` but idents/numbers/percentages are +/// borrowed sub-slices of the input (see `Tokenizer::consume_name`/ +/// `consume_numeric`), and the printer's `scratchbuf`/`indentation_buf` stay +/// empty for a bare `CssColor` (which serializes straight into a global-heap +/// `Vec`). Only an escape sequence / NUL / non-ASCII byte inside an ident +/// forces a copy-on-write `ArenaVec`, deliberately abandoned into the arena +/// (`CopyOnWriteStr`'s `into_bump_slice`), relying on a bulk free. +/// +/// A fresh `Arena::new()` per call pays a `mi_heap_new()` + `mi_heap_destroy()` +/// round-trip — which profiling attributes ~900ns per site to, dwarfing the +/// work itself — so reuse one warm heap per thread instead and reset it before +/// each use. `reset_retain_with_limit` keeps the heap while its footprint is +/// small (the common case allocates zero, a cheap retain) and only falls back +/// to `mi_heap_destroy` + `mi_heap_new` once abandoned buffers exceed the cap — +/// bounding steady-state memory without leaking. Unlike `borrowing_default()`, +/// any stray allocation lands in this arena and is reclaimed by the next reset +/// rather than leaking into `mi_heap_main()`. +/// +/// The arena borrow is confined to `f` via the `RefCell` guard, so there is no +/// `unsafe`: `f`'s return value carries no arena lifetime, and `Bun.color` runs +/// on the JS thread where neither the tokenizer nor the printer re-enters this +/// — a re-entry would be a safe `RefCell` double-borrow panic, not UB. The two +/// uses within one call are sequential (parse, then serialize), so they take +/// the borrow one after another, never nested. +fn with_color_arena(f: impl FnOnce(&Arena) -> R) -> R { + use std::cell::RefCell; + + thread_local! { + static ARENA: RefCell = RefCell::new(Arena::new()); + } + + ARENA.with_borrow_mut(|arena| { + arena.reset_retain_with_limit(64 * 1024); + f(arena) + }) +} + pub fn js_function_color(global: &JSGlobalObject, frame: &CallFrame) -> JsResult { use bun_ast::symbol::Map as SymbolMap; use bun_core::ZigStringSlice; @@ -324,17 +366,18 @@ pub fn js_function_color(global: &JSGlobalObject, frame: &CallFrame) -> JsResult input = args[0].to_slice(global)?; - // MimallocArena::new() calls mi_heap_new(), so defer creation to the - // paths that actually allocate. - let arena = Arena::new(); - let mut parser_input = css::ParserInput::new(input.slice(), &arena); - let mut parser = css::Parser::new( - &mut parser_input, - None, - css::css_parser::ParserOpts::default(), - None, - ); - break 'brk CssColor::parse(&mut parser); + // Borrow the per-thread scratch arena (see `with_color_arena`); the + // parsed `CssColor` is fully owned, so it outlives the borrow. + break 'brk with_color_arena(|arena| { + let mut parser_input = css::ParserInput::new(input.slice(), arena); + let mut parser = css::Parser::new( + &mut parser_input, + None, + css::css_parser::ParserOpts::default(), + None, + ); + CssColor::parse(&mut parser) + }); }; match parsed_color { @@ -598,27 +641,32 @@ pub fn js_function_color(global: &JSGlobalObject, frame: &CallFrame) -> JsResult return str.transfer_to_js(global); } - // Fallback to CSS string output - let arena = Arena::new(); - let mut dest: Vec = Vec::new(); - - let symbols = SymbolMap::init_list(Default::default()); - let mut printer = css::Printer::new( - &arena, - bun_alloc::ArenaVec::::new_in(&arena), - &mut dest, - &css::PrinterOptions::default(), - None, - None, - &symbols, - ); - - if let Err(err) = result.to_css(&mut printer) { - return Err(global.throw(format_args!("color() internal error: {}", err.name()))); - } - drop(printer); + // Fallback to CSS string output. Borrow the per-thread scratch arena + // (see `with_color_arena`) rather than `borrowing_default()` so any + // stray printer allocation is reclaimed instead of leaking. + return with_color_arena(|arena| { + let mut dest: Vec = Vec::new(); + + let symbols = SymbolMap::init_list(Default::default()); + let mut printer = css::Printer::new( + arena, + bun_alloc::ArenaVec::::new_in(arena), + &mut dest, + &css::PrinterOptions::default(), + None, + None, + &symbols, + ); + + if let Err(err) = result.to_css(&mut printer) { + return Err( + global.throw(format_args!("color() internal error: {}", err.name())) + ); + } + drop(printer); - return bun_jsc::bun_string_jsc::create_utf8_for_js(global, &dest); + bun_jsc::bun_string_jsc::create_utf8_for_js(global, &dest) + }); } } } diff --git a/test/js/bun/css/color.test.ts b/test/js/bun/css/color.test.ts index bafcf5863505..8e2741d1c055 100644 --- a/test/js/bun/css/color.test.ts +++ b/test/js/bun/css/color.test.ts @@ -536,3 +536,41 @@ describe("input forms", () => { expect(color("#f00", "[rgb]")).toEqual([255, 0, 0]); }); }); + +// The string parser reuses one per-thread scratch arena across calls instead +// of creating a fresh mimalloc heap each time. Most color literals allocate +// nothing into it, but a CSS escape sequence (`\72\65\64` == "red") or a +// function token (`url(...)`) forces the tokenizer to allocate copy-on-write +// buffers there. This interleaves those arena-allocating inputs with cheap ones +// and invalid ones in a tight loop: if the arena reset/reuse leaked state +// between calls, an escaped-ident or function-token parse would corrupt a later +// call's result. Every call must return exactly what it returns in isolation. +test("reused parse arena stays correct across interleaved calls", () => { + // [input, expected `css` output] — covers escapes (arena copy-on-write), + // plain literals (no arena alloc), and invalid/function tokens. + const cases: Array<[string, string | null]> = [ + ["\\72\\65\\64", "red"], // "red" via hex escapes -> arena copy-on-write + ["#f00", "red"], // plain literal -> no arena allocation + ["re\\64", "red"], // "red" with a single escaped 'd' + ["rgb(0, 0, 255)", "#00f"], // plain literal + ["\\62\\6c\\75\\65", "#00f"], // "blue" via escapes -> arena copy-on-write + ["hsl(0, 100%, 50%)", "red"], // plain literal + ["url(#bad)", null], // function token -> arena, invalid -> null + ["bad color input", null], // invalid + ]; + + // Sanity: each result in isolation matches the expectation. + for (const [input, expected] of cases) { + expect(color(input, "css")).toBe(expected); + } + + withoutAggressiveGC(() => { + for (let i = 0; i < 10_000; i++) { + for (const [input, expected] of cases) { + if (color(input, "css") !== expected) { + throw new Error(`color(${JSON.stringify(input)}, "css") !== ${JSON.stringify(expected)} on iteration ${i}`); + } + } + } + }); +});