Repository navigation
Conversation
When a style rule's selectors are not all compatible with the targets and cannot be wrapped in :is(), minify_style_arm moves the incompatible ones into one rule each. It removed them from the selector list one at a time with ordered_remove(i) and then drained the two follow-up lists with ordered_remove(0), each of which shifts the rest of the list, so a rule with n incompatible selectors was O(n^2). Every selector of a nested rule is incompatible when nesting has to be compiled away, which the default bun build targets require: a 120 KB nested rule with 16k selectors took 6 s. Partition the list in one pass and consume the follow-up lists by value. The output is unchanged.
|
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 CSS minifier now partitions selector lists and emits incompatible rules through direct iteration. Regression tests cover source order, nested selector expansion, and performance for large nested selector lists. ChangesCSS selector iteration
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The selector-splitting optimization preserves output behavior and is covered by regressions for ordering, nesting, and performance. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Reproduced on 1.4.2 and on a debug build of main with the ledger script: The regression test ( CI: 180 of 181 jobs pass and the new test passes on every lane. The one red job (debian 13 x64-asan) fails only in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a clean linear-time rewrite of the three ordered_remove drain loops with order preserved.
What was reviewed:
- Verified the
mem::takepartition and both consumingfor … in listloops preserve iteration order, so output is byte-identical;SmallListwrapsSmallVecand hasDefault+ owningIntoIterator, so no drop/ownership hazards. - Checked for other
ordered_remove(0)/ordered_remove(i)drain sites insrc/css/— none remain in this pass. - The new test mirrors the sibling
*-hang.test.tsfiles (same targets encoding, same 90 s outlier timeout) and asserts a self-relative ratio rather than a wall-clock threshold;minifyTestis registered with arity 3 so the targets argument is honored.
Extended reasoning...
Overview
The PR touches one function, minify_style_arm in src/css/rules/mod.rs, replacing an index-based ordered_remove(i) partition and two while len > 0 { ordered_remove(0) } drain loops with core::mem::take + re-append and consuming for … in list iteration. It adds test/js/bun/css/nested-selector-list-split-hang.test.ts, a ratio-based perf guard driving cssInternals.minifyTest from bun:internal-for-testing.
Security risks
None. This is safe Rust over an internal SmallList (a smallvec::SmallVec newtype in src/collections/lib.rs) with Default and owning IntoIterator impls, so mem::take leaves a valid empty list and by-value iteration drops correctly. No untrusted-input parsing, FFI, or auth/crypto paths are involved.
Level of scrutiny
Low-to-moderate. The change is ~30 lines, mechanical, and order-preserving — the compatible selectors are appended back in encounter order, and both follow-up lists are consumed front-to-back exactly as the old ordered_remove(0) loops did, so emitted rule order is unchanged. This is precisely the "no orderedRemove(0) draining of unbounded queues" pattern REVIEW.md flags; a repo-wide grep of src/css/ shows no other ordered_remove(0)/ordered_remove(i) drain sites in this minify pass.
Other factors
The new test file follows the established local convention alongside duplicate-declaration-merge-hang.test.ts, atan2-backtracking-hang.test.ts, and angle-serialization-hang.test.ts in the same directory — same { chrome: 80 << 16 } target encoding, same 90 s per-test timeout for a perf-guard outlier, and bun:internal-for-testing rather than adding production hooks. It asserts output correctness first (toStartWith/toEndWith on the split rules) and then a machine-speed-independent best-of ratio (nested vs. same-size flat baseline, < 8×), which is the sanctioned form for complexity guards and avoids absolute wall-clock flakiness under debug/ASAN. The bug-hunt exit reason was dry_streak and no findings or prior objections exist on the PR.
Take the asserted output from the timed runs, sample once when a run is slow (debug builds), build the 50k selector list with a native join, and drop the per-test timeout. The test now takes under 3 s on a debug build. Also cover the order of a mixed compatible/incompatible list.
|
Updated 6:43 PM PT - Sep 10th, 2026
❌ @robobun, your commit 2fa92b5 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 42243That installs a local version of the PR into your bun-42243 --bun |
There was a problem hiding this comment.
LGTM — the follow-up commit tightened the test to fit the default timeout and added the source-order correctness case.
What was reviewed:
- The three
ordered_removeloops inminify_style_armare replaced with owning iteration; order is preserved in each (take-and-reappend for the partition, in-orderforfor the two drains), so output is byte-identical. core::mem::takeonsty.selectors.vleaves a valid emptySmallList(Default+IntoIteratorare implemented insrc/collections/lib.rs), andincompatiblelosingmutis correct since it's now consumed by value.- The perf test is self-relative (nested vs. flat on the same machine) with an 8× bound against a 2-4× fixed / 19×+ unfixed gap, and the new file matches the existing
*-hang.test.tspattern intest/js/bun/css/.
Extended reasoning...
Overview
This PR fixes an accidentally-quadratic loop in the CSS minifier's minify_style_arm (src/css/rules/mod.rs). Three sites drained a SmallList via ordered_remove(0) / ordered_remove(i), shifting the tail on every removal. The fix replaces them with owning iteration: the selector partition now core::mem::takes the list and re-appends each selector into either the kept list or incompatible, and the two follow-up drain loops become for … in over the owned lists. Net Rust change is ~15 lines. A new test file adds a source-order correctness check for both partition halves (top-level and nested) plus a self-relative perf ratio test (50k-selector nested rule vs. the same list flat).
Security risks
None. This is a pure algorithmic-complexity change to an internal CSS minifier code path. No parsing of new input shapes, no new external boundaries, no unsafe code, no allocation-size arithmetic on untrusted data beyond what already existed.
Level of scrutiny
Low-to-moderate. The transformation is mechanical and each of the three replacements is locally verifiable as order-preserving: the partition iterates in source order and appends to two lists; the two drain loops previously popped index 0 in a while, which is exactly front-to-back iteration. SmallList implements both Default and IntoIterator (src/collections/lib.rs), so core::mem::take yields a valid empty list to re-append into and the by-value for is well-defined. REVIEW.md explicitly names orderedRemove(0) draining as a pattern to eliminate, so this is the canonical fix.
Other factors
A prior review under this app's identity left comments; the author pushed a follow-up commit that (a) dropped the 90 s per-test timeout override by shrinking the workload (60k→50k selectors, 2 s→400 ms bench budget, faster string construction), and (b) added an explicit correctness test asserting exact minified output for both compatible and incompatible halves in source order. The new test file follows the established *-hang.test.ts convention already used in test/js/bun/css/ (e.g. duplicate-declaration-merge-hang.test.ts, angle-serialization-hang.test.ts), so a standalone file is consistent with local practice. No CODEOWNERS cover src/css/. No outstanding CHANGES_REQUESTED reviews. Bug-hunt exit reason was dry_streak with zero findings.
This branch has not been deployed
2 participants
Problem
.a,.b{ .c0,…,.c16383 {color:red} }) takes 6 s inBun.build. The same list at the top level takes 38 ms for 65k selectors (60% of samples inmemmovecalled fromminify_style_arm).:is(),minify_style_arm(src/css/rules/mod.rs) moved the incompatible ones out withordered_remove(i)(line 698) and drained two follow-up lists withordered_remove(0)(lines 785, 882). Each call shifts the rest of the list.Fix
incompatibleandincompatible_rulesby value.test/js/bun/css/nested-selector-list-split-hang.test.ts(time ratio of a 50k-selector nested rule against the same list at the top level: 2x to 3x with the fix, 21x on debug main, 2600x on 1.4.2, plus an order check for a mixed list). Alsotest/js/bun/css/css.test.ts, the othernested-*tests,test/bundler/esbuild/css.test.ts.Background
:is(), or, when:is()is not available, splits the unsupported selectors into one rule each.&. For targets without native nesting every one of them counts as unsupported, so the split runs over the whole list.bun buildtargets predate both nesting and:is().Notes
Bun.build, nested / flat): n = 2048 / 4096 / 8192 / 16384 was 62 / 331 / 965 / 4104 ms nested before, 5 / 9 / 10 / 20 ms after; flat is 2 to 10 ms either way.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file