Skip to content

Commit bf35513

Browse files
nex3nex3
authored
Avoid adding duplicate modules when importing forwarded stylesheets (#973)
Avoid adding duplicate modules when importing forwarded stylesheets In Google stylesheets that imported import-only stylesheets with many forwards many of times, we were seeing Environment._globalModules grow large enough that variable accesses were a problem. This addresses that in several ways: * Forwarded modules are now ignored if the module is already being forwarded. * Module equality is more intelligent, so that shadowed and forwarded modules can be effectively de-duplicated. * If a module would be fully shadowed by a later import *and* that module has no CSS, we no longer add an empty ShadowedModule to the module list.
1 parent 4d78316 commit bf35513

File tree

8 files changed

+132
-38
lines changed

8 files changed

+132
-38
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
## 1.26.4
2+
3+
* Be more memory-efficient when handling `@forward`s through `@import`s.
4+
15
## 1.26.3
26

37
* Fix a bug where `--watch` mode could go into an infinite loop compiling CSS

‎lib/src/ast/sass/statement/forward_rule.dart‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ class ForwardRule implements Statement {
116116
buffer
117117
..write(" show ")
118118
..write(_memberList(shownMixinsAndFunctions, shownVariables));
119-
} else if (hiddenMixinsAndFunctions != null) {
119+
} else if (hiddenMixinsAndFunctions != null &&
120+
hiddenMixinsAndFunctions.isNotEmpty) {
120121
buffer
121122
..write(" hide ")
122123
..write(_memberList(hiddenMixinsAndFunctions, hiddenVariables));
@@ -135,7 +136,8 @@ class ForwardRule implements Statement {
135136
/// Returns a combined list of names of the given members.
136137
String _memberList(
137138
Iterable<String> mixinsAndFunctions, Iterable<String> variables) =>
138-
shownMixinsAndFunctions
139-
.followedBy(shownVariables.map((name) => "\$$name"))
140-
.join(", ");
139+
[
140+
if (shownMixinsAndFunctions != null) ...shownMixinsAndFunctions,
141+
if (shownVariables != null) for (var name in shownVariables) "\$$name"
142+
].join(", ");
141143
}

‎lib/src/async_environment.dart‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class AsyncEnvironment {
4949
/// The modules forwarded by this module.
5050
///
5151
/// This is `null` if there are no forwarded modules.
52-
List<Module> _forwardedModules;
52+
Set<Module> _forwardedModules;
5353

5454
/// A map from modules in [_forwardedModules] to the nodes whose spans
5555
/// indicate where those modules were originally forwarded.
@@ -264,10 +264,10 @@ class AsyncEnvironment {
264264
/// Exposes the members in [module] to downstream modules as though they were
265265
/// defined in this module, according to the modifications defined by [rule].
266266
void forwardModule(Module module, ForwardRule rule) {
267-
_forwardedModules ??= [];
267+
_forwardedModules ??= {};
268268
_forwardedModuleNodes ??= {};
269269

270-
var view = ForwardedModuleView(module, rule);
270+
var view = ForwardedModuleView.ifNecessary(module, rule);
271271
for (var other in _forwardedModules) {
272272
_assertNoConflicts(
273273
view.variables, other.variables, view, other, "variable", rule);
@@ -331,7 +331,18 @@ class AsyncEnvironment {
331331
var forwarded = module._environment._forwardedModules;
332332
if (forwarded == null) return;
333333

334-
_forwardedModules ??= [];
334+
// Omit modules from [forwarded] that are already globally available and
335+
// forwarded in this module.
336+
if (_forwardedModules != null) {
337+
forwarded = {
338+
for (var module in forwarded)
339+
if (!_forwardedModules.contains(module) ||
340+
!_globalModules.contains(module))
341+
module
342+
};
343+
}
344+
345+
_forwardedModules ??= {};
335346
_forwardedModuleNodes ??= {};
336347

337348
var forwardedVariableNames =
@@ -351,20 +362,26 @@ class AsyncEnvironment {
351362
functions: forwardedFunctionNames);
352363
if (shadowed != null) {
353364
_globalModules.remove(module);
354-
_globalModules.add(shadowed);
355-
_globalModuleNodes[shadowed] = _globalModuleNodes.remove(module);
365+
366+
if (!shadowed.isEmpty) {
367+
_globalModules.add(shadowed);
368+
_globalModuleNodes[shadowed] = _globalModuleNodes.remove(module);
369+
}
356370
}
357371
}
358-
for (var i = 0; i < _forwardedModules.length; i++) {
359-
var module = _forwardedModules[i];
372+
for (var module in _forwardedModules.toList()) {
360373
var shadowed = ShadowedModuleView.ifNecessary(module,
361374
variables: forwardedVariableNames,
362375
mixins: forwardedMixinNames,
363376
functions: forwardedFunctionNames);
364377
if (shadowed != null) {
365-
_forwardedModules[i] = shadowed;
366-
_forwardedModuleNodes[shadowed] =
367-
_forwardedModuleNodes.remove(module);
378+
_forwardedModules.remove(module);
379+
380+
if (!shadowed.isEmpty) {
381+
_forwardedModules.add(shadowed);
382+
_forwardedModuleNodes[shadowed] =
383+
_forwardedModuleNodes.remove(module);
384+
}
368385
}
369386
}
370387

@@ -815,7 +832,8 @@ class AsyncEnvironment {
815832
Module toDummyModule() {
816833
return _EnvironmentModule(
817834
this,
818-
CssStylesheet(const [], SourceFile.decoded(const []).span(0)),
835+
CssStylesheet(const [],
836+
SourceFile.decoded(const [], url: "<dummy module>").span(0)),
819837
Extender.empty,
820838
forwarded: _forwardedModules);
821839
}
@@ -905,8 +923,8 @@ class _EnvironmentModule implements Module {
905923

906924
factory _EnvironmentModule(
907925
AsyncEnvironment environment, CssStylesheet css, Extender extender,
908-
{List<Module> forwarded}) {
909-
forwarded ??= const [];
926+
{Set<Module> forwarded}) {
927+
forwarded ??= const {};
910928
return _EnvironmentModule._(
911929
environment,
912930
css,
@@ -931,7 +949,7 @@ class _EnvironmentModule implements Module {
931949
}
932950

933951
/// Create [_modulesByVariable] for a set of forwarded modules.
934-
static Map<String, Module> _makeModulesByVariable(List<Module> forwarded) {
952+
static Map<String, Module> _makeModulesByVariable(Set<Module> forwarded) {
935953
if (forwarded.isEmpty) return const {};
936954

937955
var modulesByVariable = <String, Module>{};
@@ -1020,5 +1038,5 @@ class _EnvironmentModule implements Module {
10201038
transitivelyContainsExtensions: transitivelyContainsExtensions);
10211039
}
10221040

1023-
String toString() => p.prettyUri(css.span.sourceUrl);
1041+
String toString() => url == null ? "<unknown url>" : p.prettyUri(url);
10241042
}

‎lib/src/environment.dart‎

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// DO NOT EDIT. This file was generated from async_environment.dart.
66
// See tool/grind/synchronize.dart for details.
77
//
8-
// Checksum: df5ee8bde1eec6e47c1d025041921aba01637696
8+
// Checksum: bf1542d44f45a40a1f56c8eec3046c61ee32c36a
99
//
1010
// ignore_for_file: unused_import
1111

@@ -55,7 +55,7 @@ class Environment {
5555
/// The modules forwarded by this module.
5656
///
5757
/// This is `null` if there are no forwarded modules.
58-
List<Module<Callable>> _forwardedModules;
58+
Set<Module<Callable>> _forwardedModules;
5959

6060
/// A map from modules in [_forwardedModules] to the nodes whose spans
6161
/// indicate where those modules were originally forwarded.
@@ -271,10 +271,10 @@ class Environment {
271271
/// Exposes the members in [module] to downstream modules as though they were
272272
/// defined in this module, according to the modifications defined by [rule].
273273
void forwardModule(Module<Callable> module, ForwardRule rule) {
274-
_forwardedModules ??= [];
274+
_forwardedModules ??= {};
275275
_forwardedModuleNodes ??= {};
276276

277-
var view = ForwardedModuleView(module, rule);
277+
var view = ForwardedModuleView.ifNecessary(module, rule);
278278
for (var other in _forwardedModules) {
279279
_assertNoConflicts(
280280
view.variables, other.variables, view, other, "variable", rule);
@@ -338,7 +338,18 @@ class Environment {
338338
var forwarded = module._environment._forwardedModules;
339339
if (forwarded == null) return;
340340

341-
_forwardedModules ??= [];
341+
// Omit modules from [forwarded] that are already globally available and
342+
// forwarded in this module.
343+
if (_forwardedModules != null) {
344+
forwarded = {
345+
for (var module in forwarded)
346+
if (!_forwardedModules.contains(module) ||
347+
!_globalModules.contains(module))
348+
module
349+
};
350+
}
351+
352+
_forwardedModules ??= {};
342353
_forwardedModuleNodes ??= {};
343354

344355
var forwardedVariableNames =
@@ -358,20 +369,26 @@ class Environment {
358369
functions: forwardedFunctionNames);
359370
if (shadowed != null) {
360371
_globalModules.remove(module);
361-
_globalModules.add(shadowed);
362-
_globalModuleNodes[shadowed] = _globalModuleNodes.remove(module);
372+
373+
if (!shadowed.isEmpty) {
374+
_globalModules.add(shadowed);
375+
_globalModuleNodes[shadowed] = _globalModuleNodes.remove(module);
376+
}
363377
}
364378
}
365-
for (var i = 0; i < _forwardedModules.length; i++) {
366-
var module = _forwardedModules[i];
379+
for (var module in _forwardedModules.toList()) {
367380
var shadowed = ShadowedModuleView.ifNecessary(module,
368381
variables: forwardedVariableNames,
369382
mixins: forwardedMixinNames,
370383
functions: forwardedFunctionNames);
371384
if (shadowed != null) {
372-
_forwardedModules[i] = shadowed;
373-
_forwardedModuleNodes[shadowed] =
374-
_forwardedModuleNodes.remove(module);
385+
_forwardedModules.remove(module);
386+
387+
if (!shadowed.isEmpty) {
388+
_forwardedModules.add(shadowed);
389+
_forwardedModuleNodes[shadowed] =
390+
_forwardedModuleNodes.remove(module);
391+
}
375392
}
376393
}
377394

@@ -820,7 +837,8 @@ class Environment {
820837
Module<Callable> toDummyModule() {
821838
return _EnvironmentModule(
822839
this,
823-
CssStylesheet(const [], SourceFile.decoded(const []).span(0)),
840+
CssStylesheet(const [],
841+
SourceFile.decoded(const [], url: "<dummy module>").span(0)),
824842
Extender.empty,
825843
forwarded: _forwardedModules);
826844
}
@@ -911,8 +929,8 @@ class _EnvironmentModule implements Module<Callable> {
911929

912930
factory _EnvironmentModule(
913931
Environment environment, CssStylesheet css, Extender extender,
914-
{List<Module<Callable>> forwarded}) {
915-
forwarded ??= const [];
932+
{Set<Module<Callable>> forwarded}) {
933+
forwarded ??= const {};
916934
return _EnvironmentModule._(
917935
environment,
918936
css,
@@ -938,7 +956,7 @@ class _EnvironmentModule implements Module<Callable> {
938956

939957
/// Create [_modulesByVariable] for a set of forwarded modules.
940958
static Map<String, Module<Callable>> _makeModulesByVariable(
941-
List<Module<Callable>> forwarded) {
959+
Set<Module<Callable>> forwarded) {
942960
if (forwarded.isEmpty) return const {};
943961

944962
var modulesByVariable = <String, Module<Callable>>{};
@@ -1027,5 +1045,5 @@ class _EnvironmentModule implements Module<Callable> {
10271045
transitivelyContainsExtensions: transitivelyContainsExtensions);
10281046
}
10291047

1030-
String toString() => p.prettyUri(css.span.sourceUrl);
1048+
String toString() => url == null ? "<unknown url>" : p.prettyUri(url);
10311049
}

‎lib/src/module/forwarded_view.dart‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,22 @@ class ForwardedModuleView<T extends AsyncCallable> implements Module<T> {
3434
final Map<String, T> functions;
3535
final Map<String, T> mixins;
3636

37+
/// Like [ForwardedModuleView], but returns `inner` as-is if it doesn't need
38+
/// any modification.
39+
static Module<T> ifNecessary<T extends AsyncCallable>(
40+
Module<T> inner, ForwardRule rule) {
41+
if (rule.prefix == null &&
42+
rule.shownMixinsAndFunctions == null &&
43+
rule.shownVariables == null &&
44+
(rule.hiddenMixinsAndFunctions == null ||
45+
rule.hiddenMixinsAndFunctions.isEmpty) &&
46+
(rule.hiddenVariables == null || rule.hiddenVariables.isEmpty)) {
47+
return inner;
48+
} else {
49+
return ForwardedModuleView(inner, rule);
50+
}
51+
}
52+
3753
ForwardedModuleView(this._inner, this._rule)
3854
: variables = _forwardedMap(_inner.variables, _rule.prefix,
3955
_rule.shownVariables, _rule.hiddenVariables),
@@ -102,5 +118,14 @@ class ForwardedModuleView<T extends AsyncCallable> implements Module<T> {
102118
return _inner.variableIdentity(name);
103119
}
104120

121+
bool operator ==(Object other) =>
122+
other is ForwardedModuleView &&
123+
_inner == other._inner &&
124+
_rule == other._rule;
125+
126+
int get hashCode => _inner.hashCode ^ _rule.hashCode;
127+
105128
Module<T> cloneCss() => ForwardedModuleView(_inner.cloneCss(), _rule);
129+
130+
String toString() => "forwarded $_inner";
106131
}

‎lib/src/module/shadowed_view.dart‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import '../exception.dart';
99
import '../extend/extender.dart';
1010
import '../module.dart';
1111
import '../util/limited_map_view.dart';
12+
import '../utils.dart';
1213
import '../value.dart';
1314

1415
/// A [Module] that only exposes members that aren't shadowed by a given
@@ -30,6 +31,13 @@ class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
3031
final Map<String, T> functions;
3132
final Map<String, T> mixins;
3233

34+
/// Returns whether this module exposes no members or CSS.
35+
bool get isEmpty =>
36+
variables.isEmpty &&
37+
functions.isEmpty &&
38+
mixins.isEmpty &&
39+
css.children.isEmpty;
40+
3341
/// Like [ShadowedModuleView], but returns `null` if [inner] would be unchanged.
3442
static ShadowedModuleView<T> ifNecessary<T extends AsyncCallable>(
3543
Module<T> inner,
@@ -79,6 +87,17 @@ class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
7987
return _inner.variableIdentity(name);
8088
}
8189

90+
bool operator ==(Object other) =>
91+
other is ShadowedModuleView &&
92+
_inner == other._inner &&
93+
iterableEquals(variables.keys, other.variables.keys) &&
94+
iterableEquals(functions.keys, other.functions.keys) &&
95+
iterableEquals(mixins.keys, other.mixins.keys);
96+
97+
int get hashCode => _inner.hashCode;
98+
8299
Module<T> cloneCss() => ShadowedModuleView._(
83100
_inner.cloneCss(), variables, variableNodes, functions, mixins);
101+
102+
String toString() => "shadowed $_inner";
84103
}

‎lib/src/utils.dart‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,14 @@ int codeUnitIndexToCodepointIndex(String string, int codeUnitIndex) {
173173
return codepointIndex;
174174
}
175175

176+
/// Returns whether [iterable1] and [iterable2] have the same contents.
177+
bool iterableEquals(Iterable<Object> iterable1, Iterable<Object> iterable2) =>
178+
const IterableEquality<Object>().equals(iterable1, iterable2);
179+
180+
/// Returns a hash code for [iterable] that matches [iterableEquals].
181+
int iterableHash(Iterable<Object> iterable) =>
182+
const IterableEquality<Object>().hash(iterable);
183+
176184
/// Returns whether [list1] and [list2] have the same contents.
177185
bool listEquals(List<Object> list1, List<Object> list2) =>
178186
const ListEquality<Object>().equals(list1, list2);

‎pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
name: sass
2-
version: 1.26.3
2+
version: 1.26.4
33
description: A Sass implementation in Dart.
44
author: Sass Team
55
homepage: https://github.com/sass/dart-sass

0 commit comments

Comments
 (0)