From 3de531b9bc5eaf6729233977086c4354a0ed277f Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Sat, 2 Dec 2017 14:36:52 -0800 Subject: [PATCH] Don't create scopes when nothing is declared This seems to provide a fairly minimal speed boost, but it's more than nothing. It's also a pretty easy change now that we have ParentStatement. --- lib/src/ast/sass/statement/if_rule.dart | 19 +++++++-- lib/src/ast/sass/statement/parent.dart | 13 ++++++- lib/src/async_environment.dart | 32 ++++++++++++--- lib/src/environment.dart | 33 +++++++++++++--- lib/src/visitor/async_evaluate.dart | 50 ++++++++++++++---------- lib/src/visitor/evaluate.dart | 52 +++++++++++++++---------- 6 files changed, 142 insertions(+), 57 deletions(-) diff --git a/lib/src/ast/sass/statement/if_rule.dart b/lib/src/ast/sass/statement/if_rule.dart index 945b5f6a..23063843 100644 --- a/lib/src/ast/sass/statement/if_rule.dart +++ b/lib/src/ast/sass/statement/if_rule.dart @@ -7,6 +7,9 @@ import 'package:source_span/source_span.dart'; import '../../../visitor/interface/statement.dart'; import '../expression.dart'; import '../statement.dart'; +import 'function_rule.dart'; +import 'mixin_rule.dart'; +import 'variable_declaration.dart'; /// An `@if` rule. /// @@ -53,12 +56,20 @@ class IfClause { /// The statements to evaluate if this clause matches. final List children; - IfClause(this.expression, Iterable children) - : children = new List.unmodifiable(children); + /// Whether any of [children] is a variable, function, or mixin declaration. + final bool hasDeclarations; + + IfClause(Expression expression, Iterable children) + : this._(expression, new List.unmodifiable(children)); IfClause.last(Iterable children) - : expression = null, - children = new List.unmodifiable(children); + : this._(null, new List.unmodifiable(children)); + + IfClause._(this.expression, this.children) + : hasDeclarations = children.any((child) => + child is VariableDeclaration || + child is FunctionRule || + child is MixinRule); String toString() => (expression == null ? "@else" : "@if $expression") + diff --git a/lib/src/ast/sass/statement/parent.dart b/lib/src/ast/sass/statement/parent.dart index 84d3f2ad..98b8641a 100644 --- a/lib/src/ast/sass/statement/parent.dart +++ b/lib/src/ast/sass/statement/parent.dart @@ -3,11 +3,22 @@ // https://opensource.org/licenses/MIT. import '../statement.dart'; +import 'function_rule.dart'; +import 'mixin_rule.dart'; +import 'variable_declaration.dart'; /// A [Statement] that can have child statements. abstract class ParentStatement implements Statement { /// The child statements of this statement. final List children; - ParentStatement(this.children); + /// Whether any of [children] is a variable, function, or mixin declaration. + final bool hasDeclarations; + + ParentStatement(this.children) + : hasDeclarations = children?.any((child) => + child is VariableDeclaration || + child is FunctionRule || + child is MixinRule) ?? + false; } diff --git a/lib/src/async_environment.dart b/lib/src/async_environment.dart index 57b172c2..9ab42679 100644 --- a/lib/src/async_environment.dart +++ b/lib/src/async_environment.dart @@ -82,11 +82,11 @@ class AsyncEnvironment { bool get inMixin => _inMixin; var _inMixin = false; - /// Whether the environment is currently in a semi-global scope. + /// Whether the environment is currently in a global or semi-global scope. /// /// A semi-global scope can assign to global variables, but it doesn't declare /// them by default. - var _inSemiGlobalScope = false; + var _inSemiGlobalScope = true; AsyncEnvironment() : _variables = [normalizedMap()], @@ -279,10 +279,32 @@ class AsyncEnvironment { /// Variables, functions, and mixins declared in a given scope are /// inaccessible outside of it. If [semiGlobal] is passed, this scope can /// assign to global variables without a `!global` declaration. - Future scope(Future callback(), {bool semiGlobal: false}) async { - semiGlobal = semiGlobal && (_inSemiGlobalScope || _variables.length == 1); + /// + /// If [when] is false, this doesn't create a new scope and instead just + /// executes [callback] and returns its result. + Future scope(Future callback(), + {bool semiGlobal: false, bool when: true}) async { + if (!when) { + // We still have to track semi-globalness so that + // + // div { + // @if ... { + // $x: y; + // } + // } + // + // doesn't assign to the global scope. + var wasInSemiGlobalScope = _inSemiGlobalScope; + _inSemiGlobalScope = semiGlobal; + try { + return await callback(); + } finally { + _inSemiGlobalScope = wasInSemiGlobalScope; + } + } + + semiGlobal = semiGlobal && _inSemiGlobalScope; - // TODO: avoid creating a new scope if no variables are declared. var wasInSemiGlobalScope = _inSemiGlobalScope; _inSemiGlobalScope = semiGlobal; _variables.add(normalizedMap()); diff --git a/lib/src/environment.dart b/lib/src/environment.dart index f4fa839c..7e4dcde9 100644 --- a/lib/src/environment.dart +++ b/lib/src/environment.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_environment.dart. // See tool/synchronize.dart for details. // -// Checksum: 97410abbd78c3bbc9899f3ac460cc0736218bfe3 +// Checksum: 4669f41a70664bd5f391c6b8627264a5d0ad8f6c import 'ast/sass.dart'; import 'callable.dart'; @@ -85,11 +85,11 @@ class Environment { bool get inMixin => _inMixin; var _inMixin = false; - /// Whether the environment is currently in a semi-global scope. + /// Whether the environment is currently in a global or semi-global scope. /// /// A semi-global scope can assign to global variables, but it doesn't declare /// them by default. - var _inSemiGlobalScope = false; + var _inSemiGlobalScope = true; Environment() : _variables = [normalizedMap()], @@ -282,10 +282,31 @@ class Environment { /// Variables, functions, and mixins declared in a given scope are /// inaccessible outside of it. If [semiGlobal] is passed, this scope can /// assign to global variables without a `!global` declaration. - T scope(T callback(), {bool semiGlobal: false}) { - semiGlobal = semiGlobal && (_inSemiGlobalScope || _variables.length == 1); + /// + /// If [when] is false, this doesn't create a new scope and instead just + /// executes [callback] and returns its result. + T scope(T callback(), {bool semiGlobal: false, bool when: true}) { + if (!when) { + // We still have to track semi-globalness so that + // + // div { + // @if ... { + // $x: y; + // } + // } + // + // doesn't assign to the global scope. + var wasInSemiGlobalScope = _inSemiGlobalScope; + _inSemiGlobalScope = semiGlobal; + try { + return callback(); + } finally { + _inSemiGlobalScope = wasInSemiGlobalScope; + } + } + + semiGlobal = semiGlobal && _inSemiGlobalScope; - // TODO: avoid creating a new scope if no variables are declared. var wasInSemiGlobalScope = _inSemiGlobalScope; _inSemiGlobalScope = semiGlobal; _variables.add(normalizedMap()); diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 113c22d6..13828776 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -327,7 +327,7 @@ class _EvaluateVisitor for (var child in node.children) { await child.accept(this); } - }); + }, when: node.hasDeclarations); return null; } @@ -341,7 +341,7 @@ class _EvaluateVisitor } if (outerCopy != null) root.addChild(outerCopy); - await _scopeForAtRoot(innerCopy ?? root, query, included)(() async { + await _scopeForAtRoot(node, innerCopy ?? root, query, included)(() async { for (var child in node.children) { await child.accept(this); } @@ -386,14 +386,14 @@ class _EvaluateVisitor /// This returns a callback that adjusts various instance variables for its /// duration, based on which rules are excluded by [query]. It always assigns /// [_parent] to [newParent]. - _ScopeCallback _scopeForAtRoot(CssParentNode newParent, AtRootQuery query, - List included) { + _ScopeCallback _scopeForAtRoot(AtRootRule node, CssParentNode newParent, + AtRootQuery query, List included) { var scope = (Future callback()) async { // We can't use [_withParent] here because it'll add the node to the tree // in the wrong place. var oldParent = _parent; _parent = newParent; - await _environment.scope(callback); + await _environment.scope(callback, when: node.hasDeclarations); _parent = oldParent; }; @@ -488,7 +488,7 @@ class _EvaluateVisitor for (var child in node.children) { await child.accept(this); } - }); + }, when: node.hasDeclarations); _declarationName = oldDeclarationName; } @@ -588,9 +588,11 @@ class _EvaluateVisitor for (var child in node.children) { await child.accept(this); } - }); + }, scopeWhen: false); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); _inUnknownAtRule = wasInUnknownAtRule; _inKeyframes = wasInKeyframes; @@ -645,7 +647,8 @@ class _EvaluateVisitor return await _environment.scope( () => _handleReturn( clause.children, (child) => child.accept(this)), - semiGlobal: true); + semiGlobal: true, + when: clause.hasDeclarations); } Future visitImportRule(ImportRule node) async { @@ -878,10 +881,12 @@ class _EvaluateVisitor for (var child in node.children) { await child.accept(this); } - }); + }, scopeWhen: false); } }); - }, through: (node) => node is CssStyleRule || node is CssMediaRule); + }, + through: (node) => node is CssStyleRule || node is CssMediaRule, + scopeWhen: node.hasDeclarations); return null; } @@ -931,7 +936,9 @@ class _EvaluateVisitor for (var child in node.children) { await child.accept(this); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); return null; } @@ -955,7 +962,9 @@ class _EvaluateVisitor await child.accept(this); } }); - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); _atRootExcludingStyleRule = oldAtRootExcludingStyleRule; if (!_inStyleRule) { @@ -992,7 +1001,9 @@ class _EvaluateVisitor } }); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); return null; } @@ -1068,7 +1079,7 @@ class _EvaluateVisitor if (result != null) return result; } return null; - }, semiGlobal: true); + }, semiGlobal: true, when: node.hasDeclarations); } // ## Expressions @@ -1250,9 +1261,6 @@ class _EvaluateVisitor _verifyArguments( positional.length, named, callable.declaration.arguments, span); - // TODO: if we get here and there are no rest params involved, mark - // the callable as fast-path and don't do error checking or extra - // allocations for future calls. var declaredArguments = callable.declaration.arguments.arguments; var minLength = math.min(positional.length, declaredArguments.length); for (var i = 0; i < minLength; i++) { @@ -1633,9 +1641,11 @@ class _EvaluateVisitor /// If [through] is passed, [node] is added as a child of the first parent for /// which [through] returns `false`. That parent is copied unless it's the /// lattermost child of its parent. + /// + /// Runs [callback] in a new environment scope unless [scopeWhen] is false. Future _withParent( S node, Future callback(), - {bool through(CssNode node)}) async { + {bool through(CssNode node), bool scopeWhen: true}) async { var oldParent = _parent; // Go up through parents that match [through]. @@ -1657,7 +1667,7 @@ class _EvaluateVisitor parent.addChild(node); _parent = node; - var result = await _environment.scope(callback); + var result = await _environment.scope(callback, when: scopeWhen); _parent = oldParent; return result; diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index aadfaebb..0b51614e 100644 --- a/lib/src/visitor/evaluate.dart +++ b/lib/src/visitor/evaluate.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_evaluate.dart. // See tool/synchronize.dart for details. // -// Checksum: 33e96540f3e5999ed172d168221b27910e8672b1 +// Checksum: cdeeec2634c97638aef571043c0be1e99f22d7d5 import 'dart:math' as math; @@ -328,7 +328,7 @@ class _EvaluateVisitor for (var child in node.children) { child.accept(this); } - }); + }, when: node.hasDeclarations); return null; } @@ -342,7 +342,7 @@ class _EvaluateVisitor } if (outerCopy != null) root.addChild(outerCopy); - _scopeForAtRoot(innerCopy ?? root, query, included)(() { + _scopeForAtRoot(node, innerCopy ?? root, query, included)(() { for (var child in node.children) { child.accept(this); } @@ -387,14 +387,14 @@ class _EvaluateVisitor /// This returns a callback that adjusts various instance variables for its /// duration, based on which rules are excluded by [query]. It always assigns /// [_parent] to [newParent]. - _ScopeCallback _scopeForAtRoot(CssParentNode newParent, AtRootQuery query, - List included) { + _ScopeCallback _scopeForAtRoot(AtRootRule node, CssParentNode newParent, + AtRootQuery query, List included) { var scope = (void callback()) { // We can't use [_withParent] here because it'll add the node to the tree // in the wrong place. var oldParent = _parent; _parent = newParent; - _environment.scope(callback); + _environment.scope(callback, when: node.hasDeclarations); _parent = oldParent; }; @@ -487,7 +487,7 @@ class _EvaluateVisitor for (var child in node.children) { child.accept(this); } - }); + }, when: node.hasDeclarations); _declarationName = oldDeclarationName; } @@ -583,9 +583,11 @@ class _EvaluateVisitor for (var child in node.children) { child.accept(this); } - }); + }, scopeWhen: false); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); _inUnknownAtRule = wasInUnknownAtRule; _inKeyframes = wasInKeyframes; @@ -640,7 +642,8 @@ class _EvaluateVisitor return _environment.scope( () => _handleReturn( clause.children, (child) => child.accept(this)), - semiGlobal: true); + semiGlobal: true, + when: clause.hasDeclarations); } Value visitImportRule(ImportRule node) { @@ -872,10 +875,12 @@ class _EvaluateVisitor for (var child in node.children) { child.accept(this); } - }); + }, scopeWhen: false); } }); - }, through: (node) => node is CssStyleRule || node is CssMediaRule); + }, + through: (node) => node is CssStyleRule || node is CssMediaRule, + scopeWhen: node.hasDeclarations); return null; } @@ -922,7 +927,9 @@ class _EvaluateVisitor for (var child in node.children) { child.accept(this); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); return null; } @@ -946,7 +953,9 @@ class _EvaluateVisitor child.accept(this); } }); - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); _atRootExcludingStyleRule = oldAtRootExcludingStyleRule; if (!_inStyleRule) { @@ -983,7 +992,9 @@ class _EvaluateVisitor } }); } - }, through: (node) => node is CssStyleRule); + }, + through: (node) => node is CssStyleRule, + scopeWhen: node.hasDeclarations); return null; } @@ -1058,7 +1069,7 @@ class _EvaluateVisitor if (result != null) return result; } return null; - }, semiGlobal: true); + }, semiGlobal: true, when: node.hasDeclarations); } // ## Expressions @@ -1231,9 +1242,6 @@ class _EvaluateVisitor _verifyArguments( positional.length, named, callable.declaration.arguments, span); - // TODO: if we get here and there are no rest params involved, mark - // the callable as fast-path and don't do error checking or extra - // allocations for future calls. var declaredArguments = callable.declaration.arguments.arguments; var minLength = math.min(positional.length, declaredArguments.length); for (var i = 0; i < minLength; i++) { @@ -1604,8 +1612,10 @@ class _EvaluateVisitor /// If [through] is passed, [node] is added as a child of the first parent for /// which [through] returns `false`. That parent is copied unless it's the /// lattermost child of its parent. + /// + /// Runs [callback] in a new environment scope unless [scopeWhen] is false. T _withParent(S node, T callback(), - {bool through(CssNode node)}) { + {bool through(CssNode node), bool scopeWhen: true}) { var oldParent = _parent; // Go up through parents that match [through]. @@ -1627,7 +1637,7 @@ class _EvaluateVisitor parent.addChild(node); _parent = node; - var result = _environment.scope(callback); + var result = _environment.scope(callback, when: scopeWhen); _parent = oldParent; return result;