Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 10 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
@robobun adopt |
|
Rebased onto main. #39770 landed the parse_nested_block half, so this PR is now the skip_until_delimiter extraction (-9,110 B of .text on current main) plus 16 css.test.ts cases that pin down the moved loop. It is a code motion change, so those cases pass on main as well, and the only red Buildkite test so far is the unrelated test/bake/deinitialization.test.ts Windows crash that is already being tracked. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical extraction of monomorphized bookkeeping into non-generic #[inline(never)] helpers.
What was reviewed:
- Traced the original
parse_nested_blockbody statement-by-statement againstenter_nested_block+parse_entirely+exit_nested_block; ordering ofnesting_depthinc/dec,stop_beforesave/restore,at_start_of.take(), and both early-return error paths are preserved. - Verified the
skip_until_delimiterloop body is byte-identical to what was inlined inparse_until_before, anddelimiters(the merged set, notdelimiters_) is what gets passed. - Checked that
enter_nested_block'sErrreturns via?match the original early returns (same error values, same parser state on exit).
Extended reasoning...
Overview
This PR extracts non-generic bookkeeping out of two generic CSS parser functions in src/css/css_parser.rs to reduce binary size from monomorphization bloat. parse_nested_block<T> (192 instantiations) is split into enter_nested_block → closure → exit_nested_block, and the trailing skip loop in parse_until_before<T, C> is pulled into skip_until_delimiter. All three helpers are #[inline(never)] so they collapse to one copy each. A small NestedBlock Copy struct carries block_type, start_position, and saved_stop_before between enter and exit.
Security risks
None. This is internal control-flow reorganization inside the CSS parser with no change to input handling, bounds, allocation, or error surfacing.
Level of scrutiny
Low-to-moderate: the change is a pure refactor with a stated size win, but parse_nested_block sits on the hot path and has subtle state (nesting depth counter, stop_before restore, unclosed-block-at-EOF recording, at_start_of clearing). I diffed the old body against the new enter/exit split line-by-line: every mutation happens in the same order relative to the closure call, both early-error paths (unclosed_block_at_eof shortcut and MAX_NESTING_DEPTH) return before Ok(NestedBlock) and propagate identically via ?, and result.is_err() is threaded through as result_is_err: bool so exit_nested_block stays non-generic over T.
Other factors
The skip_until_delimiter extraction correctly receives the merged delimiters (= parser.stop_before | delimiters_), not the raw argument, matching the original loop. The FIXME comment moved with the loop it describes. No new public surface, no test changes needed since behavior is identical, and the PR description reports the CSS suite passes on the debug build.
parse_until_before<T, C> ends with a loop that skips to the next delimiter. The loop does not depend on T or C, but it was compiled into every instantiation, and into every caller that inlines one. The loop is now skip_until_delimiter, one #[inline(never)] function. On a linux-x64 release build of current main this removes 9,110 B of .text (74,110,128 B to 74,093,744 B of stripped binary). The enter/exit split of parse_nested_block that this branch also carried landed on main in #39770, so that part is dropped here.
c0a97ea to
28ed5c2
Compare
|
Updated 7:31 PM PT - Aug 20th, 2026
✅ @robobun, your commit 36e2bac956f88b56b7703ddcd08ce55a5c740936 passed in 🧪 To try this PR locally: bunx bun-pr 39520That installs a local version of the PR into your bun-39520 --bun |
robobun
changed the title
There was a problem hiding this comment.
LGTM — mechanical extraction of the skip loop into an #[inline(never)] helper, same pattern #39770 already landed for parse_nested_block.
What was reviewed:
- The moved loop body is byte-identical;
delimiters(the merged bitset) and&mut Parserare the only captures, both passed explicitly. parser.stop_beforeis restored before the call andat_start_ofis already consumed above, so the helper observes the same state the inline loop did.DelimitersisCopy, so pass-by-value is fine; the FIXME comment moved with the code it describes.
Extended reasoning...
Overview
This PR moves the trailing "skip tokens until we hit a delimiter" loop out of the generic parse_until_before<T, C> body in src/css/css_parser.rs into a monomorphic #[inline(never)] fn skip_until_delimiter(&mut Parser, Delimiters). The loop body is unchanged; only its location and the #[inline(never)] attribute are new. This is the remaining piece of the original PR after #39770 landed the enter_nested_block/exit_nested_block split — the rebase left just this one extraction.
Security risks
None. This is CSS tokenizer bookkeeping with no I/O, no allocation, and no user-controlled indexing introduced. The loop's bounds behavior (break on delimiter byte or tokenizer EOF, skip nested blocks via consume_until_end_of_block) is unchanged.
Level of scrutiny
Low. This is a pure code-motion refactor for binary size, following an already-merged sibling change in the same file. The only things that could go wrong are (a) the extracted function seeing different state than the inline loop did, or (b) a capture being dropped. I checked both: saved_stop_before is restored to parser.stop_before before the call (same ordering as before), at_start_of is taken earlier in the block, and the two values the loop reads — delimiters and parser — are passed as explicit arguments. Delimiters is a small Copy bitflags type, so pass-by-value matches the previous local read.
Other factors
The PR description includes measured .text deltas on release builds and confirms the CSS test suites pass on debug. No outstanding reviewer comments. The FIXME comment moved with the loop rather than being deleted, which is correct.
The skip loop at the end of parse_until_before now lives in skip_until_delimiter. These cases pin down what it does: nested blocks are skipped as one unit, the inherited closing delimiter stops the skip, and end of input ends it. They go through the two paths where the skip is visible in the output: the @charset rule and forgiving selector lists.
There was a problem hiding this comment.
LGTM — pure code motion of the delimiter-skip loop out of the generic body, matching the pattern #39770 already landed for parse_nested_block.
What was reviewed:
- Verified the loop body in
skip_until_delimiteris byte-identical to what was inlined; theStoperror path still early-returns before the skip, andstop_beforeis restored before the call — same ordering as before. - Checked that
delimiters(the union withsaved_stop_before) is what's passed through, notdelimiters_, so the skip terminates on the same set. - New
minify_testcases exercise the skip via@charsetand forgiving selector lists, covering nested blocks, EOF, and inherited-delimiter termination.
Extended reasoning...
Overview
This PR extracts the trailing skip-to-delimiter loop from parse_until_before<T, C> in src/css/css_parser.rs into a standalone non-generic #[inline(never)] function, skip_until_delimiter. The loop does not reference T or C, so hoisting it out of the generic body avoids duplicating it into every monomorphized instantiation. The PR also adds a describe("skipping to a delimiter") block to test/js/bun/css/css.test.ts covering @charset prelude skipping and forgiving-selector-list recovery, both of which exercise this exact loop.
Security risks
None. This is a mechanical refactor of CSS-parser control flow with no change to input handling, no new allocations, and no FFI surface.
Level of scrutiny
Low. The diff is a textbook extract-function: the loop body moved verbatim, the pre-loop state restoration (parser.stop_before = saved_stop_before) still runs before the skip, and the ParseUntilErrorBehavior::Stop early-return still bypasses the skip entirely. The extracted function receives &mut Parser and the same delimiters union that the inlined loop used. This is the same transformation #39770 applied to parse_nested_block (now nested_block_enter/nested_block_exit), and the PR was rebased to drop the overlapping hunk and keep only this remainder.
Other factors
The added tests use the existing minify_test helper in the surrounding describe("general") block and cover the observable edges of the skip: semicolons/commas inside nested ()/[]/{} blocks not terminating the skip, EOF with and without an unclosed block, and the inherited close-paren delimiter for the last-selector case. The PR body documents the measured 9,110 B .text reduction and confirms the CSS suites pass on the rebased branch (the noted timeouts reproduce on a debug build of main under the same load). No outstanding reviewer comments and no prior review from me.
…over The comment that moved with the loop was a note inherited from rust-cssparser. Replace it with a description of what the function does.
There was a problem hiding this comment.
LGTM — pure extraction of the non-generic skip loop into an #[inline(never)] helper, same pattern as #39770.
What was reviewed:
- Verified the loop body in
skip_until_delimiteris byte-identical to what was inline; the mergeddelimiters(notdelimiters_) is passed, and theStoperror path still returns before the skip. - Checked that
stop_beforeis restored andat_start_ofis consumed before the helper call, so the extracted loop sees the same parser state. - The 16 new tests exercise both visible skip paths (
@charsetbody, forgiving selector lists) and the author confirmed 12/16 fail with the nested-block branch removed.
Extended reasoning...
Overview
This PR extracts the trailing skip loop from parse_until_before<T, C> in src/css/css_parser.rs into a standalone #[inline(never)] fn skip_until_delimiter. The loop does not use the generic parameters T or C, so every monomorphized instantiation was carrying a redundant copy. This is the second half of the work started in #39770 (which did the same for parse_nested_block and already landed on main). The PR also adds 16 minify_test cases in test/js/bun/css/css.test.ts that pin the skip behavior through @charset and :is()/:where() forgiving selector lists.
Security risks
None. This is a code-motion refactor in the CSS tokenizer's error-recovery path. No new inputs are accepted, no bounds are changed, no allocations are introduced.
Level of scrutiny
Low. The diff is a mechanical extraction: the loop body is character-for-character identical, the argument passed is the same merged delimiters local (line 447: parser.stop_before | delimiters_), and the call site is placed at exactly the same point in control flow — after stop_before is restored and at_start_of has been taken. The ParseUntilErrorBehavior::Stop early return still happens before the skip. The old FIXME comment was replaced with an accurate doc comment (the third commit in this PR). This follows an established pattern that a maintainer already merged for the sibling function.
Other factors
The PR description is unusually thorough: it reports measured .text savings (-9,110 B), demonstrates the tests are load-bearing via mutation testing (12/16 fail with the consume_until_end_of_block branch removed; the whole suite fails if the unmerged delimiters_ is passed instead), and explains why these specific test paths were chosen (bun's CSS parser runs with error_recovery off, so @charset and forgiving selector lists are the two places where the skip is externally observable). The tests use the existing minify_test helper and are placed in the existing css.test.ts file per repo conventions. No outstanding reviewer comments; alii adopted the PR via robobun.
This branch has not been deployed
2 participants
Problem
parse_until_before<T, C>(src/css/css_parser.rs:440) ends with a loop that skips input up to the next delimiter. The loop does not useTorC, but every instantiation and every caller that inlines one gets a copy.parse_nested_blockhad the same problem. Trim ~2 MB from the release binary without touching hot paths #39770 fixed that part on main, so this PR now only carries theparse_until_beforepart.Fix
skip_until_delimiter, one#[inline(never)]function. The body moved as is, and theStoperror path still returns before it..textshrinks by 9,110 B, all of it inbun_css. The strippedbungoes from 74,110,128 B to 74,093,744 B.skipping to a delimiterblock intest/js/bun/css/css.test.ts(16 cases) pins down the moved loop through@charsetand forgiving selector lists. This is a refactor, so they pass on main too. With the nested block branch of the loop removed, 12 of them fail (see Notes).bun bd test test/js/bun/css/ test/bundler/css/. The only failures are 5 s timeouts that a debug build of main hits too on the loaded build box (see Notes).Background
parse_until_beforeruns a closure on the input up to a delimiter such as{, then skips what the closure left unread. Every declaration and prelude goes through it.#[inline(never)]function keeps one copy.-icf=safe, but ICF cannot fold these copies: each one is built around a different closure call.Notes
Conflict resolution on rebase: #39770 (main, Aug 20) added
nested_block_enter,nested_block_exitandNestedBlockState, which is the same split this PR made under the namesenter_nested_block,exit_nested_blockandNestedBlock. I kept main's version and dropped this PR's copy. The remaining diff is theskip_until_delimiterhunk from the original commit, unchanged.Measurement of the full original diff against main as of Aug 18 (before #39770), for the record:
.text52,938,146 B to 52,835,422 B (-102,724 B), strippedbun76,846,128 B to 76,715,056 B. Before: 164 out-of-line copies ofparse_nested_block(102,109 B) plus 214 copies ofparse_entirely<closure>for those blocks (185,074 B). After: 110 plus 20 copies (126,639 B together) plus one copy each of the three helpers. Most of that win is now on main through #39770.Measurement of the rebased remainder (this PR as it stands): the one out-of-line
parse_until_beforegoes from 350 B to 214 B,skip_until_delimiteris one 154 B function, and theparse_nested_blockcopies that inlineparse_until_beforeshrink from 112,111 B to 109,860 B. Total.textdelta -9,110 B, bun_css-attributed delta -9,110 B.Why these tests: bun parses CSS with
error_recoveryoff, so an invalid declaration or rule is a fatal error and the skip after it is not visible in the output. Two paths do show it.@charsetis consumed withparse_until_after(SEMICOLON | CLOSE_CURLY_BRACKET, parse_empty)(css_parser.rs,RuleListParser), so the skip loop is what consumes the rule. In:is()and:where(), an invalid selector is dropped and the skip loop advances to the next top-level comma or to the)inherited throughstop_before(selectors/parser.rs,parse_list_with_state). The 16 cases pass with both a main build and a branch build of the debug binary. Two mutations of the branch as a check on the tests: with theconsume_until_end_of_blockbranch removed fromskip_until_delimiter, 12 of the 16 fail (the other 4 are the plain delimiter and end of input cases). Withparse_until_beforepassing the unmergeddelimiters_instead of the merged set,css.test.tsalready fails while the file loads, because the last declaration of every block then runs past its}.Test run on the rebased branch:
test/js/bun/css/andtest/bundler/css/pass except 5 s timeouts incss-fuzz.test.ts(local-only, it runs 1000Bun.buildcalls per test and times out on a debug build of main too),nested-selector-expansion.test.ts,selector-list-error-recovery.test.tsandnested-vendor-prefix-duplication.test.ts. The build box had a load average of 70 to 80 on 16 cores. I ran those three files alternately with a debug binary built from main and one built from this branch: both binaries hit the same timeouts, and per-test times were within the run-to-run noise (for example 2.1 s vs 2.0 s, 5.8 s vs 7.1 s vs 4.9 s for the same test across runs). The same files passed here on Aug 18 on a quiet box.Observation, not changed here:
consume_until_end_of_blockis#[cold]and allocates aVecper call, but every nested block exit calls it. That is pre-existing and is being handled separately.