Repository navigation
perf(css): recycle nested block contents during the walk - #21628
Conversation
|
|
This PR is packaged and the instant preview is available (ce12f4e). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@ce12f4e
yarn add -D webpack@https://pkg.pr.new/webpack@ce12f4e
pnpm add -D webpack@https://pkg.pr.new/webpack@ce12f4e |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21628 +/- ##
==========================================
+ Coverage 94.39% 94.41% +0.01%
==========================================
Files 628 628
Lines 80273 80611 +338
Branches 23732 23809 +77
==========================================
+ Hits 75777 76107 +330
- Misses 4496 4504 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "many-modules-esm", scenario '{"name":"mode-development","mode":"development"}' |
821.2 KB | 2,304.4 KB | -64.36% |
| ⚡ | Memory | unit benchmark "css-parser-tailwind-unit", process (expanded, no visitors) |
4.7 MB | 1.2 MB | ×3.9 |
| ⚡ | Memory | benchmark "asset-modules-inline", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
397.6 KB | 272.7 KB | +45.82% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/css-stream-nested-blocks (aef7cf8) with main (6412396)
bdc662b to
bf19f97
Compare
Generated code sizeComparing
1 asset(s) changed size
No runtime gained or lost a runtime module. Built |
The walk already streamed top-level nodes, but everything inside one block
was held until that whole subtree finished, so a stylesheet wrapped in a
single `@media` kept its entire AST. Enter a rule as soon as its prelude is
sealed at `{`, walk and recycle each child as it completes, and exit at `}`
where the rule's end is final — peak storage becomes the open path.
Off by default (`streamBlocks`) and walk-only. A streamed rule's children
reach the visitors instead of its body, so `A.declarations` / `A.childRules`
read empty for it; `CssParser` reads both and cannot opt in until those
sites accumulate during the walk instead.
Peak column storage over 20k rules: `@media`-wrapped 8.05 MiB -> 0, the same
for `@supports` and nested at-rules, CSS nesting 4.39 MiB -> 0. Shapes with
nothing to reclaim pay for the bookkeeping (flat +9%, many small blocks
+17%) — the lazy activation threshold that fixes it is not implemented yet.
Differential against the default walk over 20 shapes: no ordering or count
differences, and the whole delta is the provisional `end` a rule reports at
`enter` before its block closes.
`_streamConsumeBlock` recurses through the block consumer, so its own locals already are the stack — the four parallel arrays it pushed and popped per rule were eight array operations, with their bounds and capacity checks, on every rule. It cost 5.2% self time in a CPU profile of a flat stylesheet, where streaming reclaims nothing and the work is pure overhead. Save the frame into locals across the recursive call instead. Interleaved A/B, min of 35 samples per side, 20k rules: flat +9.3% -> +1.8%, many small blocks +17.2% -> +10.6%, one big `@media` -23.9% -> -2.7%, `@supports` +3.0% -> -1.7%, nested at-rules -5.2% -> -3.3%, CSS nesting +0.8% -> -0.6%. Memory is unchanged: every shape that held a subtree still recycles to zero. The remaining cost on small blocks is the streaming machinery running where there is nothing to reclaim, which the lazy activation threshold addresses.
The streamed walk ran on every block, so a small one paid the machinery for
nothing: +9.3% on a flat stylesheet, +17.2% on many small `@media`.
Enter a rule and start recycling only once its block passes
`_STREAM_MIN_NODES`. `_nodeCount` restarts at each top-level node, so
`_nodeCount - mark` is exactly how far the current block has grown; under
the threshold the sink collects like the non-streaming path and the rule is
handed back materialized for the ordinary walk, keeping `A.declarations` /
`A.childRules` readable on it.
Activation has to enter every still-buffering ancestor first, outermost in,
or a nested block enters before its parent — `@media{@supports{...}}` fired
the inner rule's `enter` first. Ancestors are unreachable from locals, so
the per-block frame moved into depth-indexed tables: nesting is shallow and
strictly LIFO, so an indexed store avoids the bounds and capacity checks
that push/pop cost on every rule.
Memory is now released on every shape that used to hold a subtree: one big
`@media`, `@supports`, nested at-rules and CSS nesting all go from
8.05 / 4.39 MiB to 0. CPU is +1.8% to +7.7% — the sink still costs an
indirect call per child before activation, which the next commit removes by
checking the threshold inside the block consumer instead.
A block wrote its frame and pushed every child through the sink from the moment it opened, so a stylesheet with no big block paid for machinery it never used. Keep the block's state in `consumeABlocksContentsInto`'s locals and write the frame only where an ancestor could still be reached for -- just before a child rule, and on the block's own growth past the threshold. A block of nothing but declarations, which is every leaf rule, now touches no frame at all, and the growth check is one compare in the collect loop rather than a call and a type test per child. Two fixes fall out of entering a rule before its block is consumed: - Sibling indices were wrong on every streamed block: the parent's counter was bumped when a child block opened and then reused as the walk counter, so the first child of a large `@media` reported index 456. Each list now counts separately, as `_walkRule` does, and the index a child carries is the one the collected walk gives it. - `A.declarations` / `A.childRules` returned `null` on a streamed rule -- indistinguishable from an at-rule with no block, which is what `CssParser` reads to decide. The body slot is claimed, empty, before `enter` fires; claiming it up front also lets the slots be handed back per child, which is the last thing a streamed block still grew. Threshold raised to 16384 nodes: a block that crosses it just before its own `}` activates for almost nothing back. Measured against main, 20k rules per shape, each arm in its own process, median of 13: @media -9.6%, @supports -8.0%, @media>@supports -11.4%, CSS nesting -7.5%, flat +0.2%, many small @media +1.0%. Retained heap while the block is open, forced GC: 31.2 MiB of node columns and 3.9 MiB of JS heap down to 0 and 0.16 MiB. The default path is unchanged.
`streamBlocks` was walk-only, because the printer builds a parent's text
out of its children's -- exactly what a streamed body has already released.
So printing held the whole subtree: 60 MiB live while a 20k-rule `@media`
was open, for 3.2 MiB of output.
A streamed rule now prints as it goes: its `prelude{` is held back when the
block opens, each finished child is emitted straight after it, and the `}`
closes it. Held rather than emitted, because an opener that turns out to
have nothing inside it still has to be droppable at its `}` -- a whole
stack of them, since the block that empties may be several levels down.
Two transforms genuinely need a whole block at once, and neither is given
up:
- A longhand family only merges in a block with no child rule, so a block
of declarations alone is the one that declines to stream.
- Only the last of a set of identical declarations can be read. There is no
looking ahead for the later one, so the earlier one is taken back out of
the output instead: `PrintContext` keeps the output as the pieces it was
emitted in, and a piece can be retracted after later ones are out.
Positions follow from that -- a piece's place in the output is no longer
known when it is emitted, so mappings anchor to a piece and are resolved
against the pieces once, when a map is asked for. Printing without one now
never scans the text for newlines at all.
Output is byte for byte the collected printer's, source map included, over
23 shapes covering empty blocks, duplicate declarations across a child
rule, kept comments, raw hacks, custom properties and `@keyframes`.
Minifying 20k rules, each arm in its own process, median of 11: `@media`
-36.8%, `@media>@supports` -38.1%, CSS nesting -36.9%, flat +0.4%, many
small `@media` +2.0%. Retained heap while the block is open: 60.4 -> 7.7
MiB on `@media`, 16.3 -> 3.8 MiB on nesting, against 3.2 and 0.7 MiB of
output. The collected path is unchanged, on both CSS and HTML.
…here needed
Two regressions on stylesheets that never stream, both from how the output
is now held.
Every emit became a piece of its own, so a flat stylesheet -- the common
one -- built an array of 20k strings where it used to append to one. Output
accumulates into a plain string again, and a piece is cut off only for text
that may still be taken back, which is the block's own declarations and
nothing else. Mappings carry an offset within their piece, so an anchor can
sit mid-string.
The other was subtler and was costing 3.8 MiB. A printer returns its text
as a rope of its children's pieces, and the old `_emit` happened to flatten
each one while scanning it for newlines -- a scan this branch had removed as
useless, since positions are now worked out only if a map is asked for.
Without it, ropes append onto ropes and every fragment of every node stays
reachable until the output is finally flattened: a second copy of the whole
output. Flattening each piece as it lands fixes the memory and costs 40% CPU,
an allocation per piece. So the tail is cut off and forced flat once per 64
KiB instead, which the result would have copied anyway.
Measured against main, each arm in its own process, arms alternated:
minify, retained heap while the block is open, 20k rules
@media 60.4 -> 3.4 MiB flat 5.9 -> 5.0 MiB
nesting 16.3 -> 1.2 MiB small @media 3.9 -> 3.9 MiB
minify CPU, 10k rules, median of 9
@media -35.3% @supports -31.9% nesting -29.2%
flat +2.2% small @media +3.3% (both inside the +/-5% this box resolves)
Output and source map stay byte for byte main's, streamed or not.
`streamBlocks` was an opt-in because streaming used to be walk-only, and a caller that printed had to decline it. Printing streams too now, so there is nothing left for the option to trade off: a block under the threshold is still collected and walked in one batch exactly as it always was, and one past it streams. So the walk always streams and the option is gone. It stays a flag internally, because the standalone `parseA*` entry points hand back a materialized tree and must not stream; `_setupParse` leaves it off for them and only `grammar` turns it on. What a streamed rule reports about its own block is now the walk's ordinary behaviour rather than something a caller asked for, so `A.declarations` and `A.childRules` say so where they are defined: they read as an empty block on a streamed rule, and `null` still means no block at all. The tests can no longer print a stylesheet both ways to compare, so they assert the output itself, over fixtures already in minimal form. Every case pins whether it is exercising the streamed or the collected path, so none of them can quietly stop testing streaming if the threshold moves.
Generated positions are worked out when a map is asked for rather than as the output is built, by walking the output alongside the mappings. But the walk asked `indexOf` for the next newline once per mapping, and `indexOf` has no end bound: with no newline ahead it scans to the end of the piece and returns -1, every time. Minified output has no newlines at all, so every one of a piece's mappings scanned the whole piece -- quadratic in the mappings a piece holds, which is one per top-level rule. Carry the next newline's offset across mappings instead, and look for another only after one is passed. A piece with no newlines is scanned once. Minifying 10k rules, each arm in its own process, median of 9: flat +2.7% -> -2.9%, many small `@media` +3.6% -> -5.8%.
cf02cea to
1b8718b
Compare
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Note
|
| Layer / File(s) | Summary |
|---|---|
Chunked printer output lib/util/SourceProcessor.js |
PrintContext stores output in chunks, supports pending and retractable pieces, and resolves source-map anchors from chunk positions. |
Nested block streaming lib/css/syntax.js |
Large nested blocks publish frames, walk completed children incrementally, preserve indexes, and reset streaming state during cleanup. |
Rule serialization and accessors lib/css/syntax.js |
Rule prelude serialization is separated from body processing. Streamed rule accessors document empty materialized bodies. |
Streaming behavior validation test/CssSyntax.unittest.js |
Tests cover thresholds, traversal, nested blocks, output, declaration handling, comments, and processor cleanup. |
Bun worker lifecycle
| Layer / File(s) | Summary |
|---|---|
Bun restart and shutdown handling .github/workflows/test.yml, package.json |
Workflow comments distinguish restart exits from normal shutdown. The Bun Jest command raises the worker idle memory limit to 2048 MB. |
Possibly related PRs
- webpack/webpack#21646: Both changes modify Bun worker memory configuration and worker termination handling.
Suggested labels: area: build-perf
Suggested reviewers: bjohansebas
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title uses valid Conventional Commit syntax and accurately describes the main performance change, but the branch prefix is not provided for verification. | Provide the branch name or prefix, or confirm that the branch prefix is perf. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/patches/jest-worker+30.4.1.patch (1)
32-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not suppress Bun preload failures.
test/bun-preload.jsprovides required Jest compatibility in Bun worker threads. If it fails to load, this emptycatchstarts the worker without that setup. Let the worker fail at bootstrap so the CI log shows the actual cause.Proposed fix
if (process.versions.bun) { - try { - require(require("path").resolve(process.cwd(), "test/bun-preload.js")); - } catch (e) {} + require(require("path").resolve(process.cwd(), "test/bun-preload.js")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/patches/jest-worker`+30.4.1.patch around lines 32 - 35, Remove the empty catch around the Bun preload require so failures from loading test/bun-preload.js propagate during worker bootstrap. Keep the process.versions.bun guard and existing preload resolution unchanged.
🧹 Nitpick comments (3)
test/CssSyntax.unittest.js (2)
1024-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
_STREAM_MAX_DEPTHfallback.
_streamConsumeBlockinlib/css/syntax.js(Lines 4402-4413) falls back to the materializing path whend >= _STREAM_MAX_DEPTH. No case in this describe block nests 64 rules deep, so that branch is untested. The fallback sets_setBody,_setBlock, and_setEndon its own and never publishes a frame, so a regression there would not surface in the current suite.A case that nests 70 qualified rules and places
BIGat the innermost level exercises both the fallback and the streamed levels above it.As per coding guidelines for
test/**/*.{js,json}: "Cover every changed line and branch with tests, preferably through integration cases; do not lower patch coverage below the project target."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/CssSyntax.unittest.js` around lines 1024 - 1046, Add a test case in the existing state-cleanup describe block that builds roughly 70 nested qualified rules with BIG at the innermost level, then processes and validates the result through the normal walk/minify assertions. Ensure the nesting reaches the _STREAM_MAX_DEPTH fallback in _streamConsumeBlock while preserving coverage for state cleanup after processing.Source: Coding guidelines
761-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the streamed path for the
BIGcases that do not assert it.The comment at Lines 762-766 states that every case pins which path it exercises. Four cases do not: Lines 839-846, 848-870, 898-905, and 955-964 use
BIGbut never assert that the block streamed._STREAM_MIN_NODESis an internal constant. If it rises, these cases fall back to the collected path and still pass, so they stop testing streaming without failing.Cases at Lines 971, 988, and 1014 already pin the path with
childCount. Apply the same guard to theBIGcases, or assert once thatBIGstreams.💚 Proposed guard
const BIG = repeat(1800, rule); const SMALL = repeat(4, rule); + + it("BIG crosses the streaming threshold and SMALL does not", () => { + // Guards every case below that uses these two fixtures. + expect(childCount(`@media screen{${BIG}}`)).toBe(0); + expect(childCount(`@media screen{${SMALL}}`)).toBe(4); + });As per coding guidelines for
test/**/*.{js,json}: "Cover every changed line and branch with tests, preferably through integration cases; do not lower patch coverage below the project target."Also applies to: 839-846
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/CssSyntax.unittest.js` around lines 761 - 776, Update the BIG-based test cases around describe("CssSyntax — block streaming") that currently lack a streaming assertion, including the cases near Lines 839, 848, 898, and 955. Add the same childCount-based guard used by the existing pinned cases, or add one shared assertion that verifies BIG takes the streamed path, so these tests fail if the threshold changes.Source: Coding guidelines
lib/util/SourceProcessor.js (1)
319-339: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSkip the push when
_tailis empty.
emitRetractablecalls_cutTailon every call. In a streamed block each declaration goes throughemitRetractable, so the tail is empty for every call after the first one._cutTailthen pushes""into_chunks, which doubles the array growth for declaration-heavy streamed blocks.charCodeAt(0)on""also returnsNaN, so the flattening read does nothing there.A guard keeps
_chunksat one entry per retractable piece and preserves the chunk-index contract used byanchorandmark, because an empty tail contributes no characters.♻️ Proposed guard
_cutTail() { + if (this._tailLength === 0) return; // Reading a character is what forces it flat; the value is not wanted, and // it is kept only so the read cannot be optimized away. this._flattened = this._tail.charCodeAt(0); this._chunks.push(this._tail); this._tail = ""; this._tailLength = 0; }Note:
_streamClosecallsdropTrailing(_frameFirstChunk[d], …)inlib/css/syntax.js, which walks back over empty pieces, so removing the empty pieces does not change its result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/util/SourceProcessor.js` around lines 319 - 339, Update _cutTail so it only flattens, appends _tail to _chunks, and resets tail state when _tail is non-empty; leave the state unchanged when empty. Preserve emitRetractable’s one-entry-per-retractable-piece indexing contract used by anchor and mark.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/css/syntax.js`:
- Around line 4143-4160: Update the streamed-rule entry flow around _streamOpen
and _streamEnterRule so _currentParent is assigned from the active parent
unconditionally before _rulePrelude runs, including when no enter visitor
exists. Preserve correct path.parent resolution for from-selector rewriting, and
add coverage for streamed `@keyframes` rules.
---
Outside diff comments:
In `@test/patches/jest-worker`+30.4.1.patch:
- Around line 32-35: Remove the empty catch around the Bun preload require so
failures from loading test/bun-preload.js propagate during worker bootstrap.
Keep the process.versions.bun guard and existing preload resolution unchanged.
---
Nitpick comments:
In `@lib/util/SourceProcessor.js`:
- Around line 319-339: Update _cutTail so it only flattens, appends _tail to
_chunks, and resets tail state when _tail is non-empty; leave the state
unchanged when empty. Preserve emitRetractable’s one-entry-per-retractable-piece
indexing contract used by anchor and mark.
In `@test/CssSyntax.unittest.js`:
- Around line 1024-1046: Add a test case in the existing state-cleanup describe
block that builds roughly 70 nested qualified rules with BIG at the innermost
level, then processes and validates the result through the normal walk/minify
assertions. Ensure the nesting reaches the _STREAM_MAX_DEPTH fallback in
_streamConsumeBlock while preserving coverage for state cleanup after
processing.
- Around line 761-776: Update the BIG-based test cases around
describe("CssSyntax — block streaming") that currently lack a streaming
assertion, including the cases near Lines 839, 848, 898, and 955. Add the same
childCount-based guard used by the existing pinned cases, or add one shared
assertion that verifies BIG takes the streamed path, so these tests fail if the
threshold changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d50ec46d-6a4c-4218-b278-6d721eed4761
⛔ Files ignored due to path filters (1)
types.d.tsis excluded by!types.d.ts
📒 Files selected for processing (6)
.github/workflows/test.ymllib/css/syntax.jslib/util/SourceProcessor.jspackage.jsontest/CssSyntax.unittest.jstest/patches/jest-worker+30.4.1.patch
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
`runtimes (bun)` segfaults at a different wild address each run, on this branch and on main alike. Reproduced locally with Bun 1.3.14, the CI patches and the CI command, and it is the idle-memory restart: with the limit below what the suite uses, workers restart constantly and Bun crashes tearing one down. limit vs the suite's heaps result 16MB below segfault, spawned(61) terminated(57) 64MB below segfault, spawned(20) terminated(16) 512MB above 103 suites, 28993 tests, green 2048MB above 103 suites, 28993 tests, green The last green CI run had no limit at all and finished all 159 suites with worker heaps reaching 1337 MB. Restoring the limit set it to the 512 MB the other runtimes use — under half of that — so it fires on workers doing ordinary work, and the run dies about a third of the way in. Raise it to 2048 MB: above what the suite needs, still a bound on a worker that runs away. Node and Deno keep 512 MB, where a spurious restart only costs time. Ending a worker rather than terminating it, as the patch already does, buys Bun a restart it would not otherwise survive — a small run took five and passed — but not a run full of them. Both teardown paths crash in bulk, so the limit is the lever, and the patch is left as it is.
769753e to
699b011
Compare
A prelude can be read in terms of what encloses it: `from` is the `0%` a keyframe selector means, but only inside `@keyframes`. `_streamOpen` bound the node and left the parent as whatever the last walked node had, because `_streamEnterRule` binds it only when the rule has an `enter` visitor to run — which printing on its own does not. A `@keyframes` selector whose own block crossed the threshold therefore printed as `from` where the collected printer prints `0%`, and the two disagreed on the same input. Also stop cutting an empty piece off the output. Every retractable piece cuts the accumulated tail off in front of it, so a run of declarations in a streamed block cut an empty one for each after the first: 3000 consecutive declarations built 6000 pieces where 3001 carry the whole output. Both found by CodeRabbit on the pull request, and both now covered: the keyframe case, and the fallback past the depth the frame table holds, which nothing reached before.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/util/SourceProcessor.js (1)
233-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCut the tail before capturing
writer.mark.markstores only_chunks.length. If_tailcontains preceding output,emitRetractable()moves it into the chunk atmark. After the marked piece is retracted,dropTrailing(mark, ...)can remove a trailing character from that preceding output. Add a regression test for preceding tail output and a retracted final piece.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/util/SourceProcessor.js` around lines 233 - 238, The mark getter must account for pending _tail output before capturing the next emitted piece’s position. Update the mark/emitRetractable flow in SourceProcessor so preceding tail content is cut into chunks before reading writer.mark, ensuring dropTrailing(mark, ...) cannot retract characters from earlier output; add a regression test covering preceding tail output followed by a retracted final piece.
🧹 Nitpick comments (1)
lib/util/SourceProcessor.js (1)
320-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the new
_cutTailcomment.The comment spans four lines. Keep only the reason for the early return and fit it in two short lines.
Proposed comment
- // Nothing has accumulated since the last cut — which is every retractable - // piece after the first in a run of them. A piece holding no characters - // would only pad the output, and an anchor's offset into an empty tail is - // the start of whatever is cut next either way. + // Skip empty tails to avoid padding chunks. + // Anchor offsets remain unchanged.As per coding guidelines, comments under
lib/must be at most two short lines and add non-obvious information. As per path instructions, comments underlib/**must be at most two short lines and add non-obvious information.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/util/SourceProcessor.js` around lines 320 - 324, Shorten the comment immediately above the _tailLength === 0 early return in _cutTail to no more than two short lines, retaining only the non-obvious reason for returning when no characters have accumulated.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/util/SourceProcessor.js`:
- Around line 233-238: The mark getter must account for pending _tail output
before capturing the next emitted piece’s position. Update the
mark/emitRetractable flow in SourceProcessor so preceding tail content is cut
into chunks before reading writer.mark, ensuring dropTrailing(mark, ...) cannot
retract characters from earlier output; add a regression test covering preceding
tail output followed by a retracted final piece.
---
Nitpick comments:
In `@lib/util/SourceProcessor.js`:
- Around line 320-324: Shorten the comment immediately above the _tailLength ===
0 early return in _cutTail to no more than two short lines, retaining only the
non-obvious reason for returning when no characters have accumulated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d86fbc8-a32d-4b5d-84b6-f0b06cf55daa
📒 Files selected for processing (3)
lib/css/syntax.jslib/util/SourceProcessor.jstest/CssSyntax.unittest.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/CssSyntax.unittest.js
- lib/css/syntax.js
The `;` a `}` makes redundant is dropped by walking the output back from the
end, bounded by where the block started. That bound was taken while the
block's opener was still held back, so the piece it names can hold output
from before the block — the walk stops short of it today only because the
opener's piece ends in `{` and is never taken back, which is an invariant
nothing states. Take the bound after cutting the tail, so it names the
block's own first piece and the walk cannot reach past it either way.
The bound now covers a case it did not: a run of retractable pieces already
cuts after each, so a mark taken between them was mid-tail.
Raised by CodeRabbit as a correctness risk. It is not reachable today —
reverting either the cut or the bound entirely leaves every test passing —
so this is the invariant made explicit rather than a fix. The new case pins
what is observable: a declaration before a streamed block keeps its `;` when
the block drops its own.
Summary
The CSS grammar streams and recycles per top-level node, so anything wrapped in one large block holds its whole subtree: 20k rules inside a single
@mediakeep 8.05 MiB of columns, against 0 for the same rules written flat. This enters a rule once its block grows past a threshold, then walks and recycles each later child as it completes, so peak storage is the open path rather than the block. Draft: the memory goal is met on every shape, CPU is not yet at parity (see below).Measured, 20k rules per shape, walk-only, interleaved min-of-N:
@media { …20k… }@supports { …20k… }@media{@supports{ … }}@mediaStreaming is opt-in (
streamBlocks) and walk-only, so nothing inCssParseror the printer changes yet.What kind of change does this PR introduce?
perf
Did you add tests for your changes?
Not yet — draft. Verified so far with a differential against the non-streaming walk (27 shapes including six past 6,000 rules): 0 sequence differences, every delta being the provisional
endreported atenter, 0 atexit. Existing suites are unaffected because streaming is opt-in: 320/320CssSyntaxunit tests and 917/917configCases/css.Does this PR introduce a breaking change?
No — opt-in and off by default. Two semantic notes for when it is enabled: a rule entered before its block closes reports a provisional
endatenter(final atexit), and a streamed rule hands its children to the visitors instead of collecting them, soA.declarations/A.childRulesread empty on it.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a — internal parser option, not part of the public config surface.
Use of AI
Claude Code was used to implement this change and to run the measurements and the differential harness reported above. All numbers in this description are from runs on this branch against
main; the design decisions and the correctness checks were reviewed by a human before each commit.Generated by Claude Code
Summary by CodeRabbit
Performance
Bug Fixes
Reliability