Skip to content

Commit 9fdf561

Browse files
nex3nex3
authored
Fix a performance bug in BinaryOperatorExpression.span (#569)
Previously, evaluator called BinaryOperationExpression.span for each binary operation it evaluated, which in turn called spanForList() to create a span covering both child expressions. spanForList() then called .span for both the left and right child operations *twice*, leading to exponential behavior. This is now avoided in three complementary ways: 1. The evaluator avoids eagerly calling AstNode.span, instead keeping the original AstNode until the span itself needs to be accessed. This means that a span will only be accessed when an error actually occurs, and then only one operation's span will be accessed. 2. BinaryOperationExpression.span now iterates through any child operations before calling their .span methods, so it only performs O(1) allocations. 3. spanForList() now only calls each AstNode.span once.
1 parent b024276 commit 9fdf561

File tree

8 files changed

+453
-296
lines changed

8 files changed

+453
-296
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
## 1.16.1
2+
3+
* Fix a performance bug where stylesheet evaluation could take a very long time
4+
when many binary operators were used in sequence.
5+
16
## 1.16.0
27

38
* `rgb()` and `hsl()` now treat unquoted strings beginning with `env()`,

‎lib/src/ast/sass/expression/binary_operation.dart‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,20 @@ class BinaryOperationExpression implements Expression {
2424
/// interpreted as slash-separated numbers.
2525
final bool allowsSlash;
2626

27-
FileSpan get span => spanForList([left, right]);
27+
FileSpan get span {
28+
// Avoid creating a bunch of intermediate spans for multiple binary
29+
// expressions in a row by moving to the left- and right-most expressions.
30+
var left = this.left;
31+
while (left is BinaryOperationExpression) {
32+
left = (left as BinaryOperationExpression).left;
33+
}
34+
35+
var right = this.right;
36+
while (right is BinaryOperationExpression) {
37+
right = (right as BinaryOperationExpression).right;
38+
}
39+
return spanForList([left, right]);
40+
}
2841

2942
BinaryOperationExpression(this.operator, this.left, this.right)
3043
: allowsSlash = false;

‎lib/src/async_environment.dart‎

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import 'dart:async';
66

77
import 'package:source_span/source_span.dart';
88

9+
import 'ast/node.dart';
910
import 'callable.dart';
1011
import 'functions.dart';
1112
import 'value.dart';
@@ -26,10 +27,14 @@ class AsyncEnvironment {
2627
/// deeper in the tree.
2728
final List<Map<String, Value>> _variables;
2829

29-
/// The spans where each variable in [_variables] was defined.
30+
/// The nodes where each variable in [_variables] was defined.
3031
///
3132
/// This is `null` if source mapping is disabled.
32-
final List<Map<String, FileSpan>> _variableSpans;
33+
///
34+
/// This stores [AstNode]s rather than [FileSpan]s so it can avoid calling
35+
/// [AstNode.span] if the span isn't required, since some nodes need to do
36+
/// real work to manufacture a source span.
37+
final List<Map<String, AstNode>> _variableNodes;
3338

3439
/// A map of variable names to their indices in [_variables].
3540
///
@@ -104,7 +109,7 @@ class AsyncEnvironment {
104109
/// If [sourceMap] is `true`, this tracks variables' source locations
105110
AsyncEnvironment({bool sourceMap = false})
106111
: _variables = [normalizedMap()],
107-
_variableSpans = sourceMap ? [normalizedMap()] : null,
112+
_variableNodes = sourceMap ? [normalizedMap()] : null,
108113
_variableIndices = normalizedMap(),
109114
_functions = [normalizedMap()],
110115
_functionIndices = normalizedMap(),
@@ -113,7 +118,7 @@ class AsyncEnvironment {
113118
coreFunctions.forEach(setFunction);
114119
}
115120

116-
AsyncEnvironment._(this._variables, this._variableSpans, this._functions,
121+
AsyncEnvironment._(this._variables, this._variableNodes, this._functions,
117122
this._mixins, this._content)
118123
// Lazily fill in the indices rather than eagerly copying them from the
119124
// existing environment in closure() because the copying took a lot of
@@ -130,7 +135,7 @@ class AsyncEnvironment {
130135
/// when the closure was created will be reflected.
131136
AsyncEnvironment closure() => AsyncEnvironment._(
132137
_variables.toList(),
133-
_variableSpans?.toList(),
138+
_variableNodes?.toList(),
134139
_functions.toList(),
135140
_mixins.toList(),
136141
_content);
@@ -156,18 +161,24 @@ class AsyncEnvironment {
156161
return _variables[index][name];
157162
}
158163

159-
/// Returns the source span for the variable named [name], or `null` if no
160-
/// such variable is declared.
161-
FileSpan getVariableSpan(String name) {
164+
/// Returns the node for the variable named [name], or `null` if no such
165+
/// variable is declared.
166+
///
167+
/// This node is intended as a proxy for the [FileSpan] indicating where the
168+
/// variable's value originated. It's returned as an [AstNode] rather than a
169+
/// [FileSpan] so we can avoid calling [AstNode.span] if the span isn't
170+
/// required, since some nodes need to do real work to manufacture a source
171+
/// span.
172+
AstNode getVariableNode(String name) {
162173
if (_lastVariableName == name) {
163-
return _variableSpans[_lastVariableIndex][name];
174+
return _variableNodes[_lastVariableIndex][name];
164175
}
165176

166177
var index = _variableIndices[name];
167178
if (index != null) {
168179
_lastVariableName = name;
169180
_lastVariableIndex = index;
170-
return _variableSpans[index][name];
181+
return _variableNodes[index][name];
171182
}
172183

173184
index = _variableIndex(name);
@@ -176,7 +187,7 @@ class AsyncEnvironment {
176187
_lastVariableName = name;
177188
_lastVariableIndex = index;
178189
_variableIndices[name] = index;
179-
return _variableSpans[index][name];
190+
return _variableNodes[index][name];
180191
}
181192

182193
/// Returns whether a variable named [name] exists.
@@ -194,12 +205,17 @@ class AsyncEnvironment {
194205
return null;
195206
}
196207

197-
/// Sets the variable named [name] to [value], associated with the given [span].
208+
/// Sets the variable named [name] to [value], associated with
209+
/// [nodeWithSpan]'s source span.
198210
///
199211
/// If [global] is `true`, this sets the variable at the top-level scope.
200212
/// Otherwise, if the variable was already defined, it'll set it in the
201213
/// previous scope. If it's undefined, it'll set it in the current scope.
202-
void setVariable(String name, Value value, FileSpan span,
214+
///
215+
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
216+
/// [AstNode.span] if the span isn't required, since some nodes need to do
217+
/// real work to manufacture a source span.
218+
void setVariable(String name, Value value, AstNode nodeWithSpan,
203219
{bool global = false}) {
204220
if (global || _variables.length == 1) {
205221
// Don't set the index if there's already a variable with the given name,
@@ -211,7 +227,7 @@ class AsyncEnvironment {
211227
});
212228

213229
_variables.first[name] = value;
214-
if (_variableSpans != null) _variableSpans.first[name] = span;
230+
if (_variableNodes != null) _variableNodes.first[name] = nodeWithSpan;
215231
return;
216232
}
217233

@@ -227,20 +243,25 @@ class AsyncEnvironment {
227243
_lastVariableName = name;
228244
_lastVariableIndex = index;
229245
_variables[index][name] = value;
230-
if (_variableSpans != null) _variableSpans[index][name] = span;
246+
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
231247
}
232248

233-
/// Sets the variable named [name] to [value] in the current scope, associated with the given [span].
249+
/// Sets the variable named [name] to [value], associated with
250+
/// [nodeWithSpan]'s source span.
234251
///
235252
/// Unlike [setVariable], this will declare the variable in the current scope
236253
/// even if a declaration already exists in an outer scope.
237-
void setLocalVariable(String name, Value value, FileSpan span) {
254+
///
255+
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
256+
/// [AstNode.span] if the span isn't required, since some nodes need to do
257+
/// real work to manufacture a source span.
258+
void setLocalVariable(String name, Value value, AstNode nodeWithSpan) {
238259
var index = _variables.length - 1;
239260
_lastVariableName = name;
240261
_lastVariableIndex = index;
241262
_variableIndices[name] = index;
242263
_variables[index][name] = value;
243-
if (_variableSpans != null) _variableSpans[index][name] = span;
264+
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
244265
}
245266

246267
/// Returns the value of the function named [name], or `null` if no such
@@ -358,7 +379,7 @@ class AsyncEnvironment {
358379
_inSemiGlobalScope = semiGlobal;
359380

360381
_variables.add(normalizedMap());
361-
_variableSpans?.add(normalizedMap());
382+
_variableNodes?.add(normalizedMap());
362383
_functions.add(normalizedMap());
363384
_mixins.add(normalizedMap());
364385
try {

‎lib/src/environment.dart‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,13 @@
55
// DO NOT EDIT. This file was generated from async_environment.dart.
66
// See tool/synchronize.dart for details.
77
//
8-
// Checksum: 9e0f9274f4778b701f268bcf4fc349a1cf17a159
8+
// Checksum: 449ed8a8ad29fe107656a666e6e6005ef539b834
99
//
1010
// ignore_for_file: unused_import
1111

1212
import 'package:source_span/source_span.dart';
1313

14+
import 'ast/node.dart';
1415
import 'callable.dart';
1516
import 'functions.dart';
1617
import 'value.dart';
@@ -31,10 +32,14 @@ class Environment {
3132
/// deeper in the tree.
3233
final List<Map<String, Value>> _variables;
3334

34-
/// The spans where each variable in [_variables] was defined.
35+
/// The nodes where each variable in [_variables] was defined.
3536
///
3637
/// This is `null` if source mapping is disabled.
37-
final List<Map<String, FileSpan>> _variableSpans;
38+
///
39+
/// This stores [AstNode]s rather than [FileSpan]s so it can avoid calling
40+
/// [AstNode.span] if the span isn't required, since some nodes need to do
41+
/// real work to manufacture a source span.
42+
final List<Map<String, AstNode>> _variableNodes;
3843

3944
/// A map of variable names to their indices in [_variables].
4045
///
@@ -109,7 +114,7 @@ class Environment {
109114
/// If [sourceMap] is `true`, this tracks variables' source locations
110115
Environment({bool sourceMap = false})
111116
: _variables = [normalizedMap()],
112-
_variableSpans = sourceMap ? [normalizedMap()] : null,
117+
_variableNodes = sourceMap ? [normalizedMap()] : null,
113118
_variableIndices = normalizedMap(),
114119
_functions = [normalizedMap()],
115120
_functionIndices = normalizedMap(),
@@ -118,7 +123,7 @@ class Environment {
118123
coreFunctions.forEach(setFunction);
119124
}
120125

121-
Environment._(this._variables, this._variableSpans, this._functions,
126+
Environment._(this._variables, this._variableNodes, this._functions,
122127
this._mixins, this._content)
123128
// Lazily fill in the indices rather than eagerly copying them from the
124129
// existing environment in closure() because the copying took a lot of
@@ -135,7 +140,7 @@ class Environment {
135140
/// when the closure was created will be reflected.
136141
Environment closure() => Environment._(
137142
_variables.toList(),
138-
_variableSpans?.toList(),
143+
_variableNodes?.toList(),
139144
_functions.toList(),
140145
_mixins.toList(),
141146
_content);
@@ -161,18 +166,24 @@ class Environment {
161166
return _variables[index][name];
162167
}
163168

164-
/// Returns the source span for the variable named [name], or `null` if no
165-
/// such variable is declared.
166-
FileSpan getVariableSpan(String name) {
169+
/// Returns the node for the variable named [name], or `null` if no such
170+
/// variable is declared.
171+
///
172+
/// This node is intended as a proxy for the [FileSpan] indicating where the
173+
/// variable's value originated. It's returned as an [AstNode] rather than a
174+
/// [FileSpan] so we can avoid calling [AstNode.span] if the span isn't
175+
/// required, since some nodes need to do real work to manufacture a source
176+
/// span.
177+
AstNode getVariableNode(String name) {
167178
if (_lastVariableName == name) {
168-
return _variableSpans[_lastVariableIndex][name];
179+
return _variableNodes[_lastVariableIndex][name];
169180
}
170181

171182
var index = _variableIndices[name];
172183
if (index != null) {
173184
_lastVariableName = name;
174185
_lastVariableIndex = index;
175-
return _variableSpans[index][name];
186+
return _variableNodes[index][name];
176187
}
177188

178189
index = _variableIndex(name);
@@ -181,7 +192,7 @@ class Environment {
181192
_lastVariableName = name;
182193
_lastVariableIndex = index;
183194
_variableIndices[name] = index;
184-
return _variableSpans[index][name];
195+
return _variableNodes[index][name];
185196
}
186197

187198
/// Returns whether a variable named [name] exists.
@@ -199,12 +210,17 @@ class Environment {
199210
return null;
200211
}
201212

202-
/// Sets the variable named [name] to [value], associated with the given [span].
213+
/// Sets the variable named [name] to [value], associated with
214+
/// [nodeWithSpan]'s source span.
203215
///
204216
/// If [global] is `true`, this sets the variable at the top-level scope.
205217
/// Otherwise, if the variable was already defined, it'll set it in the
206218
/// previous scope. If it's undefined, it'll set it in the current scope.
207-
void setVariable(String name, Value value, FileSpan span,
219+
///
220+
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
221+
/// [AstNode.span] if the span isn't required, since some nodes need to do
222+
/// real work to manufacture a source span.
223+
void setVariable(String name, Value value, AstNode nodeWithSpan,
208224
{bool global = false}) {
209225
if (global || _variables.length == 1) {
210226
// Don't set the index if there's already a variable with the given name,
@@ -216,7 +232,7 @@ class Environment {
216232
});
217233

218234
_variables.first[name] = value;
219-
if (_variableSpans != null) _variableSpans.first[name] = span;
235+
if (_variableNodes != null) _variableNodes.first[name] = nodeWithSpan;
220236
return;
221237
}
222238

@@ -232,20 +248,25 @@ class Environment {
232248
_lastVariableName = name;
233249
_lastVariableIndex = index;
234250
_variables[index][name] = value;
235-
if (_variableSpans != null) _variableSpans[index][name] = span;
251+
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
236252
}
237253

238-
/// Sets the variable named [name] to [value] in the current scope, associated with the given [span].
254+
/// Sets the variable named [name] to [value], associated with
255+
/// [nodeWithSpan]'s source span.
239256
///
240257
/// Unlike [setVariable], this will declare the variable in the current scope
241258
/// even if a declaration already exists in an outer scope.
242-
void setLocalVariable(String name, Value value, FileSpan span) {
259+
///
260+
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
261+
/// [AstNode.span] if the span isn't required, since some nodes need to do
262+
/// real work to manufacture a source span.
263+
void setLocalVariable(String name, Value value, AstNode nodeWithSpan) {
243264
var index = _variables.length - 1;
244265
_lastVariableName = name;
245266
_lastVariableIndex = index;
246267
_variableIndices[name] = index;
247268
_variables[index][name] = value;
248-
if (_variableSpans != null) _variableSpans[index][name] = span;
269+
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
249270
}
250271

251272
/// Returns the value of the function named [name], or `null` if no such
@@ -361,7 +382,7 @@ class Environment {
361382
_inSemiGlobalScope = semiGlobal;
362383

363384
_variables.add(normalizedMap());
364-
_variableSpans?.add(normalizedMap());
385+
_variableNodes?.add(normalizedMap());
365386
_functions.add(normalizedMap());
366387
_mixins.add(normalizedMap());
367388
try {

‎lib/src/utils.dart‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,10 +174,15 @@ Frame frameForSpan(SourceSpan span, String member, {Uri url}) => Frame(
174174
/// returns `null`.
175175
FileSpan spanForList(List<AstNode> nodes) {
176176
if (nodes.isEmpty) return null;
177+
177178
// Spans may be null for dynamically-constructed ASTs.
178-
if (nodes.first.span == null) return null;
179-
if (nodes.last.span == null) return null;
180-
return nodes.first.span.expand(nodes.last.span);
179+
var left = nodes.first.span;
180+
if (left == null) return null;
181+
182+
var right = nodes.last.span;
183+
if (right == null) return null;
184+
185+
return left.expand(right);
181186
}
182187

183188
/// Returns [name] without a vendor prefix.

0 commit comments

Comments
 (0)