Skip to content

Commit

Permalink
Fix a performance bug in BinaryOperatorExpression.span (#569)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@nex3
nex3 authored Jan 17, 2019
1 parent b024276 commit 9fdf561
Show file tree
Hide file tree
Showing 8 changed files with 453 additions and 296 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 1.16.1

* Fix a performance bug where stylesheet evaluation could take a very long time
when many binary operators were used in sequence.

## 1.16.0

* `rgb()` and `hsl()` now treat unquoted strings beginning with `env()`,
Expand Down
15 changes: 14 additions & 1 deletion lib/src/ast/sass/expression/binary_operation.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,20 @@ class BinaryOperationExpression implements Expression {
/// interpreted as slash-separated numbers.
final bool allowsSlash;

FileSpan get span => spanForList([left, right]);
FileSpan get span {
// Avoid creating a bunch of intermediate spans for multiple binary
// expressions in a row by moving to the left- and right-most expressions.
var left = this.left;
while (left is BinaryOperationExpression) {
left = (left as BinaryOperationExpression).left;
}

var right = this.right;
while (right is BinaryOperationExpression) {
right = (right as BinaryOperationExpression).right;
}
return spanForList([left, right]);
}

BinaryOperationExpression(this.operator, this.left, this.right)
: allowsSlash = false;
Expand Down
59 changes: 40 additions & 19 deletions lib/src/async_environment.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';

import 'package:source_span/source_span.dart';

import 'ast/node.dart';
import 'callable.dart';
import 'functions.dart';
import 'value.dart';
Expand All @@ -26,10 +27,14 @@ class AsyncEnvironment {
/// deeper in the tree.
final List<Map<String, Value>> _variables;

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

/// A map of variable names to their indices in [_variables].
///
Expand Down Expand Up @@ -104,7 +109,7 @@ class AsyncEnvironment {
/// If [sourceMap] is `true`, this tracks variables' source locations
AsyncEnvironment({bool sourceMap = false})
: _variables = [normalizedMap()],
_variableSpans = sourceMap ? [normalizedMap()] : null,
_variableNodes = sourceMap ? [normalizedMap()] : null,
_variableIndices = normalizedMap(),
_functions = [normalizedMap()],
_functionIndices = normalizedMap(),
Expand All @@ -113,7 +118,7 @@ class AsyncEnvironment {
coreFunctions.forEach(setFunction);
}

AsyncEnvironment._(this._variables, this._variableSpans, this._functions,
AsyncEnvironment._(this._variables, this._variableNodes, this._functions,
this._mixins, this._content)
// Lazily fill in the indices rather than eagerly copying them from the
// existing environment in closure() because the copying took a lot of
Expand All @@ -130,7 +135,7 @@ class AsyncEnvironment {
/// when the closure was created will be reflected.
AsyncEnvironment closure() => AsyncEnvironment._(
_variables.toList(),
_variableSpans?.toList(),
_variableNodes?.toList(),
_functions.toList(),
_mixins.toList(),
_content);
Expand All @@ -156,18 +161,24 @@ class AsyncEnvironment {
return _variables[index][name];
}

/// Returns the source span for the variable named [name], or `null` if no
/// such variable is declared.
FileSpan getVariableSpan(String name) {
/// Returns the node for the variable named [name], or `null` if no such
/// variable is declared.
///
/// This node is intended as a proxy for the [FileSpan] indicating where the
/// variable's value originated. It's returned as an [AstNode] rather than a
/// [FileSpan] so we can avoid calling [AstNode.span] if the span isn't
/// required, since some nodes need to do real work to manufacture a source
/// span.
AstNode getVariableNode(String name) {
if (_lastVariableName == name) {
return _variableSpans[_lastVariableIndex][name];
return _variableNodes[_lastVariableIndex][name];
}

var index = _variableIndices[name];
if (index != null) {
_lastVariableName = name;
_lastVariableIndex = index;
return _variableSpans[index][name];
return _variableNodes[index][name];
}

index = _variableIndex(name);
Expand All @@ -176,7 +187,7 @@ class AsyncEnvironment {
_lastVariableName = name;
_lastVariableIndex = index;
_variableIndices[name] = index;
return _variableSpans[index][name];
return _variableNodes[index][name];
}

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

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

_variables.first[name] = value;
if (_variableSpans != null) _variableSpans.first[name] = span;
if (_variableNodes != null) _variableNodes.first[name] = nodeWithSpan;
return;
}

Expand All @@ -227,20 +243,25 @@ class AsyncEnvironment {
_lastVariableName = name;
_lastVariableIndex = index;
_variables[index][name] = value;
if (_variableSpans != null) _variableSpans[index][name] = span;
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
}

/// Sets the variable named [name] to [value] in the current scope, associated with the given [span].
/// Sets the variable named [name] to [value], associated with
/// [nodeWithSpan]'s source span.
///
/// Unlike [setVariable], this will declare the variable in the current scope
/// even if a declaration already exists in an outer scope.
void setLocalVariable(String name, Value value, FileSpan span) {
///
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
/// [AstNode.span] if the span isn't required, since some nodes need to do
/// real work to manufacture a source span.
void setLocalVariable(String name, Value value, AstNode nodeWithSpan) {
var index = _variables.length - 1;
_lastVariableName = name;
_lastVariableIndex = index;
_variableIndices[name] = index;
_variables[index][name] = value;
if (_variableSpans != null) _variableSpans[index][name] = span;
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
}

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

_variables.add(normalizedMap());
_variableSpans?.add(normalizedMap());
_variableNodes?.add(normalizedMap());
_functions.add(normalizedMap());
_mixins.add(normalizedMap());
try {
Expand Down
61 changes: 41 additions & 20 deletions lib/src/environment.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@
// DO NOT EDIT. This file was generated from async_environment.dart.
// See tool/synchronize.dart for details.
//
// Checksum: 9e0f9274f4778b701f268bcf4fc349a1cf17a159
// Checksum: 449ed8a8ad29fe107656a666e6e6005ef539b834
//
// ignore_for_file: unused_import

import 'package:source_span/source_span.dart';

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

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

/// A map of variable names to their indices in [_variables].
///
Expand Down Expand Up @@ -109,7 +114,7 @@ class Environment {
/// If [sourceMap] is `true`, this tracks variables' source locations
Environment({bool sourceMap = false})
: _variables = [normalizedMap()],
_variableSpans = sourceMap ? [normalizedMap()] : null,
_variableNodes = sourceMap ? [normalizedMap()] : null,
_variableIndices = normalizedMap(),
_functions = [normalizedMap()],
_functionIndices = normalizedMap(),
Expand All @@ -118,7 +123,7 @@ class Environment {
coreFunctions.forEach(setFunction);
}

Environment._(this._variables, this._variableSpans, this._functions,
Environment._(this._variables, this._variableNodes, this._functions,
this._mixins, this._content)
// Lazily fill in the indices rather than eagerly copying them from the
// existing environment in closure() because the copying took a lot of
Expand All @@ -135,7 +140,7 @@ class Environment {
/// when the closure was created will be reflected.
Environment closure() => Environment._(
_variables.toList(),
_variableSpans?.toList(),
_variableNodes?.toList(),
_functions.toList(),
_mixins.toList(),
_content);
Expand All @@ -161,18 +166,24 @@ class Environment {
return _variables[index][name];
}

/// Returns the source span for the variable named [name], or `null` if no
/// such variable is declared.
FileSpan getVariableSpan(String name) {
/// Returns the node for the variable named [name], or `null` if no such
/// variable is declared.
///
/// This node is intended as a proxy for the [FileSpan] indicating where the
/// variable's value originated. It's returned as an [AstNode] rather than a
/// [FileSpan] so we can avoid calling [AstNode.span] if the span isn't
/// required, since some nodes need to do real work to manufacture a source
/// span.
AstNode getVariableNode(String name) {
if (_lastVariableName == name) {
return _variableSpans[_lastVariableIndex][name];
return _variableNodes[_lastVariableIndex][name];
}

var index = _variableIndices[name];
if (index != null) {
_lastVariableName = name;
_lastVariableIndex = index;
return _variableSpans[index][name];
return _variableNodes[index][name];
}

index = _variableIndex(name);
Expand All @@ -181,7 +192,7 @@ class Environment {
_lastVariableName = name;
_lastVariableIndex = index;
_variableIndices[name] = index;
return _variableSpans[index][name];
return _variableNodes[index][name];
}

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

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

_variables.first[name] = value;
if (_variableSpans != null) _variableSpans.first[name] = span;
if (_variableNodes != null) _variableNodes.first[name] = nodeWithSpan;
return;
}

Expand All @@ -232,20 +248,25 @@ class Environment {
_lastVariableName = name;
_lastVariableIndex = index;
_variables[index][name] = value;
if (_variableSpans != null) _variableSpans[index][name] = span;
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
}

/// Sets the variable named [name] to [value] in the current scope, associated with the given [span].
/// Sets the variable named [name] to [value], associated with
/// [nodeWithSpan]'s source span.
///
/// Unlike [setVariable], this will declare the variable in the current scope
/// even if a declaration already exists in an outer scope.
void setLocalVariable(String name, Value value, FileSpan span) {
///
/// This takes an [AstNode] rather than a [FileSpan] so it can avoid calling
/// [AstNode.span] if the span isn't required, since some nodes need to do
/// real work to manufacture a source span.
void setLocalVariable(String name, Value value, AstNode nodeWithSpan) {
var index = _variables.length - 1;
_lastVariableName = name;
_lastVariableIndex = index;
_variableIndices[name] = index;
_variables[index][name] = value;
if (_variableSpans != null) _variableSpans[index][name] = span;
if (_variableNodes != null) _variableNodes[index][name] = nodeWithSpan;
}

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

_variables.add(normalizedMap());
_variableSpans?.add(normalizedMap());
_variableNodes?.add(normalizedMap());
_functions.add(normalizedMap());
_mixins.add(normalizedMap());
try {
Expand Down
11 changes: 8 additions & 3 deletions lib/src/utils.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,15 @@ Frame frameForSpan(SourceSpan span, String member, {Uri url}) => Frame(
/// returns `null`.
FileSpan spanForList(List<AstNode> nodes) {
if (nodes.isEmpty) return null;

// Spans may be null for dynamically-constructed ASTs.
if (nodes.first.span == null) return null;
if (nodes.last.span == null) return null;
return nodes.first.span.expand(nodes.last.span);
var left = nodes.first.span;
if (left == null) return null;

var right = nodes.last.span;
if (right == null) return null;

return left.expand(right);
}

/// Returns [name] without a vendor prefix.
Expand Down
Loading

0 comments on commit 9fdf561

Please sign in to comment.