Skip to content

Commit

Permalink
Remove the heuristic where long selector lists wouldn't be trimmed (#…
Browse files Browse the repository at this point in the history
…2255)

Testing this against the `@extend`-heavy stylesheets in
vinceliuice/Colloid-gtk-theme, trimming everywhere actually *improves*
performance rather than reducing it.
  • Loading branch information
nex3 authored Jun 11, 2024
1 parent 5ddd7fc commit ecff05d
Show file tree
Hide file tree
Showing 9 changed files with 59 additions and 24 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 1.77.5

* Fully trim redundant selectors generated by `@extend`.

## 1.77.4

### Embedded Sass
Expand Down
12 changes: 12 additions & 0 deletions lib/src/ast/selector/compound.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ final class CompoundSelector extends Selector {
SimpleSelector? get singleSimple =>
components.length == 1 ? components.first : null;

/// Whether any simple selector in this contains a selector that requires
/// complex non-local reasoning to determine whether it's a super- or
/// sub-selector.
///
/// This includes both pseudo-elements and pseudo-selectors that take
/// selectors as arguments.
///
/// #nodoc
@internal
late final bool hasComplicatedSuperselectorSemantics = components
.any((component) => component.hasComplicatedSuperselectorSemantics);

CompoundSelector(Iterable<SimpleSelector> components, super.span)
: components = List.unmodifiable(components) {
if (this.components.isEmpty) {
Expand Down
4 changes: 4 additions & 0 deletions lib/src/ast/selector/pseudo.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ final class PseudoSelector extends SimpleSelector {
bool get isHostContext =>
isClass && name == 'host-context' && selector != null;

@internal
bool get hasComplicatedSuperselectorSemantics =>
isElement || selector != null;

/// The non-selector argument passed to this selector.
///
/// This is `null` if there's no argument. If [argument] and [selector] are
Expand Down
10 changes: 10 additions & 0 deletions lib/src/ast/selector/simple.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ abstract base class SimpleSelector extends Selector {
/// sequence will contain 1000 simple selectors.
int get specificity => 1000;

/// Whether this requires complex non-local reasoning to determine whether
/// it's a super- or sub-selector.
///
/// This includes both pseudo-elements and pseudo-selectors that take
/// selectors as arguments.
///
/// #nodoc
@internal
bool get hasComplicatedSuperselectorSemantics => false;

SimpleSelector(super.span);

/// Parses a simple selector from [contents].
Expand Down
7 changes: 0 additions & 7 deletions lib/src/extend/extension_store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -901,13 +901,6 @@ class ExtensionStore {
// document, and thus should never be trimmed.
List<ComplexSelector> _trim(List<ComplexSelector> selectors,
bool isOriginal(ComplexSelector complex)) {
// Avoid truly horrific quadratic behavior.
//
// TODO(nweiz): I think there may be a way to get perfect trimming without
// going quadratic by building some sort of trie-like data structure that
// can be used to look up superselectors.
if (selectors.length > 100) return selectors;

// This is n² on the sequences, but only comparing between separate
// sequences should limit the quadratic behavior. We iterate from last to
// first and reverse the result so that, if two selectors are identical, we
Expand Down
36 changes: 22 additions & 14 deletions lib/src/extend/functions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -646,24 +646,28 @@ bool complexIsSuperselector(List<ComplexSelectorComponent> complex1,
var component1 = complex1[i1];
if (component1.combinators.length > 1) return false;
if (remaining1 == 1) {
var parents = complex2.sublist(i2, complex2.length - 1);
if (parents.any((parent) => parent.combinators.length > 1)) return false;

return compoundIsSuperselector(
component1.selector, complex2.last.selector,
parents: parents);
if (complex2.any((parent) => parent.combinators.length > 1)) {
return false;
} else {
return compoundIsSuperselector(
component1.selector, complex2.last.selector,
parents: component1.selector.hasComplicatedSuperselectorSemantics
? complex2.sublist(i2, complex2.length - 1)
: null);
}
}

// Find the first index [endOfSubselector] in [complex2] such that
// `complex2.sublist(i2, endOfSubselector + 1)` is a subselector of
// [component1.selector].
var endOfSubselector = i2;
List<ComplexSelectorComponent>? parents;
while (true) {
var component2 = complex2[endOfSubselector];
if (component2.combinators.length > 1) return false;
if (compoundIsSuperselector(component1.selector, component2.selector,
parents: parents)) {
parents: component1.selector.hasComplicatedSuperselectorSemantics
? complex2.sublist(i2, endOfSubselector)
: null)) {
break;
}

Expand All @@ -675,13 +679,10 @@ bool complexIsSuperselector(List<ComplexSelectorComponent> complex1,
// to match.
return false;
}

parents ??= [];
parents.add(component2);
}

if (!_compatibleWithPreviousCombinator(
previousCombinator, parents ?? const [])) {
previousCombinator, complex2.take(endOfSubselector).skip(i2))) {
return false;
}

Expand Down Expand Up @@ -717,8 +718,8 @@ bool complexIsSuperselector(List<ComplexSelectorComponent> complex1,
/// Returns whether [parents] are valid intersitial components between one
/// complex superselector and another, given that the earlier complex
/// superselector had the combinator [previous].
bool _compatibleWithPreviousCombinator(
CssValue<Combinator>? previous, List<ComplexSelectorComponent> parents) {
bool _compatibleWithPreviousCombinator(CssValue<Combinator>? previous,
Iterable<ComplexSelectorComponent> parents) {
if (parents.isEmpty) return true;
if (previous == null) return true;

Expand Down Expand Up @@ -754,6 +755,13 @@ bool _isSupercombinator(
bool compoundIsSuperselector(
CompoundSelector compound1, CompoundSelector compound2,
{Iterable<ComplexSelectorComponent>? parents}) {
if (!compound1.hasComplicatedSuperselectorSemantics &&
!compound2.hasComplicatedSuperselectorSemantics) {
if (compound1.components.length > compound2.components.length) return false;
return compound1.components
.every((simple1) => compound2.components.any(simple1.isSuperselector));
}

// Pseudo elements effectively change the target of a compound selector rather
// than narrowing the set of elements to which it applies like other
// selectors. As such, if either selector has a pseudo element, they both must
Expand Down
4 changes: 4 additions & 0 deletions pkg/sass_api/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 10.4.5

* No user-visible changes.

## 10.4.4

* No user-visible changes.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sass_api/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ name: sass_api
# Note: Every time we add a new Sass AST node, we need to bump the *major*
# version because it's a breaking change for anyone who's implementing the
# visitor interface(s).
version: 10.4.4
version: 10.4.5
description: Additional APIs for Dart Sass.
homepage: https://github.com/sass/dart-sass

environment:
sdk: ">=3.0.0 <4.0.0"

dependencies:
sass: 1.77.4
sass: 1.77.5

dev_dependencies:
dartdoc: ">=6.0.0 <9.0.0"
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: sass
version: 1.77.4
version: 1.77.5
description: A Sass implementation in Dart.
homepage: https://github.com/sass/dart-sass

Expand Down

0 comments on commit ecff05d

Please sign in to comment.