From 35dd84796ce3e7cafbe044919ac70f38b5e4d89a Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Mon, 24 Aug 2026 10:11:45 +0200 Subject: [PATCH 1/5] feat: optimizing savings-only mode Signed-off-by: Jens Oliver Meiert --- CHANGELOG.md | 11 ++++++ README.md | 26 +++++++------ package-lock.json | 12 +++--- package.json | 4 +- src/cli/render.js | 18 +++++++-- src/consolidate.js | 84 +++++++++++++++++++++++++++--------------- src/lib/transaction.js | 57 ++++++++++++++++++++++++++++ src/merge.js | 61 ++++++++++++++++++++++++++++-- src/plugin.js | 2 +- test/cli.test.js | 44 +++++++++++++++++++++- test/dedup.test.js | 57 +++++++++++++++++++++++++++- test/helpers.js | 13 +++++++ test/plugin.test.js | 2 +- 13 files changed, 329 insertions(+), 62 deletions(-) create mode 100644 src/lib/transaction.js diff --git a/CHANGELOG.md b/CHANGELOG.md index f7f2c4f..667e64c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to CSS Dedup are documented in this file, which is (mostly) The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.13.0] - 2026-08-24 + +### Fixed + +* Corrected the `--fix` example output in the README + +### Changed + +* Changed `--savings-only` (`savingsOnly`) to decide per merge instead of per file, so a style sheet keeps the consolidations that save bytes even when others would lead to gains + - Adjusted `withheld: { count, bytes }` to report the merges that were declined and what applying them, too, would have cost, rather than the whole file’s consolidation + ## [1.12.1] - 2026-08-13 ### Changed diff --git a/README.md b/README.md index 74811a9..463f0a8 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Findings -f (-a) Savings with: -f -f -s -f -a -f -a -s Legend: -f: --fix, -a: --aggressive, -s: --savings-only ``` -(A cell reads `n/a` when no findings would be safe to apply, or if `--savings-only` would decline the merge for growing the file.) +(A cell reads `n/a` when no findings would be safe to apply, or if `--savings-only` would decline every merge on offer for growing the file.) Running with `--fix` folds `.a` and `.b` into a single rule for the shared declaration: @@ -50,15 +50,15 @@ Running with `--fix` folds `.a` and `.b` into a single rule for the shared decla ```shell $ npx css-dedup --fix default.css -1 consolidated, 0 skipped -63 → 53 bytes (-10 B, -15.9%) -Wrote default.css +Summary: +* 1 declaration consolidated: Reduced duplication and saved 10 bytes (64 → 54 bytes, -15.6%) +* Wrote default.css ``` Since duplicate declarations cost bytes wherever they live—in the style sheet itself, and (uncompressed) over the wire—the byte counts reflect two payoffs at once: less to maintain, and less to transfer. -The two aren’t always aligned, though: Folding a declaration into a shared selector list adds that list’s bytes back, so consolidating one that only has a couple of long, otherwise-unrelated selectors in common can end up costing more than it removes. CSS Dedup’s two modes call this out, so it’s a decision you can make consciously—it’s worth it if you value using declarations once for maintainability, yet not if you’re optimizing purely for transfer size. (`--fix --savings-only` automates that call: A file whose consolidation would grow it is left untouched.) +The two aren’t always aligned, though: Folding a declaration into a shared selector list adds that list’s bytes back, so consolidating one that only has a couple of long, otherwise-unrelated selectors in common can end up costing more than it removes. CSS Dedup’s two modes call this out, so it’s a decision you can make consciously—it’s worth it if you value using declarations once for maintainability, yet not if you’re optimizing purely for transfer size. (`--fix --savings-only` automates that call, merge by merge: Every consolidation that would grow the style sheet is left out, and the ones that pay for themselves still apply.) ## Usage @@ -76,7 +76,7 @@ The input is CSS. A preprocessor source named as an argument (.scss, .sass, .les | --- | --- | | `--fix`, `-f` | Consolidate declarations that are safe to merge automatically, rewriting each file in place (or printing to STDOUT for `-`) | | `--aggressive`, `-a` | Also apply merges that are probably—but not provably—safe (see [aggressive mode](#aggressive-mode)); only applies together with `--fix`, since report mode’s table already previews both variants automatically | -| `--savings-only`, `-s` | Leave a file untouched when its consolidation would make it bigger, not smaller (checked per file); only applies together with `--fix`, since report mode doesn’t write | +| `--savings-only`, `-s` | Leave out each consolidation that would make the file bigger rather than smaller, keeping the ones that save bytes (checked per merge); only applies together with `--fix`, since report mode doesn’t write | | `--ignore-selector `, `-i` | Regular expression for selectors to exclude from analysis (repeatable) | | `--no-ignore-selectors-defaults`, `-n` | Disable the built-in selector hack ignore list | | `--ignore-path `, `-p` | Regular expression tested against each file’s path, relative to the working directory; a match excludes the file (repeatable) | @@ -112,7 +112,7 @@ These are the supported options with their defaults (each can be omitted): // css-dedup.config.js export default { aggressive: false, // set to `true` to also allow probably-safe merges - savingsOnly: false, // set to `true` to skip files whose consolidation would grow them (`--fix` runs only) + savingsOnly: false, // set to `true` to leave out the individual merges that would grow the file (`--fix` runs only) ignoreSelectors: [], // additional selector patterns to exclude, e.g., `[/^\.legacy-/]` ignoreSelectorsDefaults: true, // set to `false` to disable the built-in hack list ignorePaths: [], // file paths to exclude, matched relative to the working directory, e.g., `[/dist\//]` @@ -141,7 +141,7 @@ Both functions accept an options object: { from: 'path/to/file.css', // forwarded to PostCSS; names the file in syntax-error messages aggressive: false, // set to `true` to also allow probably-safe merges - savingsOnly: false, // set to `true` to withhold a consolidation that would grow the style sheet (`dedup()` only) + savingsOnly: false, // set to `true` to withhold each merge that would grow the style sheet (`dedup()` only) ignoreSelectors: [/^\.legacy-/], // additional selector patterns to exclude ignoreSelectorsDefaults: true // set to `false` to disable the built-in hack list } @@ -162,9 +162,9 @@ Both functions accept an options object: `dedup()` returns `{ css, applied, skipped, bytes }`: `css` is the rewritten style sheet; `applied` lists what it did—each entry has `redundant: true` if it just dropped a same-rule (or same-at-rule-block) duplicate, `folded: true` if it folded a rule repeating the same selector into a later one, absent if it folded selectors from separate rules into one; `skipped` lists duplicate groups (and blocked same-selector folds) it left untouched along with why; and `bytes` is `{ before, after, saved }`—UTF-8 byte counts of the style sheet before and after, since that’s what changes over the wire, not the character count, covering everything `--fix` did as one net figure. -`saved` is `before - after`, so it’s negative on the rare file where the added selector-list text outweighs the removed declarations—dropping a same-rule duplicate never costs bytes, only folding selectors from separate rules can. With `savingsOnly: true`, a consolidation whose net `saved` would be negative is withheld: `css` comes back untouched, `applied` is empty, `bytes` reports no change (that’s what actually happened), and the declined outcome arrives as `withheld: { count, bytes }`—the number of merges and the byte counts the consolidation would have had (`withheld` is absent whenever nothing was withheld). `dedupRoot()` (the same function, operating on an already-parsed PostCSS root instead of a CSS string) returns the same shape minus `css`. +`saved` is `before - after`, so it’s negative on the rare file where the added selector-list text outweighs the removed declarations—dropping a same-rule duplicate never costs bytes, only folding selectors from separate rules can. With `savingsOnly: true`, each merge that would make `saved` more negative is withheld on its own, leaving the rest in place: `css` comes back with whatever paid for itself applied, `bytes` reports what actually happened, and the declined merges arrive as `withheld: { count, bytes }`—how many, and the byte counts the style sheet would have had with them applied too (`withheld` is absent whenever nothing was withheld). A file offering nothing but growing merges therefore still comes back untouched, with `applied` empty; one that mixes them keeps its savings. `dedupRoot()` (the same function, operating on an already-parsed PostCSS root instead of a CSS string) returns the same shape minus `css`. -`dedup()` additionally returns `sourceMapStale: true` when the style sheet passed in carried a `/*# sourceMappingURL=… */` comment _and_ the run actually rewrote it—the CLI’s source map caveat in machine-readable form, so a wrapper can surface it instead of shipping a broken map unnoticed. It’s absent otherwise, including when nothing was applied or the consolidation was withheld, since the style sheet (and hence the map’s accuracy) is then unchanged. `dedupRoot()` doesn’t report it, deliberately: It’s what [the PostCSS plugin](#postcss-plugin) calls, and that path stringifies through PostCSS, which emits a correct map of its own. +`dedup()` additionally returns `sourceMapStale: true` when the style sheet passed in carried a `/*# sourceMappingURL=… */` comment _and_ the run actually rewrote it—the CLI’s source map caveat in machine-readable form, so a wrapper can surface it instead of shipping a broken map unnoticed. It’s absent otherwise, including when nothing was applied or every merge was withheld, since the style sheet (and hence the map’s accuracy) is then unchanged. `dedupRoot()` doesn’t report it, deliberately: It’s what [the PostCSS plugin](#postcss-plugin) calls, and that path stringifies through PostCSS, which emits a correct map of its own. ### PostCSS Plugin @@ -183,7 +183,7 @@ const fixed = await postcss([cssdedup({ fix: true })]).process(css, { from: 'def console.log(fixed.css); ``` -The plugin takes the same options as `analyze()`/`dedup()`, plus `fix: true` to switch it into consolidation mode (`aggressive: true` and `savingsOnly: true` work here, too—a withheld consolidation leaves the root untouched and surfaces as a warning). Since CSS Dedup is a source-hygiene tool—more like `stylelint --fix` than a bundle optimizer—it fits well early in a pipeline, on hand-authored CSS, before Autoprefixer and before minification; running it after either may overlap with work those tools do. (CSS Dedup also works as a last step, however, as with applying `--savings-only` optimizations to minified files.) +The plugin takes the same options as `analyze()`/`dedup()`, plus `fix: true` to switch it into consolidation mode (`aggressive: true` and `savingsOnly: true` work here, too—withheld merges are left out of the root and surface as a warning). Since CSS Dedup is a source-hygiene tool—more like `stylelint --fix` than a bundle optimizer—it fits well early in a pipeline, on hand-authored CSS, before Autoprefixer and before minification; running it after either may overlap with work those tools do. (CSS Dedup also works as a last step, however, as with applying `--savings-only` optimizations to minified files.) ## How It Works @@ -227,6 +227,7 @@ CSS Dedup: - If a merged rule (including the last occurrence itself) also carries a declaration for an overlapping property, that declaration is split out into its own small rule—keeping that occurrence’s own, original selector—placed right after the merged rule, rather than blocking the merge outright: Folding every selector onto one shared declaration block would otherwise hand that overlapping extra to selectors that never had it. Exception: If that extra is itself duplicated elsewhere in the same scope, it’s left alone and the whole merge is skipped instead, since splitting it here would orphan that other duplicate’s own merge. - If something does block it, the merge is skipped and reported rather than risking a cascade change. A blocker fences, though—it doesn’t forbid: Occurrences on the same side of it still consolidate among themselves (their own spans are clean, so the same safety argument applies), and the group is reported as skipped either way, since the duplicate keeps existing across the blocker. - Consolidation runs to a fixed point: One merge can unblock or create another (a fresh merged rule may repeat an existing rule’s selector list, an emptied rule stops fencing a span), so the passes repeat until nothing changes. + - With `--savings-only`, each merge is weighed on its own before it stands: The consolidation is performed, the style sheet measured, and the merge undone again if it turned out to cost bytes. Entangled groups are weighed together, since they can only be applied together; the strategies that can’t cost bytes (collapsing a repeat within one rule, folding two same-selector rules) aren’t weighed at all. Measuring the real thing beats predicting it—a merge that empties a rule or a conditional block saves far more than its declaration arithmetic suggests. Overall, CSS Dedup is conservative by design and will leave some safe merges for manual review. @@ -282,7 +283,7 @@ src/ index.js Public API (re-exports only) plugin.js PostCSS plugin wrapper analyze.js Read-only detection (step 5 above) - consolidate.js The `savingsOnly` gate and the fixed-point loop + consolidate.js The fixed-point loop and the result shape merge.js The merge strategies (step 6 above) lib/ Engine internals @@ -294,6 +295,7 @@ src/ shorthands.js Shorthand/longhand overlap hacks.js The default selector ignore list (step 3) caches.js Per-run memoization lifecycle + transaction.js Snapshot/rollback, for weighing one merge’s bytes util.js Shared helpers cli/ CLI internals diff --git a/package-lock.json b/package-lock.json index 336c6b6..d55638a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "css-dedup", - "version": "1.12.1", + "version": "1.13.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "css-dedup", - "version": "1.12.1", + "version": "1.13.0", "license": "MIT", "dependencies": { "postcss": "^8.5.26" @@ -16,7 +16,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", - "eslint": "^10.8.1", + "eslint": "^10.9.0", "globals": "^17.11.0", "typescript": "^7.0.2" }, @@ -678,9 +678,9 @@ } }, "node_modules/eslint": { - "version": "10.8.1", - "resolved": "https://registry.npmjs.org/eslint/-/eslint-10.8.1.tgz", - "integrity": "sha512-wqA7W2jbsC/BnV9Iv1UZpKVFkO1AdNoSmYW8NWG4HNOBbkAMvIqDZ27pI2f07dqn583NcIC44ckjAcOXDL1QbQ==", + "version": "10.9.0", + "resolved": "https://registry.npmjs.org/eslint/-/eslint-10.9.0.tgz", + "integrity": "sha512-5KeEOJZBfEVA47boFiBsf+6MmmJpffM7qEBg4pLla2e4nlKgdKlqCW0oSLOGsT8Wl5uCGJptLV1bkaiShj90Gw==", "dev": true, "license": "MIT", "workspaces": [ diff --git a/package.json b/package.json index db0f58c..5b38974 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "description": "CSS declaration deduplicator for maintainability and performance optimization", "devDependencies": { "@eslint/js": "^10.0.1", - "eslint": "^10.8.1", + "eslint": "^10.9.0", "globals": "^17.11.0", "typescript": "^7.0.2" }, @@ -60,5 +60,5 @@ }, "type": "module", "types": "src/index.d.ts", - "version": "1.12.1" + "version": "1.13.0" } diff --git a/src/cli/render.js b/src/cli/render.js index 55e599b..38b9428 100644 --- a/src/cli/render.js +++ b/src/cli/render.js @@ -117,8 +117,10 @@ function renderFixPass(payload, { isStdin, label, multi, flags }) { // The outcome first, then this run’s footnotes on it (growth caveat, // aggressive-only warning, where it was written), then the two - // forward-looking items (what was skipped, what `--aggressive` adds) - if (withheld) { + // forward-looking items (what was skipped, what `--aggressive` adds). + // The gate decides per merge, so a file can come out partly consolidated— + // “left this file untouched” only fits the run where nothing survived it. + if (withheld && !applied) { log(`* 0 declarations consolidated, ${withheld.count} withheld: \`savingsOnly\` left this file untouched—consolidating would ${formatByteDeltaClause(withheld.bytes.saved, withheld.bytes.before)}`); } else { log(`* ${applied} declaration${plural(applied)} consolidated${applied ? `: ${formatAppliedReduceClause(bytes)}` : ''}`); @@ -128,6 +130,9 @@ function renderFixPass(payload, { isStdin, label, multi, flags }) { if (bytes.saved < 0) { log('* Worth it for maintainability (each declaration used once); skip `--fix` here if you care more about transfer size.'); } + if (withheld) { + log(`* ${withheld.count} further merge${plural(withheld.count)} withheld by \`savingsOnly\`—applying ${withheld.count !== 1 ? 'them' : 'it'} too would ${formatByteDeltaClause(withheld.bytes.saved, withheld.bytes.before)}`); + } if (aggressiveDiffers) { const share = aggressiveOnly > 0 ? `${aggressiveOnly} of these merges ${aggressiveOnly !== 1 ? 'are' : 'is'}` @@ -267,8 +272,12 @@ function printOverallFixSummary(ok) { const shrinkTotal = sumBy(filesShrink, result => result.stats.bytesSaved); const growTotal = Math.abs(sumBy(filesGrow, result => result.stats.bytesSaved)); - const withheldFiles = ok.filter(result => result.stats.withheldCount > 0); + // Split by what the gate left behind—a file where nothing survived it, and + // one that kept the merges that paid for themselves, warrant different words + const withheldFiles = ok.filter(result => result.stats.withheldCount > 0 && !result.stats.applied); const withheldGrowthTotal = sumBy(withheldFiles, result => result.stats.withheldGrowth); + const partialFiles = ok.filter(result => result.stats.withheldCount > 0 && result.stats.applied); + const partialGrowthTotal = sumBy(partialFiles, result => result.stats.withheldGrowth); const aggFiles = ok.filter(result => result.stats.aggDiffers); const aggFilesShrink = aggFiles.filter(result => result.stats.aggExtraSaved > 0); @@ -299,6 +308,9 @@ function printOverallFixSummary(ok) { if (withheldFiles.length) { console.log(`* ${withheldFiles.length} file${plural(withheldFiles.length)} left untouched by \`--savings-only\`—consolidating would have made ${withheldFiles.length !== 1 ? 'them' : 'it'} ${formatBytesShareOfTotal(withheldGrowthTotal, totalBeforeAll)} bigger in total`); } + if (partialFiles.length) { + console.log(`* ${partialFiles.length} file${plural(partialFiles.length)} had further merges withheld by \`--savings-only\`—applying those too would have added ${formatBytesShareOfTotal(partialGrowthTotal, totalBeforeAll)} in total`); + } if (!aggFiles.length) return; diff --git a/src/consolidate.js b/src/consolidate.js index 190cd66..33568c8 100644 --- a/src/consolidate.js +++ b/src/consolidate.js @@ -44,7 +44,7 @@ function removeEmptiedConditionBlocks(root, initiallyEmpty) { // is bound once here, so no call site can fall back to default-mode // normalization by forgetting a flag—which would silently give one declaration // two different keys in different phases of an aggressive run. -function createContext(root, options) { +function createContext(root, options, settle) { const aggressive = options.aggressive ?? false; return { aggressive, @@ -55,6 +55,15 @@ function createContext(root, options) { spacedCommas: usesSpacedCommas(root), applied: [], skipped: [], + // The `savingsOnly` gate in `merge.js` needs the whole style sheet to + // weigh one merge (a cross-block merge’s saving lands outside the scope it + // happens in) and `settle()` to see the state it would actually ship + savingsOnly: options.savingsOnly ?? false, + root, + settle, + // Running size of the style sheet, carried between clusters (see `gated()`) + bytesNow: null, + declined: [], }; } @@ -87,17 +96,25 @@ function consolidateRoot(root, options = {}) { // Bytes, not characters—the effectiveness this measures (fewer bytes over // the wire) is a transfer-size concern. const before = Buffer.byteLength(root.toString(), 'utf8'); - const ctx = createContext(root, options); // Blocks already empty in the source, so the cleanup at the end only ever // removes what this run emptied const initiallyEmpty = new Set(); - if (ctx.aggressive) { + const aggressive = options.aggressive ?? false; + if (aggressive) { root.walkAtRules(atrule => { if (atrule.nodes && !atrule.nodes.length) initiallyEmpty.add(atrule); }); } + // Bringing the style sheet to the state it would ship in. Idempotent, so the + // gate can call it around every merge it weighs and the run can call it once + // more at the end. + const settle = () => { + if (aggressive) removeEmptiedConditionBlocks(root, initiallyEmpty); + }; + const ctx = createContext(root, options, settle); + // One merge can unblock or create another: a fresh merged rule may twin with // an existing one, and an emptied rule stops fencing the spans it sat in—so // passes repeat until nothing changes. Termination is guaranteed, since @@ -108,42 +125,49 @@ function consolidateRoot(root, options = {}) { while (ctx.applied.length !== appliedCount) { appliedCount = ctx.applied.length; ctx.skipped.length = 0; + ctx.declined.length = 0; + // The phases before the gated merges change the style sheet, so the + // running size has to be taken afresh each pass + ctx.bytesNow = null; runPass(root, ctx); } - if (ctx.aggressive) removeEmptiedConditionBlocks(root, initiallyEmpty); + settle(); const after = Buffer.byteLength(root.toString(), 'utf8'); - return { applied: ctx.applied, skipped: ctx.skipped, bytes: { before, after, saved: before - after } }; + return { + applied: ctx.applied, + skipped: ctx.skipped, + declined: ctx.declined, + bytes: { before, after, saved: before - after }, + }; } -// The `savingsOnly` gate: consolidation runs on a detached clone first, and -// only a result that doesn’t grow the style sheet is grafted back onto the -// real root—which is what lets the PostCSS plugin and the CLI share one -// implementation of the policy. A withheld result reports `applied: []` and -// unchanged bytes (what actually happened), with the would-be outcome under -// `withheld` so callers can explain what was declined. A net-zero result still -// applies (deduplicated at no byte cost). +// The `savingsOnly` gate is decided per cluster, inside `merge.js`: Every +// merge that would grow the style sheet is performed, measured, and undone, +// leaving the ones that pay for themselves in place. A file therefore keeps +// its savings even when other merges in it would have cost more than the whole +// consolidation gains—the case that used to sink every merge in the file with +// it. A net-zero merge still applies (deduplicated at no byte cost). +// +// What was declined arrives as `withheld: { count, bytes }`: How many merges, +// and the byte counts the style sheet would have had with them applied, too. +// Absent when nothing was declined. export function dedupRoot(root, options = {}) { - if (!options.savingsOnly) return consolidateRoot(root, options); - - const clone = root.clone(); - const result = consolidateRoot(clone, options); - if (result.bytes.saved < 0) { - return { - applied: [], - skipped: result.skipped, - bytes: { before: result.bytes.before, after: result.bytes.before, saved: 0 }, - withheld: { count: result.applied.length, bytes: result.bytes }, - }; - } + const { declined, ...result } = consolidateRoot(root, options); + if (!declined.length) return result; - if (result.applied.length) { - root.raws = clone.raws; - root.removeAll(); - root.append(clone.nodes); - } - return result; + // Clusters are independent by construction, so the cost of applying the + // declined merges as well is the sum of what each was measured to cost + const cost = declined.reduce((total, entry) => total + entry.cost, 0); + const count = declined.reduce((total, entry) => total + entry.count, 0); + return { + ...result, + withheld: { + count, + bytes: { before: result.bytes.after, after: result.bytes.after + cost, saved: -cost }, + }, + }; } // A `/*# sourceMappingURL=… */` comment means a build tool generated this diff --git a/src/lib/transaction.js b/src/lib/transaction.js new file mode 100644 index 0000000..a63622d --- /dev/null +++ b/src/lib/transaction.js @@ -0,0 +1,57 @@ +// Snapshot and rollback around one speculative merge, for the `savingsOnly` +// gate. The gate measures a merge by performing it and undoing it again when +// it turns out to cost bytes, rather than by predicting its cost up front—so +// no merge strategy has to carry a second, parallel implementation of itself +// in byte arithmetic, and the figure the gate decides on is the real one. + +// Every container whose child list a merge can touch: the rules’ own parents, +// plus their ancestors up to the root—an aggressive cross-block merge can +// drain a conditional block, whose removal changes *its* parent’s child list. +function affectedContainers(rules) { + const containers = new Set(); + for (const rule of rules) { + for (let node = rule.parent; node; node = node.parent) containers.add(node); + } + return [...containers]; +} + +export function byteLength(root) { + return Buffer.byteLength(root.toString(), 'utf8'); +} + +/** + * Everything needed to put a scope back exactly as it stands right now. + * Cheap enough to take per cluster—the node clones are shallow copies of one + * cluster’s rules, not of the style sheet. + */ +export function snapshot(scope, rules) { + return { + // Residual rules are appended here as merges create them, so the array + // itself is state a rollback has to restore + scopeRules: scope.rules.slice(), + containers: affectedContainers(rules).map(container => ({ container, nodes: container.nodes.slice() })), + rules: rules.map(rule => ({ rule, clone: rule.clone(), parent: rule.parent })), + }; +} + +// Restores the child lists first (which un-removes emptied rules and drops +// inserted residuals), then each rule’s own contents. The rule objects are the +// same identities throughout, so references held elsewhere—`scope.rules`, a +// cluster’s `distinctRules`—stay live across a rollback. +export function rollback(snap, scope) { + for (const { container, nodes } of snap.containers) { + container.nodes = nodes; + for (const node of nodes) node.parent = container; + } + + for (const { rule, clone, parent } of snap.rules) { + rule.selector = clone.selector; + rule.raws = { ...clone.raws }; + rule.removeAll(); + rule.append(clone.nodes.map(node => node.clone())); + rule.parent = parent; + } + + scope.rules.length = 0; + scope.rules.push(...snap.scopeRules); +} \ No newline at end of file diff --git a/src/merge.js b/src/merge.js index 60b123f..0bdb9ab 100644 --- a/src/merge.js +++ b/src/merge.js @@ -11,6 +11,7 @@ import { splitSelectors, selectorsAreMutuallyExclusive, selectorsLikelyDisjoint import { propertiesOverlap } from './lib/shorthands.js'; import { insertAfter, joinSelectors, typicalSeparator } from './lib/style.js'; import { declsOf, pushTo } from './lib/util.js'; +import { byteLength, rollback, snapshot } from './lib/transaction.js'; // All occurrences of a key are equivalent by our own normalization rules, so // the merge keeps whichever raw spelling is shortest rather than whatever the @@ -632,6 +633,53 @@ function clusterGroups(groups) { return [...clusters.values()]; } +// The `savingsOnly` gate, applied per cluster. Clusters are exactly the units +// that can be decided independently—union-find has already put every group +// sharing a rule into the same one—so accepting one and declining another +// cannot leave a half-applied merge behind. +// +// Only declaration merges are gated. The other two strategies don’t cost +// bytes: Collapsing a repeat within one rule removes text and adds none, and +// folding two same-selector rules removes a whole selector and its braces. +function gated(ctx, scope, cluster, apply) { + if (!ctx.savingsOnly) { + apply(); + return; + } + + // Measurements are taken with emptied conditional blocks already cleared + // away, so a block drained by an *earlier* cluster is not counted as this + // one’s saving. `settle()` is idempotent, so this only ever does work on the + // first cluster of a pass—the phases before it are the ones that can leave a + // block behind. + ctx.settle(); + // The style sheet only changes here between clusters, so the size measured + // after one accepted merge is the size the next one starts from: one + // stringify per cluster rather than a fresh before-and-after pair. Reset to + // `null` at the start of every pass, where the earlier phases have run. + ctx.bytesNow ??= byteLength(ctx.root); + + const rules = [...new Set(cluster.flatMap(group => group.distinctRules))]; + const snap = snapshot(scope, rules); + const appliedBefore = ctx.applied.length; + + apply(); + ctx.settle(); + + const after = byteLength(ctx.root); + const cost = after - ctx.bytesNow; + if (cost <= 0) { + ctx.bytesNow = after; + return; + } + + rollback(snap, scope); + // `skipped` is left as it stands—whatever the safety checks concluded about + // this cluster is still true, and still worth reporting + ctx.declined.push({ scope: scope.label, keys: cluster.map(group => group.key), count: ctx.applied.length - appliedBefore, cost }); + ctx.applied.length = appliedBefore; +} + export function mergeDuplicateGroups(ctx, scope) { const byKey = new Map(); for (const rule of eligibleRules(scope, ctx.ignorePatterns)) { @@ -661,8 +709,10 @@ export function mergeDuplicateGroups(ctx, scope) { } if (!outsideBlocker) { - if (cluster.length === 1) mergeSoloGroup(ctx, scope, cluster[0]); - else mergeCluster(ctx, scope, cluster); + gated(ctx, scope, cluster, () => { + if (cluster.length === 1) mergeSoloGroup(ctx, scope, cluster[0]); + else mergeCluster(ctx, scope, cluster); + }); continue; } @@ -671,12 +721,15 @@ export function mergeDuplicateGroups(ctx, scope) { const reason = `intervening ${propDescription} declaration in \`${blocking.rule.selector}\` (line ${blocking.rule.source?.start?.line})`; if (cluster.length === 1) { - mergePartialGroup(ctx, scope, group, reason); + gated(ctx, scope, cluster, () => mergePartialGroup(ctx, scope, group, reason)); continue; } + gated(ctx, scope, cluster, () => { + for (const member of cluster) mergeClusterGroupRuns(ctx, scope, member); + }); + for (const member of cluster) { - mergeClusterGroupRuns(ctx, scope, member); ctx.skipped.push({ scope: scope.label, key: member.key, diff --git a/src/plugin.js b/src/plugin.js index 80308b2..6022216 100644 --- a/src/plugin.js +++ b/src/plugin.js @@ -12,7 +12,7 @@ export default function cssdedup(options = {}) { if (options.fix) { const { skipped, withheld } = dedupRoot(root, options); if (withheld) { - root.warn(result, `Consolidation withheld (\`savingsOnly\`): ${withheld.count} merge${withheld.count !== 1 ? 's' : ''} would make the style sheet ${Math.abs(withheld.bytes.saved)} bytes bigger`); + root.warn(result, `${withheld.count} merge${withheld.count !== 1 ? 's' : ''} withheld (\`savingsOnly\`): applying ${withheld.count !== 1 ? 'them' : 'it'} would make the style sheet ${Math.abs(withheld.bytes.saved)} bytes bigger`); } for (const item of skipped) { root.warn(result, `Duplicate \`${item.key}\` left unmerged (${item.scope === 'root' ? 'root' : item.scope}): ${item.reason}`); diff --git a/test/cli.test.js b/test/cli.test.js index e4165f4..b8db58d 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -5,7 +5,7 @@ import path from 'node:path'; import { availableParallelism } from 'node:os'; import { poolSize, shouldParallelize } from '../src/cli/pool.js'; import { dedup } from '../src/index.js'; -import { BEST_CELL, RE_MERGED_AB, RE_MERGED_AC, RE_PAYOFF_FIX, RE_SYNTAX_ERROR, RE_SYNTAX_ERROR_UNCLOSED, RE_WITHHELD_ONE, cssGrowing, cssGrowingAggressive, cssShrinkingAggressive, dirTest, findingsRow, fixturesDir, makeTempDir, run, runColor, runTty } from './helpers.js'; +import { BEST_CELL, RE_MERGED_AB, RE_MERGED_AC, RE_PAYOFF_FIX, RE_SYNTAX_ERROR, RE_SYNTAX_ERROR_UNCLOSED, RE_WITHHELD_ONE, cssGrowing, cssGrowingAggressive, cssMixed, cssShrinkingAggressive, dirTest, findingsRow, fixturesDir, makeTempDir, run, runColor, runTty } from './helpers.js'; describe('CLI', () => { test('Shows help with `--help`', () => { @@ -214,13 +214,53 @@ describe('CLI', () => { assert.ok(detailIndex < countsIndex); // The counts line—the run’s conclusion—must be among the last things // printed, not stranded above the skipped-group detail - assert.ok(stdout.includes('more declarations in aggressive mode')); + assert.match(stdout, /\d+ more declarations? in aggressive mode/); assert.match(stdout, /\* 0 declarations consolidated, 1 withheld:.*\n\* 1 finding skipped \(considered unsafe to auto-merge\)/); } finally { fs.rmSync(dirTemp, { recursive: true, force: true }); } }); + test('`--fix --savings-only` keeps the merges that pay for themselves and reports the rest as withheld', () => { + const dirTemp = makeTempDir('temp_savings_partial'); + const file = path.join(dirTemp, 'mixed.css'); + fs.writeFileSync(file, cssMixed); + + try { + const { stdout } = run(['--fix', '--savings-only', file]); + // The outcome line states what was consolidated, not that the file was + // left untouched—the gate decides per merge + assert.match(stdout, /\* \d+ declarations? consolidated: Reduced duplication and saved \d+ bytes \(\d+ → \d+ bytes, -\d+\.\d%\)/); + assert.doesNotMatch(stdout, /left this file untouched/); + assert.match(stdout, /\* 1 further merge withheld by `savingsOnly`—applying it too would grow by \d+ bytes \(\+\d+\.\d%\)/); + + const output = fs.readFileSync(file, 'utf8'); + assert.match(output, /\.p,\s*\.q,\s*\.r\s*{\s*margin: 0;\s*}/); + assert.match(output, /\.b\s*{\s*color: red;\s*}/); + assert.ok(Buffer.byteLength(output, 'utf8') < Buffer.byteLength(cssMixed, 'utf8')); + } finally { + fs.rmSync(dirTemp, { recursive: true, force: true }); + } + }); + + test('`--fix --savings-only` distinguishes a partly consolidated file from an untouched one in the overall summary', () => { + const dirTemp = makeTempDir('temp_multi_summary_partial'); + const filePartial = path.join(dirTemp, 'partial.css'); + const fileGrow = path.join(dirTemp, 'grow.css'); + fs.writeFileSync(filePartial, cssMixed); + fs.writeFileSync(fileGrow, cssGrowing); + + try { + const { stdout } = run(['--fix', '--savings-only', filePartial, fileGrow]); + assert.match(stdout, /\* 1 file left untouched by `--savings-only`/); + assert.match(stdout, /\* 1 file had further merges withheld by `--savings-only`—applying those too would have added \d+ bytes \(\d+\.\d% overall\) in total/); + assert.strictEqual(fs.readFileSync(fileGrow, 'utf8'), cssGrowing); + assert.notStrictEqual(fs.readFileSync(filePartial, 'utf8'), cssMixed); + } finally { + fs.rmSync(dirTemp, { recursive: true, force: true }); + } + }); + test('`--fix --savings-only -` still writes the untouched style sheet to STDOUT when withholding', () => { const source = cssGrowing; const { stdout, stderr, status } = run(['--fix', '-s', '-'], { input: source }); diff --git a/test/dedup.test.js b/test/dedup.test.js index 290e5ca..9779739 100644 --- a/test/dedup.test.js +++ b/test/dedup.test.js @@ -3,7 +3,7 @@ import assert from 'node:assert'; import { analyze, dedup } from '../src/index.js'; import { normalizeValue } from '../src/lib/normalization.js'; import { selectorsLikelyDisjoint } from '../src/lib/selectors.js'; -import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive } from './helpers.js'; +import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssMixed } from './helpers.js'; describe('Deduplication', () => { test('Treats a `)` inside a `/* … */` comment as text when scanning a `min()` call', () => { @@ -757,4 +757,59 @@ describe('Savings only', () => { assert.strictEqual(output, css); assert.strictEqual(withheld.count, 1); }); + + test('Applies the merges that pay for themselves and declines only the ones that don\u2019t', () => { + const { css: output, applied, bytes, withheld } = dedup(cssMixed, { savingsOnly: true }); + + // The shrinking cluster merges… + assert.ok(applied.length > 0); + assert.match(output, /\.p,\s*\.q,\s*\.r\s*{\s*margin: 0;\s*}/); + // …while the growing one stays exactly as it was written + assert.match(output, /\.very-long-selector-name-one\s*{\s*color: red;\s*font-weight: bold;\s*}/); + assert.match(output, /\.b\s*{\s*color: red;\s*}/); + assert.doesNotMatch(output, /\.very-long-selector-name-one,/); + + assert.strictEqual(withheld.count, 1); + assert.ok(withheld.bytes.saved < 0, 'the declined merge is reported with what it would have cost'); + assert.ok(bytes.saved > 0, 'the run as a whole still shrinks the style sheet'); + assert.strictEqual(bytes.after, Buffer.byteLength(output, 'utf8')); + }); + + test('Never grows the style sheet, whichever mix of merges a file offers', () => { + const cases = [cssGrowing, cssGrowingAggressive, cssMixed, '.a { color: red; }\n.b { color: red; }\n']; + for (const aggressive of [false, true]) { + for (const css of cases) { + const { css: output, bytes } = dedup(css, { savingsOnly: true, aggressive }); + assert.ok( + Buffer.byteLength(output, 'utf8') <= Buffer.byteLength(css, 'utf8'), + `\`savingsOnly\` grew a style sheet (aggressive: ${aggressive})` + ); + assert.ok(bytes.saved >= 0); + } + } + }); + + test('Leaves a file untouched when every merge it offers would grow it', () => { + const { css: output, applied, bytes, withheld } = dedup(cssGrowing, { savingsOnly: true }); + assert.strictEqual(output, cssGrowing); + assert.strictEqual(applied.length, 0); + assert.strictEqual(bytes.saved, 0); + assert.strictEqual(withheld.count, 1); + }); + + test('Reports no `withheld` when every merge pays for itself', () => { + const css = '.a { color: red; top: 0; }\n.b { color: red; left: 0; }\n.c { color: red; right: 0; }\n'; + const { withheld, applied } = dedup(css, { savingsOnly: true }); + assert.ok(applied.length > 0); + assert.strictEqual(withheld, undefined); + }); + + test('Declining a merge leaves the merges around it byte-identical to an ungated run', () => { + // The gate must not perturb what it does not decline: The shrinking + // cluster has to come out exactly as it would on its own + const shrinkingAlone = '.p { margin: 0; padding: 0; }\n.q { margin: 0; top: 0; }\n.r { margin: 0; left: 0; }\n'; + const alone = dedup(shrinkingAlone).css; + const mixed = dedup(cssMixed, { savingsOnly: true }).css; + assert.ok(mixed.endsWith(alone.trimStart()) || mixed.includes(alone.trim())); + }); }); diff --git a/test/helpers.js b/test/helpers.js index 9ef5974..f7022a5 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -37,6 +37,19 @@ export const cssGrowingAggressive = [ '', ].join('\n'); +// One cluster that grows the file (the long selector list costs more than the +// removed `color` saves) and one that shrinks it (three short selectors share +// `margin`), in a single style sheet. Clusters are independent, so a per-merge +// gate applies the second and declines the first. +export const cssMixed = [ + '.very-long-selector-name-one { color: red; font-weight: bold; }', + '.b { color: red; }', + '.p { margin: 0; padding: 0; }', + '.q { margin: 0; top: 0; }', + '.r { margin: 0; left: 0; }', + '', +].join('\n'); + // Only mergeable in aggressive mode (canonicalizing the `` values is // aggressive-only), and—unlike `cssGrowingAggressive`—the merge shrinks the // file: Each rule holds only the one shared declaration, so folding them diff --git a/test/plugin.test.js b/test/plugin.test.js index e4895f8..07576ec 100644 --- a/test/plugin.test.js +++ b/test/plugin.test.js @@ -47,7 +47,7 @@ describe('Plugin: Dedup', () => { const input = '.very-long-selector-name-one { color: red; font-weight: bold; }\n.b { color: red; }\n'; const result = await postcss([cssdedup({ fix: true, savingsOnly: true })]).process(input, { from: undefined }); assert.strictEqual(result.css, input); - assert.ok(result.warnings().some(warning => /Consolidation withheld \(`savingsOnly`\): 1 merge would make the style sheet \d+ bytes bigger/.test(warning.text))); + assert.ok(result.warnings().some(warning => /1 merge withheld \(`savingsOnly`\): applying it would make the style sheet \d+ bytes bigger/.test(warning.text))); }); test('`savingsOnly: true` still applies a shrinking consolidation', async () => { From a5ab5eca4d114a9c0d6e3ad026d75b61f187acbf Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Mon, 24 Aug 2026 18:30:31 +0200 Subject: [PATCH 2/5] feat: improve merge handling and caching in savings-only mode Refactor `savings-only` implementation to assess merges individually, splitting entangled groups appropriately, and track byte outcomes efficiently using new cache mechanisms. Added logic to ensure accurate rollback and proper handling of speculative merges for nested and blocked structures. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert --- CHANGELOG.md | 2 +- README.md | 2 +- src/cli/file-pass.js | 27 ++---- src/consolidate.js | 10 +- src/lib/style.js | 7 ++ src/lib/transaction.js | 211 ++++++++++++++++++++++++++++++++++++++--- src/merge.js | 37 +++----- test/dedup.test.js | 36 ++++++- test/helpers.js | 20 ++++ 9 files changed, 289 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 667e64c..ccd4b69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed -* Changed `--savings-only` (`savingsOnly`) to decide per merge instead of per file, so a style sheet keeps the consolidations that save bytes even when others would lead to gains +* Changed `--savings-only` (`savingsOnly`) to decide per merge instead of per file, so a style sheet keeps the consolidations that save bytes even when others in it would cost more than those save - Adjusted `withheld: { count, bytes }` to report the merges that were declined and what applying them, too, would have cost, rather than the whole file’s consolidation ## [1.12.1] - 2026-08-13 diff --git a/README.md b/README.md index 463f0a8..de1e35e 100644 --- a/README.md +++ b/README.md @@ -227,7 +227,7 @@ CSS Dedup: - If a merged rule (including the last occurrence itself) also carries a declaration for an overlapping property, that declaration is split out into its own small rule—keeping that occurrence’s own, original selector—placed right after the merged rule, rather than blocking the merge outright: Folding every selector onto one shared declaration block would otherwise hand that overlapping extra to selectors that never had it. Exception: If that extra is itself duplicated elsewhere in the same scope, it’s left alone and the whole merge is skipped instead, since splitting it here would orphan that other duplicate’s own merge. - If something does block it, the merge is skipped and reported rather than risking a cascade change. A blocker fences, though—it doesn’t forbid: Occurrences on the same side of it still consolidate among themselves (their own spans are clean, so the same safety argument applies), and the group is reported as skipped either way, since the duplicate keeps existing across the blocker. - Consolidation runs to a fixed point: One merge can unblock or create another (a fresh merged rule may repeat an existing rule’s selector list, an emptied rule stops fencing a span), so the passes repeat until nothing changes. - - With `--savings-only`, each merge is weighed on its own before it stands: The consolidation is performed, the style sheet measured, and the merge undone again if it turned out to cost bytes. Entangled groups are weighed together, since they can only be applied together; the strategies that can’t cost bytes (collapsing a repeat within one rule, folding two same-selector rules) aren’t weighed at all. Measuring the real thing beats predicting it—a merge that empties a rule or a conditional block saves far more than its declaration arithmetic suggests. + - With `--savings-only`, each merge is weighed before it stands: The consolidation is performed, the style sheet measured, and the merge undone again if it turned out to cost bytes. The unit is the smallest one that can be applied alone—usually a single duplicate group, each run separately where a blocker splits one, and a whole cluster where groups share a rule. The strategies that can’t cost bytes (collapsing a repeat within one rule, folding two same-selector rules) aren’t weighed. Measuring the real thing beats predicting it—a merge that empties a rule or a conditional block saves far more than its declaration arithmetic suggests. Overall, CSS Dedup is conservative by design and will leave some safe merges for manual review. diff --git a/src/cli/file-pass.js b/src/cli/file-pass.js index fdd7804..a9c5969 100644 --- a/src/cli/file-pass.js +++ b/src/cli/file-pass.js @@ -54,33 +54,20 @@ function slimPass(pass) { return { bytes: pass.bytes, unavailable: pass.applied.length === 0 }; } -// Mirrors `dedupRoot()`’s `savingsOnly` gate against an already-computed plain -// pass, rather than running `dedup()` again just to reapply a rule that only -// looks at the first pass’s own `bytes.saved` -function applySavingsOnlyGate(pass) { - if (pass.bytes.saved >= 0) return pass; - return { - bytes: { before: pass.bytes.before, after: pass.bytes.before, saved: 0 }, - applied: [], - }; -} - // The report table’s four savings columns, in the order they’re rendered. // Named once here, where they’re produced, so the consumers in `report.js` // can’t drift out of sync with them. export const PASS_KEYS = ['passDefault', 'passDefaultS', 'passAgg', 'passAggS']; -// Only two of the four actually run `dedup()`; the `-s` variants are derived -// from those, since a second full pass would just reproduce the first one’s -// `bytes` before the gate looks at them. +// All four run `dedup()`. The `-s` variants can’t be derived from the ungated +// ones: The gate decides per merge, so a file mixing merges that save with +// merges that cost lands somewhere neither ungated figure predicts. function computeReportPasses(css, targetOptions) { - const passDefault = dedup(css, { ...targetOptions, aggressive: false, savingsOnly: false }); - const passAgg = dedup(css, { ...targetOptions, aggressive: true, savingsOnly: false }); return { - passDefault, - passDefaultS: applySavingsOnlyGate(passDefault), - passAgg, - passAggS: applySavingsOnlyGate(passAgg), + passDefault: dedup(css, { ...targetOptions, aggressive: false, savingsOnly: false }), + passDefaultS: dedup(css, { ...targetOptions, aggressive: false, savingsOnly: true }), + passAgg: dedup(css, { ...targetOptions, aggressive: true, savingsOnly: false }), + passAggS: dedup(css, { ...targetOptions, aggressive: true, savingsOnly: true }), }; } diff --git a/src/consolidate.js b/src/consolidate.js index 33568c8..9ef0831 100644 --- a/src/consolidate.js +++ b/src/consolidate.js @@ -16,6 +16,7 @@ import { ownSelectors, } from './lib/scopes.js'; import { usesMultilineSelectors, usesSpacedCommas } from './lib/style.js'; +import { resetByteCache } from './lib/transaction.js'; // Conditional group rules whose empty block is inert. `@layer` is deliberately // absent: a layer’s position in the layer order is set by its first @@ -61,8 +62,6 @@ function createContext(root, options, settle) { savingsOnly: options.savingsOnly ?? false, root, settle, - // Running size of the style sheet, carried between clusters (see `gated()`) - bytesNow: null, declined: [], }; } @@ -86,6 +85,10 @@ function runPass(root, ctx) { ctx.applied.push(...removeRedundantDuplicates(ctx, atrule, describeScope(atrule), [atRuleLabel(atrule)])); } for (const scope of scopes) foldSameSelectorRules(ctx, scope); + + // The phases above rewrite rules the gate would otherwise measure against a + // stale cache; from here on the gate maintains it itself + resetByteCache(root); for (const scope of scopes) mergeDuplicateGroups(ctx, scope); } @@ -126,9 +129,6 @@ function consolidateRoot(root, options = {}) { appliedCount = ctx.applied.length; ctx.skipped.length = 0; ctx.declined.length = 0; - // The phases before the gated merges change the style sheet, so the - // running size has to be taken afresh each pass - ctx.bytesNow = null; runPass(root, ctx); } diff --git a/src/lib/style.js b/src/lib/style.js index d8b22a5..aaf4d3e 100644 --- a/src/lib/style.js +++ b/src/lib/style.js @@ -18,6 +18,13 @@ export function resetSeparatorCache() { separatorCache = new WeakMap(); } +// A speculative merge that gets rolled back may already have had its +// separator computed—off a container holding residuals that no longer exist. +// Dropping the entry lets the next caller tally the restored tree. +export function forgetSeparator(container) { + separatorCache.delete(container); +} + // The gap most sibling nodes carry—the container’s prevailing “normal” // separation between rules. A majority vote, not just whichever neighbor is // handy, since that neighbor can be the anomaly. A gap straight after a diff --git a/src/lib/transaction.js b/src/lib/transaction.js index a63622d..3ae976e 100644 --- a/src/lib/transaction.js +++ b/src/lib/transaction.js @@ -4,6 +4,8 @@ // no merge strategy has to carry a second, parallel implementation of itself // in byte arithmetic, and the figure the gate decides on is the real one. +import { forgetSeparator } from './style.js'; + // Every container whose child list a merge can touch: the rules’ own parents, // plus their ancestors up to the root—an aggressive cross-block merge can // drain a conditional block, whose removal changes *its* parent’s child list. @@ -19,37 +21,220 @@ export function byteLength(root) { return Buffer.byteLength(root.toString(), 'utf8'); } +// Measuring a merge by re-serializing the whole style sheet costs the file’s +// size per cluster, which on a sheet made of many small duplicate groups turns +// the gate quadratic. A merge only ever changes the top-level subtrees its own +// rules sit in, though, so only those need measuring—everything else cancels +// between the before and after sums. +// +// Byte counts are cached per node and invalidated by re-measuring exactly the +// subtrees a merge touched. `raws.before` belongs to the node, so a sibling +// moving never changes a neighbor’s contribution. +let byteCache = new WeakMap(); +// Which nodes were already root children when the pass began, so a residual a +// merge inserts can be told apart from a rule that was always there +let knownRootChildren = new WeakSet(); + +export function resetByteCache(root) { + byteCache = new WeakMap(); + knownRootChildren = new WeakSet(); + for (const node of root.nodes) knownRootChildren.add(node); +} + +function textBytes(text) { + return text ? Buffer.byteLength(text, 'utf8') : 0; +} + +function measureNode(node) { + return Buffer.byteLength((node.raws.before ?? '') + node.toString(), 'utf8'); +} + +function cachedBytes(node) { + let bytes = byteCache.get(node); + if (bytes === undefined) { + bytes = measureNode(node); + byteCache.set(node, bytes); + } + return bytes; +} + +function remeasure(node) { + const bytes = measureNode(node); + byteCache.set(node, bytes); + return bytes; +} + +// The root-level ancestors of the rules a merge touches—the subtrees whose +// serialization it can change +function topLevelRegion(root, rules) { + const region = new Set(); + for (const rule of rules) { + let node = rule; + while (node.parent && node.parent !== root) node = node.parent; + if (node.parent === root) region.add(node); + } + return region; +} + +// The leading run of root children this merge could remove, plus the first one +// it could not—the only nodes whose leading whitespace PostCSS can rewrite +function captureFront(root, region) { + const front = new Map(); + if (!root.nodes.length || !region.has(root.nodes[0])) return front; + for (const node of root.nodes) { + front.set(node, node.raws.before); + if (!region.has(node)) break; + } + return front; +} + +// A node’s own mutable state plus its child list, recorded by identity rather +// than by cloning. Identity is the whole point: `runPass()` collects every +// scope up front, so a rule nested inside one this rollback touches is already +// spoken for by a later scope in the same pass. Hand that scope a clone and it +// would go on merging a subtree detached from the style sheet. +function captureNode(node) { + return { + node, + raws: { ...node.raws }, + selector: node.selector, + prop: node.prop, + value: node.value, + important: node.important, + name: node.name, + params: node.params, + children: node.nodes ? node.nodes.map(captureNode) : null, + }; +} + +function restoreNode(entry) { + const { node } = entry; + + // Scalars before `raws`: PostCSS drops a value’s raw spelling when the value + // itself is assigned, so restoring `raws` last is what puts it back + if (entry.selector !== undefined) node.selector = entry.selector; + if (entry.prop !== undefined) node.prop = entry.prop; + if (entry.value !== undefined) node.value = entry.value; + if (entry.important !== undefined) node.important = entry.important; + if (entry.name !== undefined) node.name = entry.name; + if (entry.params !== undefined) node.params = entry.params; + node.raws = { ...entry.raws }; + + if (!entry.children) return; + node.nodes = entry.children.map(child => child.node); + for (const child of node.nodes) child.parent = node; + for (const child of entry.children) restoreNode(child); +} + /** * Everything needed to put a scope back exactly as it stands right now. - * Cheap enough to take per cluster—the node clones are shallow copies of one - * cluster’s rules, not of the style sheet. + * Cheap enough to take per cluster—it walks one cluster’s rules, not the + * style sheet. */ -export function snapshot(scope, rules) { +export function snapshot(root, scope, rules) { + const region = topLevelRegion(root, rules); + // Per node, not just the sum: A rollback has to put these back into the + // cache, or the speculative measurement would outlive the tree it described + const regionBefore = new Map(); + let regionBytes = 0; + for (const node of region) { + const bytes = cachedBytes(node); + regionBefore.set(node, bytes); + regionBytes += bytes; + } + return { + root, + region, + regionBefore, + regionBytes, + rootLength: root.nodes.length, + // PostCSS hands a removed first child's `raws.before` to its successor, so + // a node this merge never touched can still change size. Only a removal at + // the very front can do that, and only region subtrees are ever removed—so + // recording the leading run of them, plus the one node that could be + // promoted past it, is enough to measure and undo. + frontBefores: captureFront(root, region), // Residual rules are appended here as merges create them, so the array // itself is state a rollback has to restore scopeRules: scope.rules.slice(), containers: affectedContainers(rules).map(container => ({ container, nodes: container.nodes.slice() })), - rules: rules.map(rule => ({ rule, clone: rule.clone(), parent: rule.parent })), + rules: rules.map(rule => ({ entry: captureNode(rule), parent: rule.parent })), }; } +/** + * What the merge just performed cost, in bytes of the whole style sheet. + * Negative means it paid for itself. + */ +export function costSince(snap) { + const { root, region, frontBefores } = snap; + let after = 0; + let detached = 0; + + // Only region subtrees can have changed, so they are the only ones worth + // re-measuring; a removed one contributes nothing and its old size stays + // counted in `regionBytes` + for (const node of region) { + if (node.parent === root) after += remeasure(node); + else detached++; + } + + // Anything else at root level is a residual this merge created. Counting + // gives that away without a scan: only when the arithmetic doesn’t add up is + // there something new to look for. + snap.inserted = null; + if (root.nodes.length !== snap.rootLength - detached) { + snap.inserted = []; + for (const node of root.nodes) { + if (region.has(node) || knownRootChildren.has(node)) continue; + after += remeasure(node); + knownRootChildren.add(node); + snap.inserted.push(node); + } + } + + // A node whose leading whitespace PostCSS rewrote when it promoted a new + // first child. Region nodes are excluded: their re-measurement above already + // reflects the new spacing. + let respaced = 0; + for (const [node, before] of frontBefores) { + if (region.has(node) || node.parent !== root || before === node.raws.before) continue; + respaced += textBytes(node.raws.before) - textBytes(before); + byteCache.delete(node); + } + + return after - snap.regionBytes + respaced; +} + // Restores the child lists first (which un-removes emptied rules and drops -// inserted residuals), then each rule’s own contents. The rule objects are the -// same identities throughout, so references held elsewhere—`scope.rules`, a -// cluster’s `distinctRules`—stay live across a rollback. +// inserted residuals), then each rule’s own contents. Every node object is the +// one that was there before, so references held elsewhere—`scope.rules`, a +// cluster’s `distinctRules`, a nested scope collected earlier this pass—stay +// live across a rollback. export function rollback(snap, scope) { + // The speculative pass remeasured these; put the real figures back + for (const [node, bytes] of snap.regionBefore) byteCache.set(node, bytes); + // Residuals it inserted are about to be dropped—they were never really here + if (snap.inserted) for (const node of snap.inserted) knownRootChildren.delete(node); + + // …and undo any re-spacing PostCSS did when it promoted a new first child + for (const [node, before] of snap.frontBefores) { + if (node.raws.before !== before) { + node.raws.before = before; + byteCache.delete(node); + } + } + for (const { container, nodes } of snap.containers) { container.nodes = nodes; for (const node of nodes) node.parent = container; + forgetSeparator(container); } - for (const { rule, clone, parent } of snap.rules) { - rule.selector = clone.selector; - rule.raws = { ...clone.raws }; - rule.removeAll(); - rule.append(clone.nodes.map(node => node.clone())); - rule.parent = parent; + for (const { entry, parent } of snap.rules) { + restoreNode(entry); + entry.node.parent = parent; } scope.rules.length = 0; diff --git a/src/merge.js b/src/merge.js index 0bdb9ab..d6771ba 100644 --- a/src/merge.js +++ b/src/merge.js @@ -11,7 +11,7 @@ import { splitSelectors, selectorsAreMutuallyExclusive, selectorsLikelyDisjoint import { propertiesOverlap } from './lib/shorthands.js'; import { insertAfter, joinSelectors, typicalSeparator } from './lib/style.js'; import { declsOf, pushTo } from './lib/util.js'; -import { byteLength, rollback, snapshot } from './lib/transaction.js'; +import { costSince, rollback, snapshot } from './lib/transaction.js'; // All occurrences of a key are equivalent by our own normalization rules, so // the merge keeps whichever raw spelling is shortest rather than whatever the @@ -306,15 +306,17 @@ export function mergePartialGroup(ctx, scope, group, reason) { runs.at(-1).push(distinctRules[i]); } + // A blocking rule separates the runs, so each touches a disjoint set of + // rules and carries its own byte outcome—`savingsOnly` weighs them one by one for (const runRules of runs) { if (runRules.length < 2) continue; const runSet = new Set(runRules); - mergeSoloGroup(ctx, scope, { + gated(ctx, scope, [key], runRules, () => mergeSoloGroup(ctx, scope, { key, occurrences: occurrences.filter(occ => runSet.has(occ.rule)), distinctRules: runRules, propNormalized, - }); + })); } // Whatever merged above resurfaces as a smaller group on the next @@ -641,7 +643,7 @@ function clusterGroups(groups) { // Only declaration merges are gated. The other two strategies don’t cost // bytes: Collapsing a repeat within one rule removes text and adds none, and // folding two same-selector rules removes a whole selector and its braces. -function gated(ctx, scope, cluster, apply) { +function gated(ctx, scope, keys, rules, apply) { if (!ctx.savingsOnly) { apply(); return; @@ -653,30 +655,20 @@ function gated(ctx, scope, cluster, apply) { // first cluster of a pass—the phases before it are the ones that can leave a // block behind. ctx.settle(); - // The style sheet only changes here between clusters, so the size measured - // after one accepted merge is the size the next one starts from: one - // stringify per cluster rather than a fresh before-and-after pair. Reset to - // `null` at the start of every pass, where the earlier phases have run. - ctx.bytesNow ??= byteLength(ctx.root); - - const rules = [...new Set(cluster.flatMap(group => group.distinctRules))]; - const snap = snapshot(scope, rules); + + const snap = snapshot(ctx.root, scope, [...new Set(rules)]); const appliedBefore = ctx.applied.length; apply(); ctx.settle(); - const after = byteLength(ctx.root); - const cost = after - ctx.bytesNow; - if (cost <= 0) { - ctx.bytesNow = after; - return; - } + const cost = costSince(snap); + if (cost <= 0) return; rollback(snap, scope); // `skipped` is left as it stands—whatever the safety checks concluded about // this cluster is still true, and still worth reporting - ctx.declined.push({ scope: scope.label, keys: cluster.map(group => group.key), count: ctx.applied.length - appliedBefore, cost }); + ctx.declined.push({ scope: scope.label, keys, count: ctx.applied.length - appliedBefore, cost }); ctx.applied.length = appliedBefore; } @@ -709,7 +701,7 @@ export function mergeDuplicateGroups(ctx, scope) { } if (!outsideBlocker) { - gated(ctx, scope, cluster, () => { + gated(ctx, scope, cluster.map(group => group.key), [...clusterRules], () => { if (cluster.length === 1) mergeSoloGroup(ctx, scope, cluster[0]); else mergeCluster(ctx, scope, cluster); }); @@ -721,11 +713,12 @@ export function mergeDuplicateGroups(ctx, scope) { const reason = `intervening ${propDescription} declaration in \`${blocking.rule.selector}\` (line ${blocking.rule.source?.start?.line})`; if (cluster.length === 1) { - gated(ctx, scope, cluster, () => mergePartialGroup(ctx, scope, group, reason)); + mergePartialGroup(ctx, scope, group, reason); continue; } - gated(ctx, scope, cluster, () => { + // Entangled members share rules, so their runs can only be weighed as one + gated(ctx, scope, cluster.map(member => member.key), [...clusterRules], () => { for (const member of cluster) mergeClusterGroupRuns(ctx, scope, member); }); diff --git a/test/dedup.test.js b/test/dedup.test.js index 9779739..8262ac1 100644 --- a/test/dedup.test.js +++ b/test/dedup.test.js @@ -3,7 +3,7 @@ import assert from 'node:assert'; import { analyze, dedup } from '../src/index.js'; import { normalizeValue } from '../src/lib/normalization.js'; import { selectorsLikelyDisjoint } from '../src/lib/selectors.js'; -import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssMixed } from './helpers.js'; +import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssMixed, cssNestedHost } from './helpers.js'; describe('Deduplication', () => { test('Treats a `)` inside a `/* … */` comment as text when scanning a `min()` call', () => { @@ -804,6 +804,40 @@ describe('Savings only', () => { assert.strictEqual(withheld, undefined); }); + test('Declining a merge on a nesting host still merges the rules nested inside it', () => { + // The inner scope is collected before the outer merge runs, so a rollback + // that replaced `.a`’s children with copies would leave that scope + // merging a subtree no longer attached to the style sheet + const { css: output, applied, bytes } = dedup(cssNestedHost, { savingsOnly: true }); + + assert.match(output, /&:hover,\s*&:focus\s*{\s*top: 0;\s*}/); + assert.match(output, /\.very-long-selector-name-one\s*{\s*color: red;/); + assert.doesNotMatch(output, /\.very-long-selector-name-one,/); + assert.strictEqual(applied.length, 1); + assert.strictEqual(bytes.saved, Buffer.byteLength(cssNestedHost, 'utf8') - Buffer.byteLength(output, 'utf8')); + assert.ok(bytes.saved > 0); + }); + + test('Weighs each safe run of a blocked group on its own', () => { + // `background` is blocked in the middle, leaving two independent runs: a + // shrinking one (short selectors) and a growing one (long selectors). + // Gating them together would let either drag the other along. + const css = [ + '.s1 { background: red; }', + '.s2 { background: red; }', + '.mid { background: blue; }', + '.a-very-long-selector-name-here { background: red; color: #fff; }', + '.another-very-long-selector-name { background: red; color: #000; }', + '', + ].join('\n'); + + const { css: output, bytes, withheld } = dedup(css, { savingsOnly: true }); + assert.match(output, /\.s1,\s*\.s2\s*{\s*background: red;\s*}/); + assert.doesNotMatch(output, /\.a-very-long-selector-name-here,/); + assert.ok(bytes.saved > 0); + assert.strictEqual(withheld.count, 1); + }); + test('Declining a merge leaves the merges around it byte-identical to an ungated run', () => { // The gate must not perturb what it does not decline: The shrinking // cluster has to come out exactly as it would on its own diff --git a/test/helpers.js b/test/helpers.js index f7022a5..7c300eb 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -50,6 +50,26 @@ export const cssMixed = [ '', ].join('\n'); +// A growing merge on a *nesting host*, plus a shrinking one among the rules +// nested inside it. Declining the outer merge must leave the nested rules the +// inner scope already holds references to intact, not swap in copies. +export const cssNestedHost = [ + '.very-long-selector-name-one {', + ' color: red;', + '', + ' &:hover {', + ' top: 0;', + ' }', + '', + ' &:focus {', + ' top: 0;', + ' }', + '}', + '', + '.b { color: red; }', + '', +].join('\n'); + // Only mergeable in aggressive mode (canonicalizing the `` values is // aggressive-only), and—unlike `cssGrowingAggressive`—the merge shrinks the // file: Each rule holds only the one shared declaration, so folding them From 8d4bb8f30790a5bd736c995df1ef49f7364efed0 Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Mon, 24 Aug 2026 18:59:59 +0200 Subject: [PATCH 3/5] fix: update cluster consolidation test assertion Adjusted the test assertion for cluster consolidation to remove unnecessary conditions and improve clarity. This ensures the test properly verifies the expected behavior without ambiguity. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert --- test/dedup.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/dedup.test.js b/test/dedup.test.js index 8262ac1..e15a2ad 100644 --- a/test/dedup.test.js +++ b/test/dedup.test.js @@ -844,6 +844,6 @@ describe('Savings only', () => { const shrinkingAlone = '.p { margin: 0; padding: 0; }\n.q { margin: 0; top: 0; }\n.r { margin: 0; left: 0; }\n'; const alone = dedup(shrinkingAlone).css; const mixed = dedup(cssMixed, { savingsOnly: true }).css; - assert.ok(mixed.endsWith(alone.trimStart()) || mixed.includes(alone.trim())); + assert.ok(mixed.includes(alone.trim()), `the shrinking cluster should consolidate exactly as it does on its own, but got:\n${mixed}`); }); }); From fb648058d641343d86cddd83614895275575c89c Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Mon, 24 Aug 2026 19:20:34 +0200 Subject: [PATCH 4/5] feat: improve merge tracking and rollback efficiency Enhanced merge tracking by introducing insertion recording, ensuring accurate rollback and efficient byte measurements. Updated tests to validate entangled group handling and ensure the accuracy of savings-only behavior. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert --- src/consolidate.js | 13 ++- src/lib/transaction.js | 195 +++++++++++++++++++++++------------------ src/merge.js | 9 +- test/dedup.test.js | 27 +++++- test/helpers.js | 22 +++++ 5 files changed, 172 insertions(+), 94 deletions(-) diff --git a/src/consolidate.js b/src/consolidate.js index 9ef0831..d6f983b 100644 --- a/src/consolidate.js +++ b/src/consolidate.js @@ -36,9 +36,16 @@ function removeEmptiedConditionBlocks(root, initiallyEmpty) { if (INERT_WHEN_EMPTY_ATRULES.has(atrule.name.toLowerCase())) candidates.push(atrule); }); + const removed = []; for (const atrule of candidates.reverse()) { - if (atrule.nodes && !atrule.nodes.length && !initiallyEmpty.has(atrule)) atrule.remove(); + if (atrule.nodes && !atrule.nodes.length && !initiallyEmpty.has(atrule)) { + // Measured before it goes: standing empty, all that’s left of it is the + // wrapper the `savingsOnly` gate needs to price + removed.push(atrule); + atrule.remove(); + } } + return removed; } // Everything the merge strategies need from this run. The normalization mode @@ -113,9 +120,7 @@ function consolidateRoot(root, options = {}) { // Bringing the style sheet to the state it would ship in. Idempotent, so the // gate can call it around every merge it weighs and the run can call it once // more at the end. - const settle = () => { - if (aggressive) removeEmptiedConditionBlocks(root, initiallyEmpty); - }; + const settle = () => (aggressive ? removeEmptiedConditionBlocks(root, initiallyEmpty) : []); const ctx = createContext(root, options, settle); // One merge can unblock or create another: a fresh merged rule may twin with diff --git a/src/lib/transaction.js b/src/lib/transaction.js index 3ae976e..3bc484c 100644 --- a/src/lib/transaction.js +++ b/src/lib/transaction.js @@ -17,36 +17,36 @@ function affectedContainers(rules) { return [...containers]; } -export function byteLength(root) { - return Buffer.byteLength(root.toString(), 'utf8'); -} - -// Measuring a merge by re-serializing the whole style sheet costs the file’s -// size per cluster, which on a sheet made of many small duplicate groups turns -// the gate quadratic. A merge only ever changes the top-level subtrees its own -// rules sit in, though, so only those need measuring—everything else cancels -// between the before and after sums. -// -// Byte counts are cached per node and invalidated by re-measuring exactly the -// subtrees a merge touched. `raws.before` belongs to the node, so a sibling -// moving never changes a neighbor’s contribution. +// A merge is priced from the few rules it actually rewrites, +// and everything around them cancels out untouched let byteCache = new WeakMap(); -// Which nodes were already root children when the pass began, so a residual a -// merge inserts can be told apart from a rule that was always there -let knownRootChildren = new WeakSet(); +// Nodes already in the tree when the pass began, so a rule a merge inserts can +// be told from one that was always there +let knownNodes = new WeakSet(); +// Where the merge strategies report the rules they insert. Finding those any +// other way means scanning a container, which is the cost this exists to +// avoid—but the accounting below still reconciles against the container’s +// child count, so a strategy that forgets to report is slow, never wrong. +let insertions = null; export function resetByteCache(root) { byteCache = new WeakMap(); - knownRootChildren = new WeakSet(); - for (const node of root.nodes) knownRootChildren.add(node); + knownNodes = new WeakSet(); + root.walk(node => knownNodes.add(node)); +} + +export function recordInsertion(node) { + if (insertions) insertions.push(node); } function textBytes(text) { return text ? Buffer.byteLength(text, 'utf8') : 0; } +// `toString()` does not include a node’s own leading whitespace, so it counts +// separately—the two together are exactly what the node adds to the output function measureNode(node) { - return Buffer.byteLength((node.raws.before ?? '') + node.toString(), 'utf8'); + return textBytes(node.raws.before) + Buffer.byteLength(node.toString(), 'utf8'); } function cachedBytes(node) { @@ -64,26 +64,13 @@ function remeasure(node) { return bytes; } -// The root-level ancestors of the rules a merge touches—the subtrees whose -// serialization it can change -function topLevelRegion(root, rules) { - const region = new Set(); - for (const rule of rules) { - let node = rule; - while (node.parent && node.parent !== root) node = node.parent; - if (node.parent === root) region.add(node); - } - return region; -} - -// The leading run of root children this merge could remove, plus the first one -// it could not—the only nodes whose leading whitespace PostCSS can rewrite -function captureFront(root, region) { +// The leading root children whose spacing a removal can rewrite: the run that +// could be removed, plus the first node that would outlive it +function captureFront(root) { const front = new Map(); - if (!root.nodes.length || !region.has(root.nodes[0])) return front; for (const node of root.nodes) { front.set(node, node.raws.before); - if (!region.has(node)) break; + if (front.size > 1) break; } return front; } @@ -127,34 +114,44 @@ function restoreNode(entry) { } /** - * Everything needed to put a scope back exactly as it stands right now. - * Cheap enough to take per cluster—it walks one cluster’s rules, not the - * style sheet. + * Everything needed to put a scope back exactly as it stands right now, and to + * price what happens in between. Cheap enough to take per cluster—it walks one + * cluster’s rules, not the style sheet. */ export function snapshot(root, scope, rules) { - const region = topLevelRegion(root, rules); - // Per node, not just the sum: A rollback has to put these back into the + // Per rule, not just the sum: a rollback has to put these back into the // cache, or the speculative measurement would outlive the tree it described - const regionBefore = new Map(); - let regionBytes = 0; - for (const node of region) { - const bytes = cachedBytes(node); - regionBefore.set(node, bytes); - regionBytes += bytes; + const ruleBefore = new Map(); + let ruleBytes = 0; + for (const rule of rules) { + const bytes = cachedBytes(rule); + ruleBefore.set(rule, bytes); + ruleBytes += bytes; } + // Where each rule started, and the child counts of those containers, so the + // reconciliation below can tell a genuine gap from an expected one + const ruleParent = new Map(); + const counts = new Map(); + for (const rule of rules) { + if (!rule.parent) continue; + ruleParent.set(rule, rule.parent); + counts.set(rule.parent, rule.parent.nodes.length); + } + + insertions = []; + return { root, - region, - regionBefore, - regionBytes, - rootLength: root.nodes.length, - // PostCSS hands a removed first child's `raws.before` to its successor, so - // a node this merge never touched can still change size. Only a removal at - // the very front can do that, and only region subtrees are ever removed—so - // recording the leading run of them, plus the one node that could be - // promoted past it, is enough to measure and undo. - frontBefores: captureFront(root, region), + ruleBefore, + ruleBytes, + ruleParent, + counts, + // PostCSS hands a removed first child’s `raws.before` to its successor, so + // a node this merge never touched can still change size. That override + // lives on `Root` alone—a nested container just splices—so only the root’s + // own leading children need recording. + frontBefores: captureFront(root), // Residual rules are appended here as merges create them, so the array // itself is state a rollback has to restore scopeRules: scope.rules.slice(), @@ -163,48 +160,76 @@ export function snapshot(root, scope, rules) { }; } +// Rules the merge reported inserting, plus—only where a container’s child +// count disagrees with what those reports account for—whatever else turned up +// in it. The scan is the safety net: It makes an unreported insertion cost a +// container walk rather than a wrong answer. +function insertedNodes(snap) { + const found = new Set(); + const perContainer = new Map(); + + for (const node of insertions) { + if (!node.parent || knownNodes.has(node) || found.has(node)) continue; + found.add(node); + perContainer.set(node.parent, (perContainer.get(node.parent) ?? 0) + 1); + } + + for (const [container, before] of snap.counts) { + // A container the merge removed outright is priced as a whole, not by its + // children + if (!container.parent && container !== snap.root) continue; + + let gone = 0; + for (const [rule, parent] of snap.ruleParent) { + if (parent === container && rule.parent !== container) gone++; + } + if (container.nodes.length === before - gone + (perContainer.get(container) ?? 0)) continue; + + for (const node of container.nodes) { + if (!knownNodes.has(node)) found.add(node); + } + } + + return [...found]; +} + /** * What the merge just performed cost, in bytes of the whole style sheet. * Negative means it paid for itself. */ -export function costSince(snap) { - const { root, region, frontBefores } = snap; +export function costSince(snap, removedContainers) { + const { root, ruleBefore, frontBefores } = snap; let after = 0; - let detached = 0; - - // Only region subtrees can have changed, so they are the only ones worth - // re-measuring; a removed one contributes nothing and its old size stays - // counted in `regionBytes` - for (const node of region) { - if (node.parent === root) after += remeasure(node); - else detached++; + + // A rule the merge removed contributes nothing, and its old size stays + // counted in `ruleBytes` + for (const rule of ruleBefore.keys()) { + if (rule.parent) after += remeasure(rule); } - // Anything else at root level is a residual this merge created. Counting - // gives that away without a scan: only when the arithmetic doesn’t add up is - // there something new to look for. - snap.inserted = null; - if (root.nodes.length !== snap.rootLength - detached) { - snap.inserted = []; - for (const node of root.nodes) { - if (region.has(node) || knownRootChildren.has(node)) continue; - after += remeasure(node); - knownRootChildren.add(node); - snap.inserted.push(node); - } + snap.inserted = insertedNodes(snap); + for (const node of snap.inserted) { + after += remeasure(node); + knownNodes.add(node); } + insertions = null; + + // A conditional block `settle()` cleared away. Its rules are already + // accounted for above; what goes with it is the block’s own wrapper, which + // is all that is left to measure now that it stands empty. + let wrappers = 0; + for (const container of removedContainers) wrappers += measureNode(container); // A node whose leading whitespace PostCSS rewrote when it promoted a new - // first child. Region nodes are excluded: their re-measurement above already - // reflects the new spacing. + // first child, and that nothing above has re-measured let respaced = 0; for (const [node, before] of frontBefores) { - if (region.has(node) || node.parent !== root || before === node.raws.before) continue; + if (ruleBefore.has(node) || node.parent !== root || before === node.raws.before) continue; respaced += textBytes(node.raws.before) - textBytes(before); byteCache.delete(node); } - return after - snap.regionBytes + respaced; + return after - snap.ruleBytes - wrappers + respaced; } // Restores the child lists first (which un-removes emptied rules and drops @@ -214,9 +239,9 @@ export function costSince(snap) { // live across a rollback. export function rollback(snap, scope) { // The speculative pass remeasured these; put the real figures back - for (const [node, bytes] of snap.regionBefore) byteCache.set(node, bytes); + for (const [node, bytes] of snap.ruleBefore) byteCache.set(node, bytes); // Residuals it inserted are about to be dropped—they were never really here - if (snap.inserted) for (const node of snap.inserted) knownRootChildren.delete(node); + if (snap.inserted) for (const node of snap.inserted) knownNodes.delete(node); // …and undo any re-spacing PostCSS did when it promoted a new first child for (const [node, before] of snap.frontBefores) { diff --git a/src/merge.js b/src/merge.js index d6771ba..1002f64 100644 --- a/src/merge.js +++ b/src/merge.js @@ -11,7 +11,7 @@ import { splitSelectors, selectorsAreMutuallyExclusive, selectorsLikelyDisjoint import { propertiesOverlap } from './lib/shorthands.js'; import { insertAfter, joinSelectors, typicalSeparator } from './lib/style.js'; import { declsOf, pushTo } from './lib/util.js'; -import { costSince, rollback, snapshot } from './lib/transaction.js'; +import { costSince, recordInsertion, rollback, snapshot } from './lib/transaction.js'; // All occurrences of a key are equivalent by our own normalization rules, so // the merge keeps whichever raw spelling is shortest rather than whatever the @@ -263,6 +263,7 @@ export function mergeSoloGroup(ctx, scope, group) { const residual = makeResidual(target, rule === target ? targetOriginalSelector : rule.selector, extras); insertAfter(insertPoint, residual, interPieceSeparator); + recordInsertion(residual); insertPoint = residual; afterResiduals.push(residual); } @@ -434,6 +435,7 @@ function mergeClusterGroupRuns(ctx, scope, group) { if (lastDecl.value !== value) lastDecl.value = value; mergedRule.append(lastDecl); lastRule.after(mergedRule); + recordInsertion(mergedRule); scope.rules.splice(scope.rules.indexOf(lastRule) + 1, 0, mergedRule); // The clone inherited `lastRule`’s own `raws.before`—right if `lastRule` @@ -540,6 +542,7 @@ function splitStarHub(ctx, scope, cluster, hub) { hub.before(finalRules[0]); for (let i = 1; i < finalRules.length; i++) { insertAfter(finalRules[i - 1], finalRules[i], interPieceSeparator); + recordInsertion(finalRules[i]); } hub.remove(); @@ -660,9 +663,7 @@ function gated(ctx, scope, keys, rules, apply) { const appliedBefore = ctx.applied.length; apply(); - ctx.settle(); - - const cost = costSince(snap); + const cost = costSince(snap, ctx.settle()); if (cost <= 0) return; rollback(snap, scope); diff --git a/test/dedup.test.js b/test/dedup.test.js index e15a2ad..ca45601 100644 --- a/test/dedup.test.js +++ b/test/dedup.test.js @@ -3,7 +3,7 @@ import assert from 'node:assert'; import { analyze, dedup } from '../src/index.js'; import { normalizeValue } from '../src/lib/normalization.js'; import { selectorsLikelyDisjoint } from '../src/lib/selectors.js'; -import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssMixed, cssNestedHost } from './helpers.js'; +import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssEntangledGrowing, cssEntangledShrinking, cssMixed, cssNestedHost } from './helpers.js'; describe('Deduplication', () => { test('Treats a `)` inside a `/* … */` comment as text when scanning a `min()` call', () => { @@ -838,6 +838,31 @@ describe('Savings only', () => { assert.strictEqual(withheld.count, 1); }); + test('Declines an entangled cluster as a whole, leaving the style sheet untouched', () => { + // Its groups all share the hub rule, so they merge as one coordinated + // whole or not at all—and that whole grows the file + const ungated = dedup(cssEntangledGrowing); + assert.ok(ungated.bytes.saved < 0, 'fixture should grow the file when ungated'); + + const { css: output, applied, skipped, bytes, withheld } = dedup(cssEntangledGrowing, { savingsOnly: true }); + assert.strictEqual(output, cssEntangledGrowing); + assert.strictEqual(applied.length, 0); + assert.strictEqual(skipped.length, 0); + assert.strictEqual(bytes.saved, 0); + assert.strictEqual(withheld.count, ungated.applied.length); + assert.strictEqual(withheld.bytes.saved, ungated.bytes.saved); + }); + + test('Applies an entangled cluster that pays for itself, exactly as an ungated run does', () => { + const ungated = dedup(cssEntangledShrinking); + assert.ok(ungated.bytes.saved > 0); + + const gated = dedup(cssEntangledShrinking, { savingsOnly: true }); + assert.strictEqual(gated.css, ungated.css); + assert.strictEqual(gated.applied.length, ungated.applied.length); + assert.strictEqual(gated.withheld, undefined); + }); + test('Declining a merge leaves the merges around it byte-identical to an ungated run', () => { // The gate must not perturb what it does not decline: The shrinking // cluster has to come out exactly as it would on its own diff --git a/test/helpers.js b/test/helpers.js index 7c300eb..742cc72 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -70,6 +70,28 @@ export const cssNestedHost = [ '', ].join('\n'); +// An entangled cluster: Three duplicate groups all sharing the hub rule, so +// they can only be merged as one coordinated whole. The selectors are long +// enough that the whole thing costs bytes, so `savingsOnly` must decline it +// together—and its identity-preserving rollback has to survive the several +// replacement rules the split creates. +export const cssEntangledGrowing = [ + '.hub-selector-that-is-long-here { color: red; top: 0; left: 0; }', + '.spoke-selector-number-one-here { color: red; }', + '.spoke-selector-number-two-here { top: 0; }', + '.spoke-selector-number-six-here { left: 0; }', + '', +].join('\n'); + +// The same shape where the merge pays off: Identical rules fold whole, so the +// coordinated merge removes rules instead of lengthening selector lists +export const cssEntangledShrinking = [ + '.a-really-quite-long-selector-one { color: red; top: 0; }', + '.a-really-quite-long-selector-two { color: red; top: 0; }', + '.a-really-quite-long-selector-six { color: red; top: 0; }', + '', +].join('\n'); + // Only mergeable in aggressive mode (canonicalizing the `` values is // aggressive-only), and—unlike `cssGrowingAggressive`—the merge shrinks the // file: Each rule holds only the one shared declaration, so folding them From b20e5ecfec68f27f6975550025930074ab24588e Mon Sep 17 00:00:00 2001 From: Jens Oliver Meiert Date: Mon, 24 Aug 2026 20:07:32 +0200 Subject: [PATCH 5/5] feat: enhance merge pricing and rollback accuracy Introduced byte measurement tracking for precise merge pricing and rollback scenarios. Improved handling of empty and removable root-level rules, consolidated condition block collection, and added extensive testing for savings-only behavior and edge cases. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert --- src/consolidate.js | 31 ++++++++++++++--------- src/lib/transaction.js | 35 +++++++++++++++++++++----- src/merge.js | 2 ++ test/dedup.test.js | 57 +++++++++++++++++++++++++++++++++++++++++- test/helpers.js | 13 ++++++++++ 5 files changed, 119 insertions(+), 19 deletions(-) diff --git a/src/consolidate.js b/src/consolidate.js index d6f983b..b6ec3af 100644 --- a/src/consolidate.js +++ b/src/consolidate.js @@ -24,20 +24,25 @@ import { resetByteCache } from './lib/transaction.js'; // the cascade. const INERT_WHEN_EMPTY_ATRULES = new Set(['media', 'supports', 'container']); -// Aggressive mode’s cross-block merges can drain the earlier of two -// same-condition blocks completely. This removes such blocks—only ones this -// run emptied, and only where emptiness is provably inert. The walk collects -// candidates parents-first, so the reverse pass sees each inner block before -// its parent and a parent emptied by its child’s removal is caught in the same -// sweep. -function removeEmptiedConditionBlocks(root, initiallyEmpty) { +// Every block aggressive mode could later find emptied, innermost first—so one +// sweep sees each inner block before its parent, and a parent emptied by its +// child’s removal is caught in the same pass. Collected once per run. +function collectConditionBlocks(root) { const candidates = []; root.walkAtRules(atrule => { if (INERT_WHEN_EMPTY_ATRULES.has(atrule.name.toLowerCase())) candidates.push(atrule); }); + return candidates.reverse(); +} +// Aggressive mode’s cross-block merges can drain the earlier of two +// same-condition blocks completely. This removes such blocks—only ones this +// run emptied, and only where emptiness is provably inert. +function removeEmptiedConditionBlocks(candidates, initiallyEmpty) { const removed = []; - for (const atrule of candidates.reverse()) { + for (const atrule of candidates) { + // Gone already, in this sweep or an earlier one + if (!atrule.parent) continue; if (atrule.nodes && !atrule.nodes.length && !initiallyEmpty.has(atrule)) { // Measured before it goes: standing empty, all that’s left of it is the // wrapper the `savingsOnly` gate needs to price @@ -111,16 +116,18 @@ function consolidateRoot(root, options = {}) { // removes what this run emptied const initiallyEmpty = new Set(); const aggressive = options.aggressive ?? false; + const conditionBlocks = aggressive ? collectConditionBlocks(root) : []; if (aggressive) { - root.walkAtRules(atrule => { + for (const atrule of conditionBlocks) { if (atrule.nodes && !atrule.nodes.length) initiallyEmpty.add(atrule); - }); + } } // Bringing the style sheet to the state it would ship in. Idempotent, so the // gate can call it around every merge it weighs and the run can call it once - // more at the end. - const settle = () => (aggressive ? removeEmptiedConditionBlocks(root, initiallyEmpty) : []); + // more at the end—which is why the candidate list is gathered once rather + // than re-walked on each of those calls. + const settle = () => (aggressive ? removeEmptiedConditionBlocks(conditionBlocks, initiallyEmpty) : []); const ctx = createContext(root, options, settle); // One merge can unblock or create another: a fresh merged rule may twin with diff --git a/src/lib/transaction.js b/src/lib/transaction.js index 3bc484c..28d487d 100644 --- a/src/lib/transaction.js +++ b/src/lib/transaction.js @@ -39,6 +39,14 @@ export function recordInsertion(node) { if (insertions) insertions.push(node); } +// How much text the gate has serialized to price merges; +// exported for the scaling test +let measuredBytes = 0; + +export function measuredByteTotal() { + return measuredBytes; +} + function textBytes(text) { return text ? Buffer.byteLength(text, 'utf8') : 0; } @@ -46,7 +54,9 @@ function textBytes(text) { // `toString()` does not include a node’s own leading whitespace, so it counts // separately—the two together are exactly what the node adds to the output function measureNode(node) { - return textBytes(node.raws.before) + Buffer.byteLength(node.toString(), 'utf8'); + const bytes = textBytes(node.raws.before) + Buffer.byteLength(node.toString(), 'utf8'); + measuredBytes += bytes; + return bytes; } function cachedBytes(node) { @@ -64,13 +74,26 @@ function remeasure(node) { return bytes; } -// The leading root children whose spacing a removal can rewrite: the run that -// could be removed, plus the first node that would outlive it -function captureFront(root) { +// The root-level subtrees this merge could remove: its rules, or the blocks +// holding them, which `settle()` clears away once drained +function removableAtRoot(root, rules) { + const removable = new Set(); + for (const rule of rules) { + let node = rule; + while (node.parent && node.parent !== root) node = node.parent; + if (node.parent === root) removable.add(node); + } + return removable; +} + +// The leading root children whose spacing a removal can rewrite. PostCSS hands +// each removed first child’s `raws.before` down the line, so a run of removals +// at the front reaches the first child that outlives them—no further. +function captureFront(root, removable) { const front = new Map(); for (const node of root.nodes) { front.set(node, node.raws.before); - if (front.size > 1) break; + if (!removable.has(node)) break; } return front; } @@ -151,7 +174,7 @@ export function snapshot(root, scope, rules) { // a node this merge never touched can still change size. That override // lives on `Root` alone—a nested container just splices—so only the root’s // own leading children need recording. - frontBefores: captureFront(root), + frontBefores: captureFront(root, removableAtRoot(root, rules)), // Residual rules are appended here as merges create them, so the array // itself is state a rollback has to restore scopeRules: scope.rules.slice(), diff --git a/src/merge.js b/src/merge.js index 1002f64..8cec8d0 100644 --- a/src/merge.js +++ b/src/merge.js @@ -252,6 +252,7 @@ export function mergeSoloGroup(ctx, scope, group) { if (targetBeforeExtras) { beforeResidual = makeResidual(target, targetOriginalSelector, targetBeforeExtras); target.before(beforeResidual); + recordInsertion(beforeResidual); target.raws.before = interPieceSeparator; } @@ -540,6 +541,7 @@ function splitStarHub(ctx, scope, cluster, hub) { if (trailingGap.length) finalRules.push(makeResidual(hub, hubOriginalSelector, trailingGap)); hub.before(finalRules[0]); + recordInsertion(finalRules[0]); for (let i = 1; i < finalRules.length; i++) { insertAfter(finalRules[i - 1], finalRules[i], interPieceSeparator); recordInsertion(finalRules[i]); diff --git a/test/dedup.test.js b/test/dedup.test.js index ca45601..b46276b 100644 --- a/test/dedup.test.js +++ b/test/dedup.test.js @@ -3,7 +3,8 @@ import assert from 'node:assert'; import { analyze, dedup } from '../src/index.js'; import { normalizeValue } from '../src/lib/normalization.js'; import { selectorsLikelyDisjoint } from '../src/lib/selectors.js'; -import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssEntangledGrowing, cssEntangledShrinking, cssMixed, cssNestedHost } from './helpers.js'; +import { measuredByteTotal } from '../src/lib/transaction.js'; +import { RE_MERGED_AB, RE_MERGED_AC, cssGrowing, cssGrowingAggressive, cssEntangledGrowing, cssEntangledShrinking, cssMixed, cssNestedHost, cssTwoLeadingRemovals } from './helpers.js'; describe('Deduplication', () => { test('Treats a `)` inside a `/* … */` comment as text when scanning a `min()` call', () => { @@ -863,6 +864,60 @@ describe('Savings only', () => { assert.strictEqual(gated.withheld, undefined); }); + test('Restores leading whitespace when a declined merge emptied two rules at the top of the file', () => { + const ungated = dedup(cssTwoLeadingRemovals); + assert.ok(ungated.bytes.saved < 0, 'fixture should grow the file when ungated'); + // The merge empties both leading rules before it is undone + assert.ok(!ungated.css.startsWith('.a {') && !ungated.css.includes('\n.b {')); + + for (const aggressive of [false, true]) { + const { css: output, applied, bytes } = dedup(cssTwoLeadingRemovals, { savingsOnly: true, aggressive }); + assert.strictEqual(output, cssTwoLeadingRemovals, `output should be restored byte for byte (aggressive: ${aggressive})`); + assert.strictEqual(applied.length, 0); + assert.strictEqual(bytes.saved, 0); + } + }); + + test('Prices a merge from the rules it touches, not from the whole style sheet', () => { + // Doubling the number of duplicate groups should roughly double the work + // the gate does. If pricing one merge re-serializes everything around it, + // the work grows with the file instead—which is how three separate + // quadratic regressions in this accounting first showed up. + const build = (groups, indent) => { + let css = ''; + for (let index = 0; index < groups; index++) { + const one = `.a${index}`.padEnd(28, 'x'); + const two = `.b${index}`.padEnd(28, 'y'); + css += `${indent}${one} { color: #${index % 900 + 100}; z-index: ${index}; }\n`; + css += `${indent}${two} { color: #${index % 900 + 100}; top: ${index}px; }\n`; + } + return css; + }; + // Both at the root and inside one shared block, where every group's + // enclosing subtree is the whole rest of the file + const shapes = { + root: groups => build(groups, ''), + block: groups => `@media (min-width: 40em) {\n${build(groups, ' ')}}\n`, + }; + + for (const [shape, make] of Object.entries(shapes)) { + const work = groups => { + const before = measuredByteTotal(); + dedup(make(groups), { savingsOnly: true }); + return measuredByteTotal() - before; + }; + const small = work(50); + const large = work(200); + // Four times the groups, so linear pricing lands near four times the + // work; the bound is loose enough for the fixed-point loop's extra + // passes, and far under the sixteen-fold a quadratic pass would cost + assert.ok( + large < small * 8, + `pricing scales with the file rather than the merge (${shape}): ${small} → ${large} bytes measured` + ); + } + }); + test('Declining a merge leaves the merges around it byte-identical to an ungated run', () => { // The gate must not perturb what it does not decline: The shrinking // cluster has to come out exactly as it would on its own diff --git a/test/helpers.js b/test/helpers.js index 742cc72..785d48b 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -92,6 +92,19 @@ export const cssEntangledShrinking = [ '', ].join('\n'); +// A declined merge that first empties the *two* leading root rules: PostCSS +// hands each removed first child’s `raws.before` to its successor, so the blank +// line before `.mid` travels down the chain and has to be handed back on +// rollback—byte for byte, or the file comes out reformatted. +export const cssTwoLeadingRemovals = [ + '.a { color: red; }', + '.b { color: red; }', + '', + '.mid { top: 0; }', + '.c-with-an-extremely-long-selector-name { color: red; padding: 0; }', + '', +].join('\n'); + // Only mergeable in aggressive mode (canonicalizing the `` values is // aggressive-only), and—unlike `cssGrowingAggressive`—the merge shrinks the // file: Each rule holds only the one shared declaration, so folding them